Ability to pass shell=True in session.run so that the env argument is actually taken into account.

[smarie]

Currently it seems that environment variables are not expanded by the underlying popen when you call session.run:

session.run(['echo', '%PATH%'])

displays %PATH% instead of the contents of this environment variable
The reason seems to be that the underlying nox.popen.popen does not use shell=True (as also recommended in #379 (comment))

This is maybe a windows-only issue ?

Anyway, I see two main ways to tackle this, all easy:

• have popen receive arbitrary **kwargs so that we can pass shell=True (and in this case, some documentation section should probably be added in the place where we talk about env)
• have shell be a first-class parameter in session.run, with default value shell=True (disruptive) or False (conservative)

What do you think ?

[smarie]

Oh I realize that this shell=True parameter is only to expand variables used within the popen arguments. The env variables seem to be correctly passed to the process, even if shell=False. So the issue is probably far less blocking, and maybe not even worth fixing since users usually provide arguments that are already env variables-free. But for debug purposes, I still believe that being able to run session.run(['echo', '%PATH%']) or session.run(['echo', '%A%'], env={'A': "hello"}) and getting the right answer would be better so that users do not get lost (as I got)

[cjolowicz]

Expecting shell syntax to work is an easy mistake to make, especially when coming to Nox from Makefiles. So thanks for pointing that out!

Unfortunately, passing commands through the shell opens a can of worms, with shell metacharacters, portability issues, and another process to take care of. It would also break existing Noxfiles that are written for the current behavior. So I think that it would be better not to enable this behavior by default.

Using a list with shell=True also does not work. On POSIX, you'd end up with something like /bin/sh -c "echo" "$PATH", which will silently discard all but the first argument. So we'd need to specify the command as a single string, which brings us back to #329.

Personally, I'm not convinced that supporting an explicit option for session.run to run a shell brings many benefits, although I'm happy to learn more about possible use cases for this. It seems to me that there simply isn't much use to shelling out when you already have Python at your disposal. Your example above could be solved in Python without spawning an extra process (print(os.environ["PATH"])). If you truly need to run a shell, it may be better to be explicit about the specific shell you want to use. The command echo %PATH% is only valid in cmd.exe, so maybe session.run(["cmd.exe", "/c", "echo %PATH%"]) is more appropriate?

Do you think that the documentation should be clearer in this respect?

[theacodes]

Yeah, I'm -1 on adding shell=True as either default or explicit behavior. If someone really needs this, they can use subprocess directly.

[smarie]

Does a direct subprocess run with the correct session's bin paths ? If not, then providing a session.popen(....., shell, env) could be a viable alternative for such advanced usage.

I did not mention it in my original post, but I did this because I was suspecting that my env was not properly passed to session.run. In order to eliminate this possibility in my mind, I came up with this simple experiment where I would first run echo %PATH% then echo %A% (with env={'A': 'HELLO'}) and finally come back to my actual non-working command.

So my point is: even if it is not useful for nominal use, it might be the intuitive action a developper would try in a debugging experiment. Maybe this can be solved through a specific documentation section about environment variables vs. shell expansion, I dont know.

[cjolowicz]

Does a direct subprocess run with the correct session's bin paths ?

No, we don't modify the environment of Nox itself. But you could set PATH for the subprocess using session.bin_paths.

Edit: Or rather, pass the bin paths to shutil.which, that'll also work correctly on Windows.

If not, then providing a session.popen(....., shell, env) could be a viable alternative for such advanced usage.

If it's for advanced usage in a debugging session, invoking the shell explicitly via session.run seems sufficient to me. I would also consider using python -c "..." instead of running a shell.

So my point is: even if it is not useful for nominal use, it might be the intuitive action a developper would try in a debugging experiment. Maybe this can be solved through a specific documentation section about environment variables vs. shell expansion, I dont know.

The documentation of session.run has examples for both bash and cmd.exe, and one of these examples involves an environment variable. If you see a way to make this documentation clearer and help people avoid this mistake, it would definitely be appreciated.

[smarie]

The documentation of session.run has examples for both bash and cmd.exe, and one of these examples involves an environment variable.

Indeed this documentation seems sufficient for me. You can close the ticket if no other idea/feedback pops up

[cjolowicz]

Alright, I'm closing this for now. Thanks for filing the issue @smarie :)