Fix LTO issues on OSX by only passing -fuse-ld=lld on windows

[Keithcat1]

Fixes #15578
I can't be completely sure that it will work because I don't have aMack for testing.
This PR also renames -d:lto to -d:nim_lto and -d:strip to -d:strip_debug_info

[alaviss]

This PR also renames -d:lto to -d:nim_lto and -d:strip to -d:strip_debug_info

Please keep the name. With 1.4 shipped these names are now canonical and should not be touched.

[ghost]

@alaviss maybe we can add both? and somehow deprecate old ones

[ghost]

Also IMO it's better to rename strip to nimStrip instead of strip_debug_info

[alaviss]

@alaviss maybe we can add both? and somehow deprecate old ones

Build system flags should never be touched unless strictly necessary. Change a flag and someone build system breaks. I'd prefer that we leave the flags as is.

[timotheecour]

rename strip to nimStrip, lto to nimLto, and keep the old names for compatibility reasons with a deprecation notice (the deprecation could be extended until whenever deemed reasonable, including indefinitely)

[alaviss]

(the deprecation could be extended until whenever deemed reasonable, including indefinitely)

Then what's the point of deprecating the names? Now you have 2 names per switch, doesn't sound like a good idea to me. Now that these names are canonical, everyone will know to avoid them if it conflicts with their project.

Instead of bike shedding over an established name we could just layout some documentation to make sure newer names are scoped in the future.

[timotheecour]

Then what's the point of deprecating the names

minimizing the amount of arbitrary historical baggage. Deprecation messages are here for helping you transition code.

everyone will know to avoid them if it conflicts with their project.

Instead of bike shedding over an established name we could just layout some documentation to make sure newer names are scoped in the future.

documentation is already here: https://nim-lang.github.io/Nim/contributing.html#best-practices but having it in docs is never as good as enforcing it, see nim-lang/RFCs#269 for a proposal on how to enforce it.

[Keithcat1]

So what should I do then?

[alaviss]

minimizing the amount of arbitrary historical baggage. Deprecation messages are here for helping you transition code.

It will still be 4 flags for the same 2 functions and we can never remove two of them. At one point I think we should just introduce flags like these into real configuration flags instead of the hackish define that can only work if you pass them via the command line.

[timotheecour]

we can never remove two of them

same as for any feature, the deprecation cycle involves a (possibly indefinite, depending on use in projects) deprecation cycle followed by error.
Even if indefinite, we should promote the idiomatic way, uniformity makes things easier to learn.

I'll defer to @Araq to make final call on this but I've explained my reasoning here #15614 (comment) and add this:

• -d:lto was introduced recently (in april, for 1.4) and is buggy (cf this issue)
• -d:nimLto can be backported to 1.4.2

At one point I think we should just introduce flags like these into real configuration flags instead of the hackish define that can only work if you pass them via the command line.

That's a good point:

• switch("passc","-flto") works in my ~/.config/nim/config.nims
• -d:lto does not work in my ~/.config/nim/config.nims

I think that feature should be revisited to make it work like most all features, so that adding -d:lto in ~/.config/nim/config.nims would work.
Maybe that's one extra reason to introduce a config.nims-friendly flag -d:nimLto and -d:nimStrip and deprecate the other.

[Araq]

I agree with @timotheecour on this one, the name nimLto is better, or maybe even nimLinkTimeOptimizations, there is only so many abbreviations you can learn in your lifetime.

[Clyybber]

As a counterargument: We don't use nimDanger or nimRelease either, and lto is not something thats specific to nim.

[timotheecour]

how about:
nim --opts:<comma separate list> eg:
nim --opts:lto or nim --opts:strip or nim --opts:lto,strip

benefits:

• solves the problem (Fix LTO issues on OSX by only passing -fuse-ld=lld on windows #15614 (comment)) of -d:lto (and -d:strip) only working when passed on cmdline
• more discoverable since it's in --fullhelp which people use much more often than https://nim-lang.github.io/Nim/nimc.html (which btw doesn't even mention -d:strip nor -d:lto yet)
• avoids nim prefix

lto is not something thats specific to nim

the nim prefix indicates that nim package handles it

We don't use nimDanger or nimRelease either

I'd argue we could even support it in --opts, eg: --opts:danger,strip,lto
it would fix #14272 too (-d:release -d:danger don't work as expected in user configs)

meanwhile, -d:danger,-d:release could be deprecated yet indefinitely supported.

[alaviss]

nim --opts:lto or nim --opts:strip or nim --opts:lto,strip

I'm all for it as long as multiple --opts: adds up. A way to check if an opts is specified in configuration/nim code would be nice.

The release define should stay (and be defined by --opts:release) IMO, as it has semantics impact on a lot of code.

[timotheecour]

I'm all for it as long as multiple --opts: adds up.

yes, --opts:release --opts:lto,strip would work.
Optionally we can also add support for negation eg: --opts:strip,lto --opts:strip:off would resovle to --opts:lto (better than gcc's negation via -fomit-frame-pointer => -fno-omit-frame-pointer)

A way to check if an opts is specified in configuration/nim code would be nice

yes, via the existing std/compilesettings.nim

The release define should stay (and be defined by --opts:release) IMO, as it has semantics impact on a lot of code.

yes, -d:release would still be honored, in addition to --opts:release; whether -d:release triggers a warning or not, I don't care much (but note that --opts:release would not be subject to #14272 unlike -d:release)

And a final benefit is that a misspelt -d:foo doesn't trigger any kind of warning, whereas a misspelt --opts:foo would trigger an error

[ghost]

@timotheecour maybe you can create a separate RFC for that idea? I'm all for it, and Araq roughly wanted the same thing to fix -d:release and -d:danger IIRC.

[genotrance]

How will --opts:release fix #14272? If $nim/config/nim.cfg checks for it and does some stuff, it is not going to get reevaluated if $project.nim.cfg sets it.

[ghost]

@genotrance well, "opts" can't be an option in the nim config so it'll be defined in the compiler instead of the config

[timotheecour]

@timotheecour maybe you can create a separate RFC for that idea? I'm all for it, and Araq roughly wanted the same thing to fix -d:release and -d:danger IIRC.

will do today

[Araq]

Oh god no... Not yet another switch that of course must be scriptable and checkable with something like system.switchActive("blah") ...

[timotheecour]

system.switchActive("blah")

that functionality already exists via system.compileOption("blah") (or std/compilesettings.nim)

[genotrance]

@genotrance well, "opts" can't be an option in the nim config so it'll be defined in the compiler instead of the config

If --opts is a command line flag, nims/cfg should be able to set it. You could check it in nims but not cfg which means all the @if blocks setting all sorts of flags based on other flags need to be moved into Nim code. Moving into nims won't solve anything since it is no different than cfg as far as global/user/project nims load order is concerned.

Next, all the logic's moved into Nim, so now as a user, you cannot really affect change in that logic. For example:

nim c --opts:release file.nim + a nim.cfg with --stackTrace:on gives you no stack trace since --opts:release turns it off in Nim. Unless there's some logic to know whether a user explicitly set a flag and whether Nim should change it.

Additionally, it's just a mess - we already have --flag like --stackTrace:on|off, --define and an --opt:speed and now we are considering --opts.

Almost seems easier to solve this with multiple passes.

[timotheecour]

Unless there's some logic to know whether a user explicitly set a flag and whether Nim should change it.

there is, see modifiedyNotes

Additionally, it's just a mess - we already have --flag like --stackTrace:on|off, --define and an --opt:speed and now we are considering --opts.

we can reuse --opt (which acts as an enum) and extend its meaning, it would work too, eg: --opt:speed,strip,lto

Almost seems easier to solve this with multiple passes.

that's messier, you'd end up with a zig zag pattern

cmdline=>nim cfg=>user cfg=>cmdline # current
cmdline=>nim cfg=>user cfg=>cmdline=>nim cfg # with multiple passes

[genotrance]

Why do we want users to have to learn some new UI/syntax with this new --opt system? If we simply move all the nim.cfg logic in the global nim.cfg file into code and continue to use -d:release and so forth, it will work as expected, assuming the same could be accomplished with --opt + code.

Nim already has -u so in theory, user could turn things off in their project config file that overrides user or global defines.

[timotheecour]

can we use the same approach as in #18051 ? ie, the flag should have a side effect only once all the cmdline/configs have been processed (so that -d:nimLto works in your user config, and so that -d:nimLto followed by -u:nimLto (in an overriding config stage, eg in cmdline) would cancel out its effect, as with most other flags.

(and it should still be renamed, -d:lto is not good, but can be kept for a transition period (which could be infinity))

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.