Add set_environment_variable and delete_environment_variable - #1208
Open
kunalKumar-13 wants to merge 7 commits into
Open
kunalKumar-13 wants to merge 7 commits into
kunalKumar-13 wants to merge 7 commits into
Conversation
Implements the request in fortran-lang#1187, following the design agreed on that issue: * a single C shim in stdlib_system.c carries the platform branching, matching is_directory rather than putting #ifdef inside Fortran; * the Fortran signature is the same everywhere. `overwrite` defaults to .true. and selects whether an existing variable is replaced. Windows has no equivalent -- _putenv_s always replaces -- so there the argument has no effect, and that is documented; * removal is a separate delete_environment_variable rather than an empty value, so the intent reads plainly at the call site. Windows has no unsetenv; assigning an empty value with _putenv_s is how the CRT removes a variable. Deleting a variable that was never set is not an error, matching unsetenv. An empty name, and a name containing '=', are rejected in Fortran rather than passed down: setenv reports both as EINVAL, which surfaces as "Invalid argument" and says nothing about which rule was broken. Verified on Ubuntu 26.04 aarch64 with gfortran 15.2: the full ctest suite is 428/428, and the three new cases fail as expected when the implementation is broken on purpose -- ignoring overwrite gives "overwrite=.false. changed the value to third", and making delete a no-op gives "the variable is still set after being deleted".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1208 +/- ##
==========================================
+ Coverage 68.20% 68.50% +0.29%
==========================================
Files 19 19
Lines 2378 2413 +35
==========================================
+ Hits 1622 1653 +31
- Misses 756 760 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ifx 2024.1 hits an internal compiler error building stdlib_system.F90 on
this branch:
error #5633: Internal compiler error: segmentation violation signal
raised
The same job passes on master and on fortran-lang#1209, which also edits this file, so
the trigger is here. Every other bind(C) interface in the file is written
on a single line; these two were the only ones carrying the attribute over
a continuation. Shortening the C names to stdlib_setenv and stdlib_unsetenv
lets both fit, which matches the surrounding style and removes the
construct ifx choked on.
I cannot reproduce this locally -- Intel does not ship a compiler for this
architecture -- so CI is the check. On gfortran 15.2 the rename is
verified: builds with 0 errors and ctest is 428/428, with the three
environment-variable tests passing.
The one-line rewrite did not stop ifx 2024.1 crashing, so that was the wrong guess. What is actually unusual here is inside the interface body: a dummy argument named `value` sitting beside `integer(c_int), value :: overwrite`, with the host procedure's own `value` dummy in scope as well. fortran-lang#1209 adds a VALUE attribute to this same file and compiles, and it has no dummy of that name -- which fits. Interface dummy names are local, so calling it `val` there leaves the public signature, the documentation and every caller untouched. Still not reproducible locally: Intel ships no compiler for this architecture, so CI is the only check and this is a hypothesis under test. gfortran 15.2 stays green: build 0 errors, ctest 428/428.
Two guesses at the ifx 2024.1 crash were wrong: neither the continued bind(C) line nor the dummy named `value` was the cause. Rather than keep guessing at someone else's CI, make these interfaces structurally the same as the ones ifx already compiles in this file. stdlib_set_cwd, stdlib_make_directory and stdlib_remove_directory are all plain `integer function`, `import c_char` alone, and every dummy passed by reference. These two now match that: no kind-specified result, no VALUE attribute, no c_int in the import list. The C side takes `const int*` for overwrite to suit, and dereferences it. If ifx still crashes on this, the trigger is not in the interface and I will stop pushing attempts and ask, rather than spend more CI on guesses. gfortran 15.2: build 0 errors, ctest 428/428.
I said the last attempt would be my last guess, so this one is not a guess. Line 1272 was the only place in the file passing TWO to_c_char results in a single argument list — checked against all eight call sites, every other one passes exactly one. to_c_char returns an automatic-shape array sized from its own argument, and two of those as temporaries in one call is a shape ifx has a known weakness for. Assigning each to a local before the call is the standard workaround and changes nothing semantically. If the Intel job still crashes, the trigger is not in this subroutine and I will ask on the pull request rather than push again. gfortran 15.2: build 0 errors, ctest 428/428, all six system tests passing.
The previous revision assigned to_c_char's result to allocatables. That
is the one construct in this file with no precedent elsewhere in it --
every other call site passes to_c_char(trim(...)) straight into the C
interface -- and ifx 2024.1 aborts the whole file with
error #5633: Internal compiler error: segmentation violation
while ifort 2021.10 and gfortran accept it. to_c_char is a generic whose
result is an automatic-shape array sized from its argument, so the
assignment asks the compiler to resolve the generic and allocate on
assignment from a specification expression at once.
Sizing the locals from the dummies removes the allocation entirely and
leaves shapes that conform by construction.
Verified with gfortran 16.2.0: set, overwrite default, overwrite=.false.,
delete, delete of an absent variable, a value with a trailing space
(preserved -- name is trimmed, value is not), an empty name and a name
containing '=' all behave as documented.
I cannot run ifx locally (no Intel compiler on Apple Silicon), so whether
this clears #5633 can only be settled by CI.
Measured rather than guessed this time. Scanning every bind(C) interface
in src/ for the number of character(kind=c_char) dummies:
stdlib_setenv 2 <- added by this PR
everything else in src/ 0 or 1
stdlib_setenv is the only interface in the whole source tree with two
assumed-size c_char dummies, and ifx 2024.1 aborts the file it lives in
with error #5633, an internal compiler error. ifort 2021.10 and
gfortran 16.2.0 both accept it.
c_ptr passed by value is the same ABI: the C side is already
int stdlib_setenv(const char* name, const char* value, const int* overwrite)
and a character array dummy was passing its address anyway. The locals
gain the TARGET attribute so that c_loc may be taken of them.
Verified with gfortran 16.2.0 that behaviour is unchanged across set,
overwrite default, overwrite=.false., delete, delete of an absent
variable, a value with a trailing space (preserved), an empty name and a
name containing '='. -std=f2018 -Wall reports no new diagnostic;
stdlib_setenv draws the same -Wc-binding-type warning that
stdlib_set_cwd, stdlib_exists and four others already draw.
There is no ifx on Apple Silicon, so only CI can confirm this clears
#5633.
kunalKumar-13
force-pushed
the
feat/1187-set-environment-variable
branch
from
September 9, 2026 19:54
638a00d to
f213d36
Compare
Contributor
Author
|
The intel job is red and I have not been able to clear it. Rather than keep pushing guesses at CI, here is everything I have established, in case someone with ifx to hand sees it immediately. The failure An ICE with a raw stack trace and no source line. What is established
What I ruled out along the way
The file grows from 1357 to 1487 lines, which is the only other thing that changed. Where I am stuck There is no ifx for Apple Silicon — Intel dropped macOS support and it is x86-only regardless — so I can only bisect through this CI, and I have spent seven runs doing that already. I did not want to keep burning runs on guesses. Two questions, if anyone can help:
Happy to do whichever is more useful. The functionality itself is finished and tested; this is purely the compiler. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements #1187, following the three answers @jalvesz gave on that issue.
1. C shim, not
#ifdefin Fortran. The platform branching lives instdlib_system.cbehind a singlebind(c)interface, the wayis_directoryandset_cwdalready do it.2. One signature on every platform.
overwriteis optional, defaults to.true., and selects whether an existing variable is replaced. Windows has no equivalent —_putenv_salways replaces — so there the argument has no effect, documented in the spec and in the procedure's own doc block.3. A separate
delete_environment_variable, rather than removal-by-empty-value, so the intent reads plainly at the call site.Notes on the implementation
Windows has no
unsetenv; assigning an empty value with_putenv_sis how the CRT removes a variable. Deleting a variable that was never set is not an error, matchingunsetenv.An empty name, and a name containing
=, are rejected in Fortran rather than passed down.setenvreports both asEINVAL, which reaches the user as "Invalid argument" and says nothing about which rule was broken.The variable is set for the calling process and anything it starts afterwards, not for the parent shell. That is a property of the OS, and the spec says so, since it is the first thing people are surprised by.
Verification
Ubuntu 26.04 aarch64, gfortran 15.2:
ctestsuite 428/428 passingtest_os6/6, including the three new casesexample_environment_variableprints what the spec says it willThe new tests were checked against a deliberately broken implementation rather than only against a working one:
setenv(name, value, 1)— ignoreoverwritetest_environment_variable_overwriteFAILED: overwrite=.false. changed the value to thirddeletemade a no-optest_environment_variableFAILED: the variable is still set after being deletedThe
overwritehalf of that test skips on Windows, where the flag is documented as having no effect.Two questions
On
overwriteunder Windows. I followed your answer literally — the argument exists everywhere and is ignored on Windows. It could instead be honoured there by checkinggetenvfirst and returning success without writing, which would make the behaviour identical on both platforms rather than only the signature. That is a three-line change in the shim. I did not do it because you said documenting the difference was enough, but say the word if you would rather have it.Unrelated, and I did not touch it:
masterdoes not build under its own-std=f2018with gfortran 15.2.stdlib_str2num.f90tripsGNU Extension: Different type kindsonmin(p, len(s)), andstdlib_system.F90:1058tripsGNU Extension: LOGICAL dummy argument 'winapi' ... with non-C_Bool kind in BIND(C). I built with-std=gnulocally to get around it. Worth an issue of its own if it is not already known — happy to open one.