nimscript: make `exec("nim c")` honor `-d:nimExe:path`

[timotheecour]

this is a very simple proposal to fix https://github.com/nim-lang/nimble/issues/865 in a way that requires 0 code modification for nimscript/nimble code.

proposal

add magic in exec (in nimscript) as follows:

const nimExe {.strdefine.} = ""
proc exec(s: string) =
  when nimExe.len > 0:
    var s = s
    const pattern="nim "
    if s.startsWith pattern:
      s = nimExe & " " & s[pattern.len .. ^1]
  execOld(s)

then make nimble turn --nim:path into -d:nimExe:path

[alaviss]

Instead of introducing a hack like this, maybe just introduce a nimexec proc for that?

[Araq]

Fine with me if @genotrance thinks it's a good idea.

[genotrance]

I prefer simply enabling getAppFilename() for nimscript and expect users to use that. Or pointing to paramStr(0) which already provides the info they need.

[alaviss]

I find it weird that paramStr lets the script access the entirety of the command line passed to the compiler as you can't effectively parse those and you have to ignore them until you got past the script file name, which then the arguments will be for the script. But that's a different issue.

What I have against this "feature" is that it heavily skews the way exec works, which has always been to execute the said string via the shell. This sort of hidden replacement is typically considered an anti-feature and no scripting language ever do this, which is why I'd prefer a nimexec and/or a getCompiler() (which IIRC we already have one in macros?).

[timotheecour]

I prefer simply enabling getAppFilename() for nimscript

it's already there via getCurrentCompilerExe I introduced as getAppFilename in VM; one downside is it requires import os, which slows things down (refs: https://github.com/nim-lang/Nim/issues/14179#issuecomment-625200718); but the biggest downside is it requires nimble files to be updated individually.

Or pointing to paramStr(0) which already provides the info they need.

using exec(paramStr(0) & " c main") is obfuscating, noone will use that

This sort of hidden replacement is typically considered an anti-feature

it's not hidden though, it's only enabled via an explicit flag, eg -d:nimExe:path (in turn enabled by nimble --nim:path args...)

What none of you seem to pay attention to is the fact that exec("nim c ...") is pervasive, it's not like you're going to suddenly make people switch to import os; exec(getCurrentCompilerExe() & " c ...") (or nimexec) in all nimble packages / nimscript files out there. Until then nimble --nim:path test is and will remain broken. This proposal has benefit of requiring 0 change to existing nimble/nimscript files, and only only triggers when -d:nimExe:path is provided. I cannot imagine a non contrived scenario where it would cause problems.

precedent

even testament uses that trick, replacing nim c ... in testament spec to honor --nim:path. Practicality wins here.

[alaviss]

What none of you seem to pay attention to is the fact that exec("nim c ...") is pervasive, it's not like you're going to suddenly make people switch to import os; exec(getCurrentCompilerExe() & " c ...") in all nimble packages / nimscript files out there. Until then nimble --nim:path test is and will remain broken.

You seem to only pay attention to the compiler within the context of nimble. NimScript is starting to receive adoption in projects other than nimble and the compiler and is making its way as a general scripting language.

Currently NimScript is catered to be a build task language, but with increased adoption from external projects it might grow into a real general purpose scripting tool, and I don't think having more special cased behaviours would be helpful in that aspect.

even testament uses that trick, replacing nim c ... in testament spec to honor --nim:path. Practicality wins here.

testament was designed to test code that has to be compiled with the Nim compiler, it's normal that it does that. I might consider a different stance if you propose to make this nimble-specific (nimble already overrides many procs from stdlib's nimscript), but as of now this RFC wants the change to target NimScript itself, which as expressed above, I'm not a fan of.

[timotheecour]

You seem to only pay attention to the compiler within the context of nimble. [...]

yes, nimscript is useful beyond nimble and no, this feature is not specific to nimble: if you run patho/nim_temp e foo.nims, you may also want to have an easy (no-rewrite) way for your exec("nim c foo") to pickup that custom nim binary patho/nim_temp instead of nim in your PATH.

Once again, this feature will only enabled by an explicit flag, so I don't see practical downside. We could also potentially make it a proper flag eg --nim:path instead of -d:nimExe:path, to be understood more broadly.

[alaviss]

if you run patho/nim_temp e foo.nims, you may also want to have an easy (no-rewrite) way for your exec("nim c foo") to pickup that custom nim binary patho/nim_temp instead of nim in your PATH.

The scope is still the compiler and/or nimble. What about apps that integrates the compiler's VM? This feature have zero value there.

Once again, this feature will only enabled by an explicit flag, so I don't see practical downside.

I'm wary of introducing hacks to the standard library. It seems short-sighted and might go back and bite us later on, but that's just my opinion.

[Araq]

Well since this requires a Nimble fix anyway, we should investigate how Nimble can fix it on its own entirely.

[genotrance]

--nim was only added to nimble to enable testing for Nim compiler developers. Arguably, regular users will simply expect to use the nim binary associated with the nimble binary they are using and never use --nim.

No doubt, compiler developers might want --nim to be used everywhere for test purposes. nimscript and nim users could also want a shortcut though they aren't really affected, or even blocked since it is possible to get the current compiler path and use it appropriately.

Nimble simply calls and leverages nimscript, fixing this only within nimble would entail overriding exec and that will lead to different behavior between nimble and regular nimscript. But maybe because --nim is only relevant to nimble and to compiler devs, it's okay?

Maybe leaving it as is also might be good enough (which was my original thought) unless you compiler folks using nim_temp all the time really need this.

[Araq]

Ah well. Too early to tell. I am using --nim in production now but I don't know if I need the consistency with "exec".

[timotheecour]

• choosenim x.y.z also isn't a viable workaround (it has global effects)

• changing the PATH is not a good enough solution, it still requires a binary called nim, not nim_temp etc: I use the following pattern all the time: when I'm on branch foo I compile nim to bin/nim_foo so I can easily switch branch and always have the last built nim in that branch available.

• testament supports --nim:path (which I also use all the time) and so should nimscript and perhaps other tools. It's more robust than relying on PATH (no support for nim_temp or nim_$branch and can affect other programs) or choosenim (same problem and has global effect)

Perhaps we can add simply a --nim:path to nim's own cmdline, which would special case exec for use in all clients (nimscript, nimble, compiler-as-a-library), with --nim:@auto meaning to use getCurrentCompilerExe()

What about apps that integrates the compiler's VM? This feature have zero value there

also useful for this use case I believe (but I need to double check with an example)

I like solutions that require 0 code change on the (many) existing clients; only an (explicit, default off) command line switch --nim:path is needed.

[genotrance]

You're not going to run the nimscript with one version of Nim and the task with another so an additional --nim flag is not warranted. Just use the same nim that's currently running the script. Meanwhile, any changes made to exec() might also need to extend to gorgeEx(), staticExec() and friends.

Lastly, none of this will affect tests that call nim in runtime code - tests/tmytest.nim that calls nim will not be covered.

[disruptek]

I feel like you'd be in a position to iterate more quickly and perhaps get this code into the hands of your customers much faster and with less compromise to your vision if you just made a library in which to present these ideas, tests, documentation, and examples of use. Have you given this any consideration?

[dom96]

This is interesting, makes me wonder:

task foo, "foo":
  exec "nim c blah.nim"

Does nimble foo -d:release pass the -d:release to the exec? I just tested it and it doesn't. Judging by what this proposal wants we should be passing this through implicitly.

[timotheecour]

I feel like you'd be in a position to iterate more quickly and perhaps get this code into the hands of your customers much faster and with less compromise to your vision if you just made a library in which to present these ideas, tests, documentation, and examples of use. Have you given this any consideration?

a library wouldn't make sense, you can already do that via changing exec("nim c args...") to import os; exec(getCurrentCompilerExe() & " c args..."). The whole point is to enable this for all code calling exec without having to patch any code at all, via a compiler switch. Patching all code calling exec would be impractical, until then nimble --nim:path + friends would be broken

And yes, I'm happy to make a PR against nim.

You're not going to run the nimscript with one version of Nim and the task with another so an additional --nim flag is not warranted. Just use the same nim that's currently running the script

that's for consistency with other tools, eg testament --nim:path, nimble --nim:path. One way or another, you need a flag, it wouldn't be acceptable to modify exec by default; only when a flag is specified. If anything, --nim without argument could be introduced first (meaning getCurrentCompilerExe()), and then --nim:path could be supported later if needed. I do find --nim:@auto (or --nim:@self) more self documenting though.

Meanwhile, any changes made to exec() might also need to extend to gorgeEx(), staticExec() and friends.

I'm open to that.

Lastly, none of this will affect tests that call nim in runtime code - tests/tmytest.nim that calls nim will not be covered.

indeed (at very least, runtime is out of scope of this RFC). Since nimble + friends runs at compile time, the getCompilerExe() binary is guaranteed to exist, whereas this isn't the case for cgen'd code.

Does nimble foo -d:release pass the -d:release to the exec? I just tested it and it doesn't. Judging by what this proposal wants we should be passing this through implicitly.

that's out of scope of this RFC and would not be desirable in the way you're describing; it's perfectly ok for nimble files to write:

exec "nim c main"
exec "nim c -d:release main"

and what you're suggesting would prevent that; furthermore flags are useful as passed to nim running the nimscript file, and not meant to be passed to inside exec eg:

when defined foo: exec "nim c main1"
else: exec "nim c main2"

forwarding -d:foo to inside exec doesn't make sense.

What could make sense is another explicit flag --execArgs:"-d:release -d:foo" but that's out of scope for this RFC

[timotheecour]

=> see PR https://github.com/nim-lang/Nim/pull/15686 implementing this

[genotrance]

Does nimble foo -d:release pass the -d:release to the exec? I just tested it and it doesn't. Judging by what this proposal wants we should be passing this through implicitly.

Custom tasks are, well, custom - the user can do whatever they want in there and changing the behavior for the many hundreds of custom tasks out there is asking for trouble. We will surely override some user's expectations. What if a user already passes flags in their exec call - what should it do?

[timotheecour]

What if a user already passes flags in their exec call - what should it do?

as I said here https://github.com/nim-lang/RFCs/issues/273#issuecomment-714803307 it's out of scope for this RFC, but this is what I have in mind, using flag for both prepend and append is the most explicit, fool proof way:

--execArgsPre:"--gc:arc" --execArgsPost:"-u:release --stacktrace:on"

task foo, "do foo":
  exec "nim c -r -d:release main.nim args"

=> exec cmd translated to: {getAppFileName()} {--execArgsPre} c -r -d:release {--execArgsPost} main.nim in pseudocode, eg:

`{getAppFileName()} --gc:arc c -r -d:release -u:release --stacktrace:on main.nim args`

• the prepend flags can be overriden by the task command
• the append flags override all previous flags

This allows testing packages with different configurations with minimum editing and lots of flexibility. Sure, you'll find use cases where this can break, for those you'll have to edit actual files, but this should work in lots of cases. It's a bit analog to nimscript patchFile

[dom96]

The reason I bring it up is that I think it's inconsistent to say "we want the nim to be overriden" but not the flags.

[genotrance]

The reason I bring it up is that I think it's inconsistent to say "we want the nim to be overriden" but not the flags.

Using a different nim with --nim was all about enabling compiler developers so that they can verify that their custom build still works as expected, whereas passing arguments to custom tasks executing nim is about users testing their packages. I don't think the two go together.

@timotheecour - please evaluate how nimble passes arguments to a default nimble test and a custom nimble test task, and custom tasks. It was recently improved and should cover most cases. I agree it doesn't cover exec() but my logic for custom tasks is that users can use commandLineParams() and co to make their tasks smart enough.

[Araq]

Please don't change --nim, I already use it in production. And I don't want to live in Timothee's valhalla of complex switch overrides with complex stateful ordering requirements where --foo:off --foo:on means --foo:on and cannot ever hope to produce a warning about a schizophrenic setup. We have seen how well this really works with -d:release not triggering the actions of the default config when itself is used inside a config file.

[github-actions[bot]]

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 30 days.