WIP No more "Terminate Batch Job? (Y/N)" - Take 2

[mottosso]

Bugfix.

I found a better way to accomplish #626.

• Before, the Rez Python process launched a cmd.exe subshell followed by another cmd.exe subshell with the environment set up called rez_shell.bat
• Now, the Rez Python process launches a cmd.exe subshell, calling rez_shell.bat which then exits and puts the user in the original subshell.

The result is the same features and benefits, but one less layer to worry about. Most importantly, ctrl+c from a subshell doesn't exhibit the .bat prompt.

Before

$ rez env python-3 -- python -m http.server
Serving HTTP on 0.0.0.0 port 8000 (http://0.0.0.0:8000/) ...

Keyboard interrupt received, exiting.
Terminate batch job (Y/N)? y
Interrupted by user
Traceback (most recent call last):
...
AttributeError: 'module' object has no attribute 'killpg'

After

$ rez env python-3 -- python -m http.server
Serving HTTP on 0.0.0.0 port 8000 (http://0.0.0.0:8000/) ...

Keyboard interrupt received, exiting.
Interrupted by user

And, wouldn't you know it, this also solved #616, making cmd.exe a much better experience. Win!

[mottosso]

You're welcome!

Except FYI keep in mind this has significant effects on a Windows environment, including use via the API. It shifts the process hierarchy created when creating a context. I've done my best to evaluate this, but then I only have so many environment to test in. I would consider this BETA until more testing in real environments has happened.

[nerdvegas]

I noticed the alteration to the proc hierarchy, hence the minor version update (as opposed to patch) to at least nod to that fact this PR does more than may have been apparent. I'll add to the release notes to highlight that Windows users should take note. Thx A

…

On Wed, Jul 10, 2019 at 9:08 PM Marcus Ottosson ***@***.***> wrote: You're welcome! Except FYI keep in mind this has significant effects on a Windows environment, including use via the API. It shifts the process hierarchy created when creating a context. I've done my best to evaluate this, but then I only have so many environment to test in. I would consider this BETA until more testing in real environments has happened. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#627>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSQE6EJY7AZT7TVCJEDP6W7LNANCNFSM4HMLGVPA> .

[asztalosdani]

@mottosso Could you please elaborate a bit more?
We are using it on windows via the API, so we might be affected.

[mottosso]

The key difference is that arguments made via -- were previously incorporated into the rez-context.bat script, whereas they are now passed via the command-line, after rez-context.bat has finished. It can have an effect on character escaping.

E.g.

rez env mypackage -- my_command --with-an "argument"

Was previously turned into:

rem Start of context.bat

set KEY=valueA;%KEY%
set KEY=valueB;%KEY%
set KEY=valueC;%KEY%
...

rem Coming in from `--`
my_command --with-an "argument"

rem Finally, yielding control back to the user
cmd /K

Where that final cmd /K launched a new process which is where you typically end up after a rez env. With this PR, that last command no longer exist, as that is where the "Terminate Batch Job?" was coming from. The process you enter is the one from the original call to cmd. It's simpler.

The crux is with this -- mechanism. To incorporate that into this new process hierarchy, it is passed to the original process as a string, over the command-line, as opposed to written into the batch script. The batch script does a better job at escaping (from what I can tell) than the REPL mechanism.

Finally, because there is one level of processes less, if you had anything that depended on that, then you would need to update it. For example, if you got to the rez.exe executable by stepping up through the hierarchy (via e.g. psutil) then it would have one level less to traverse. Another example is Windows command-line buffer, of which there are 4 per default. There were 5 buffers in use prior to this PR (one per process in the hierarchy), and now there are 4, which is why history works. And again, if you depended on that (for whatever reason, I can't think of any off hand), then that would break too.

to at least nod to that fact this PR does more than may have been apparent.

It may very well cripple any and all use of Rez, as it's affecting the point at which contexts are generated and entered. Buyer beware. At least we'll get some good testing going now that it's released into the wild and can hopefully either confirm that it works or fix it more quickly.

[asztalosdani]

Cool, thanks for the heads up!