Fix Issue 18433 - rdmd doesn't respect DFLAGS for its cache hash

[wilzbach]

Note this is only part of the required fix.
DMD at the moment doesn't allow to incrementally add to DFLAGS, so this needs to be fixed before this can be merged:

> DFLAGS="-version=Foo" ../dmd/generated/linux/release/64/dmd -c -v foo.d

predefs   DigitalMars Posix linux ELFv1 LittleEndian D_Version2 all D_SIMD D_InlineAsm_X86_64 X86_64 CRuntime_Glibc D_LP64 D_PIC assert D_HardFloat
binary    ../dmd/generated/linux/release/64/dmd
version   v2.079.0-284-g23b2e2e0d
config    ../dmd/generated/linux/release/64/dmd.conf
DFLAGS    -I../dmd/generated/linux/release/64/../../../../../druntime/import -I../dmd/generated/linux/release/64/../../../../../phobos -L-L../dmd/generated/linux/release/64/../../../../../phobos/generated/linux/release/64 -L--export-dynamic -fPIC

(no Foo set here)

> DFLAGS="-version=Foo" dmd -conf= -I~/dlang/phobos -I~/dlang/druntime/import -c -v foo.d

predefs   Foo DigitalMars Posix linux ELFv1 LittleEndian D_Version2 all D_SIMD D_InlineAsm_X86_64 X86_64 CRuntime_Glibc D_LP64 D_PIC assert D_HardFloat
binary    /home/seb/dlang/dmd/generated/linux/release/64/dmd
version   v2.079.0-284-g23b2e2e0d
config    
DFLAGS    -version=Foo

Fixes #424

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18433	enhancement	rdmd doesn't respect DFLAGS for its cache hash

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + tools#343"

[wilzbach]

Hmm apparently this was done on purpose:

The dmd.conf file overrides any DFLAGS environment variable setting prior to running dmd. This is by design.

https://issues.dlang.org/show_bug.cgi?id=1660

I strongly disagree with this decision.
Note that dub has the expected behavior:

cat > foo.d << EOF
/+dub.sdl:
+/
void main(){
    import std.stdio;
    version(Foo)
    {
        "foo".writeln;
    }
    else
    {
        "bar".writeln;
    }
}
EOF
DFLAGS="-version=Foo" dub --single -v foo.d
/usr/bin/dmd -c -of.dub/build/application-$DFLAGS-linux.posix-x86_64-dmd_2079-DECA6118698521B3160990E7A5661AB0/foo.o -w -version=Have_foo -version=Foo foo.d -vcolumns
Linking...
/usr/bin/dmd -of.dub/build/application-$DFLAGS-linux.posix-x86_64-dmd_2079-DECA6118698521B3160990E7A5661AB0/foo .dub/build/application-$DFLAGS-linux.posix-x86_64-dmd_2079-DECA6118698521B3160990E7A5661AB0/foo.o -L--no-as-needed
Copying target from /home/seb/dlang/dmd/.dub/build/application-$DFLAGS-linux.posix-x86_64-dmd_2079-DECA6118698521B3160990E7A5661AB0/foo to /home/seb/dlang/dmd
Running ./foo 
foo

[marler8997]

OPTLINK does the same thing, the config file overrides any environment variable (I wasted a bunch of time figuring this out).

The intuitive priority to me would be:

1. Command Line Option
2. Environment Variable
3. Config File

Not sure if this order can be changed without breaking a bunch of stuff.

In any case, whether or not the order can be changed, rdmd would more "more correct" if it could read the config file, in which case the code that reads/parses the config file should be shared between the compiler and rdmd. It's just that it becomes "more important" when the priority of the config file is higher.

[wilzbach]

Not sure if this order can be changed without breaking a bunch of stuff.

Not entirely sure either, but what if DMD would append to the DFLAGS environment variable (or an internal copy to be precise) instead of overwriting it?
(see also: https://github.com/dlang/dmd/blob/master/src/dmd/dinifile.d#L139)

We could even trigger a deprecation/warning a la "Hey your exported DFLAGS variable has no effect. We have been replacing it silently with the settings in this config file."

[CyberShadow]

Was about to say this - dmd ignores the DFLAGS environment variable.

Close?

[marler8997]

Appending might be odd if there are duplicate options...you could get multuple standard libraries for example. We could write some logic to merge the options but that could get complex. Not sure what the right move is here.

I like the warning idea.

Maybe we start by seeing if changing the priority is even something Walter would want. Maybe he had a reason for the current priority?

[CyberShadow]

IIRC Walter has a general dislike for getting things from the environment. He said something along the lines of that anything could put stuff there (esp. on Windows where any program can edit the default environment, and they often do), and it can be hard to debug issues when things in the environment are causing problems (because you need to know to look there).

[marler8997]

@CyberShadow Yeah I can understand this sentiment. I would be fine with completely removing support for DFLAGS as an environment variable. Here are the options I see:

1. Make the DFLAGS environment variable a higher priority than the config file. This would allow rdmd to more closely match DMD's behavior without having to process the config file, though, to match it perfectly it would still need to process it

2. Remove support for DFLAGS as an environment variable. I would add a warning like @wilzbach suggested when it is overriden by the config file, and I would start deprecation of DFLAGS when set via an environment variable.

3. Leave everything as is. We could mimic DMD's behavior in rdmd by reusing the code that DMD uses to process the config file, and also have it reuse the same logic to fallback to getting DFLAGS as an environment variable. Or we could just forget about this use case and say that rdmd won't work correctly when DFLAGS is set, in which case we could just say that it is not recommended to use DFLAGS as an environment variable otherwise you will get undefined/unsupported behavior.

[CyberShadow]

Make the DFLAGS environment variable a higher priority than the config file.

I think this would actually break my setup from a few years ago, where I effectively aliased dmd to dmd $DFLAGS. Making dmd read DFLAGS suddenly would mean that the options would be parsed twice - which should be fine in most cases, but maybe not when something is parsed between the two and the later DFLAGS ends up canceling it.

Remove support for DFLAGS as an environment variable. I would add a warning like @wilzbach suggested when it is overriden by the config file, and I would start deprecation of DFLAGS when set via an environment variable.

Why would you want to add a warning when it is overridden by the config file? In that situation, removing it completely will have no effect. Did you mean to say that a warning will appear when it is not overridden by the config file?

Leave everything as is. We could mimic DMD's behavior in rdmd by reusing the code that DMD uses to process the config file, and also have it reuse the same logic to fallback to getting DFLAGS as an environment variable. Or we could just forget about this use case and say that rdmd won't work correctly when DFLAGS is set, in which case we could just say that it is not recommended to use DFLAGS as an environment variable otherwise you will get undefined/unsupported behavior.

Sorry, I've completely lost track of what this is about. If DFLAGS can currently only be set by changing the configuration file, then all we need to do is ensure that the configuration file is one of the "dependencies" that rdmd checks the modification time of, which I believe it already does. Right?

[wilzbach]

If DFLAGS can currently only be set by changing the configuration file, then all we need to do is ensure that the configuration file is one of the "dependencies" that rdmd checks the modification time of, which I believe it already does. Right?

No, DMD does parse DFLAGS from the environment (it does so always), it just replaces that string with the content of dmd.conf.
However, as pointed out in the opening comment when -conf= is passed, dmd does parse DFLAGS
Even DMD's testsuite depends on this behavior:

https://github.com/dlang/dmd/blob/master/test/Makefile#L101

Why would you want to add a warning when it is overridden by the config file?

The idea was to let this code issue a warning:

DFLAGS=-version=Foo dmd

(the user may have expected dmd to behave otherwise)

But thinking about it, it probably doesn't solve our problem either :/

An non-breaking option would be to introduce an -env flag for dmd which tells it to have higher priority for environment flags (or append them to dmd.conf).

[marler8997]

@CyberShadow

The current state of DFLAGS is that it will read it from either the config file, or as an environment variable in that order. If it doesn't read it from the config file (or if you provide -conf=) then it will check if it exists as an environment variable. It seems from your responses that you understood this to work differently? If so, maybe re-evaluate the options I provided?

[CyberShadow]

No, DMD does parse DFLAGS from the environment (it does so always), it just replaces that string with the content of dmd.conf.

Right, we're on the same page here. I meant the current behavior with the default configuration file.

Even DMD's testsuite depends on this behavior:

Is that with -conf= (i.e. no configuration file)?

An non-breaking option would be to introduce an -env flag for dmd which tells it to have higher priority for environment flags (or append them to dmd.conf).

Maybe we can introduce a new variable and add it to the default definition of DFLAGS?

Here's a different idea: make dmd -v list all environment variables that DMD read and used from the enviroment (including those that didn't exist but would have been used if they did exist). rdmd can then record them and their values, and force a rebuild if it finds that any of them changed or appeared/disappeared.

[wilzbach]

Here's a different idea: make dmd -v list all environment variables that DMD read and used from the enviroment (including those that didn't exist but would have been used if they did exist)

FYI: dmd does this since 2.079 - dlang/dmd#7865 (see also the logs above)

Is that with -conf= (i.e. no configuration file)?

Yep (though a lot of the bash scripts don't set it and everything "just works" because the debug dmd.conf is created by default.

Maybe we can introduce a new variable and add it to the default definition of DFLAGS?

Not sure whether this would create less confusion if we introduce DFLAGS2 or DFLAGS_APPEND or D_FLAGS` or did you had a better name?

[CyberShadow]

FYI: dmd does this since 2.079 - dlang/dmd#7865 (see also the logs above)

No, that's unusable for this case. rdmd can't reasonably simply check if the final DFLAGS will change since the previous invocation without actually invoking dmd. As I wrote above, it will need to list all environment variables that DMD read and used from the enviroment, including those that didn't exist but would have been used if they did exist. Their values aren't actually needed - rdmd just needs to know if they changed since the previous invocation, so it will need to read their values from the environment at least for the second invocation.