-d:release -d:danger don't work as expected in user configs

[ghost]

The problem is that currently the Nim compiler evaluates user configs last, so if a user puts -d:release or -d:danger in their
own config file, it won't have any effect on the actual compiler switches because the user config file will be evaluated last.

This can be quite unexpected for newbies since there are no warnings/hints about this behaviour at all. If you put these in your config file it'll compile just fine, defined(release) will return true, but the switches like --opt:speed won't get activated.

$ nim -v
Nim Compiler Version 1.3.1 [Linux: amd64]
Compiled at 2020-05-07
Copyright (c) 2006-2020 by Andreas Rumpf

git hash: 5fa7d374c4cb777372cf5b967575f228bda23c2b
active boot switches: -d:release

[Skrylar]

shell script to reproduce (run this in an empty directory)

set -x

if [ ! -f main.nim ]; then
    cat << DONK > main.nim
echo "yardanico made me do it"
DONK
fi

if [ -f nim.cfg ]; then
    mv nim.cfg nim.cfg.off
fi

if [ ! -f nim.cfg.off ]; then
    cat << DONK > nim.cfg.off
-d:danger
DONK
fi

nim c -o:test-inline -d:danger main.nim
mv nim.cfg.off nim.cfg
nim c -o:test-cfg main.nim
du -sh test-cfg test-inline

[ghost]

I discovered this when trying to minimise binary size:

echo "Hello world"

# nim.cfg
--os:any
--gc:arc
--panics:on
--define:posix
--define:release
--define:danger
--define:noSignalHandler
--cc:clang
--passC:"-flto"
--passL:"-flto"

# with no config
$ nim c --os:any --gc:arc --panics:on -d:posix -d:release -d:danger -d:noSignalHandler --cc:clang --passC:"-flto" --passL:"-flto" hello.nim 
$ wc hello
    3   113 18688 hello
# after adding config
$ nim c hello.nim
   79   863 69536 hello

[Clyybber]

This is caused by the fact that https://github.com/nim-lang/Nim/blob/devel/config/nim.cfg is now read before the project .cfg file.
This also causes -d:release or -d:danger not working at all in .cfg (aside from defining danger and release :p)

[ghost]

Even simpler way to reproduce:

# a.nim
proc a = 
  raise newException(ValueError, "test")
a()

Compile with nim c -r a.nim and you get the stack trace.
Now compile with nim c -d:release -r a.nim and you don't get the full stack trace as expected.
Now create a file nim.cfg and this inside:

-d:release

Now do nim c -r a.nim and the stack trace is still there which means that the -d:release is not really getting applied.

P.S: If you do it in config.nims, it still doesn't get applied.

[Clyybber]

Also this is not a regression :p

[ghost]

Seems to be documented in https://nim-lang.org/docs/nims.html only but it's just a note
Note: In general, the define switches can also be set in NimScripts using switch or --, as shown in above examples. Only the release define (-d:release) cannot be set in NimScripts.
And it doesn't say that this wouldn't work in nim.cfg (and I haven't found the documentation which says that for nim.cfg)

[ghost]

So I guess there are two approaches - make this actually work or document it in an obvious place and optionally add a warning so when Nim sees -d:danger or -d:release in user config files it shows a warning

[alaviss]

I see a few ways to implement this:

• Make --define special: The compiler should re-read all configurations that has been read before upon encountering a define or any flag that defines a symbol.

• Make release and danger special: Add something like options.release = "--stacktrace:off --opt:speed", then -d:release and/or -d:danger can just add those flags in before any other flags.

• Turn conditionals in nim.cfg into configuration blocks: Instead of "if this then parse", turn a block of:

  @if symbol
    flagA:on
  @endif
  

  into a configuration unit, ie: options.symbol = "flagA:on", then after processing all configurations, if symbol is defined, add the flags before any other flags. This is basically syntactic sugar for the previous proposal.

If we were to implement something like this, I'd personally prefer the first proposal, as it's easier to implement/understand IMO, though it might cause trouble with configurations that can have side effects (ie. nimscript's staticExec)

[Skrylar]

I would figure the config files were the same as if the lines were added to the command line, since they’re sort of patterned after @-files like this. Then defines should work in the order they were read, with an undefined added to the CLI so config files closer to the module being compiled have an opportunity to unset anything the workspace set above them.

[Araq]

Make release and danger special: Add something like options.release = "--stacktrace:off --opt:speed", then -d:release and/or -d:danger can just add those flags in before any other flags.

That's the solution I had in mind.

[ghost]

Yeah, I think that making release and danger special would make the most sense

[genotrance]

Making release and danger special is not sufficient. Anything in nim.cfg that has an @if block is affected. A big example is cross-compilation typically invoked by --os which can tweak how all OS specific settings cascade, mingw being a specific case, and arch specific stuff like arm64. Lastly lto and strip.

Also, the user could have a global user cfg with other defines and if blocks which won't be able to reevaluate on a per-project basis.

[timotheecour]

this would be fixed via #15614 (comment), see RFC nim-lang/RFCs#278

EDIT: see #15742

[ghost]

Fixed by #18051