Page MenuHomePhabricator

logtoprocess: add the possibility to not start a shell
AbandonedPublic

Authored by lothiraldan on Nov 15 2017, 10:44 AM.

Details

Reviewers
mbthomas
Group Reviewers
hg-reviewers
Summary

Starting a shell on Windows means starting a console GUI window in almost all
cases. If we add a logtoprocess for commandfinish and the prompt do some hg
calls, it will create a short-lived GUI window which is annoying for end-
users.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Nov 15 2017, 10:44 AM
mbthomas accepted this revision.Nov 15 2017, 12:18 PM
mbthomas added a subscriber: mbthomas.
mbthomas added inline comments.
hgext/logtoprocess.py
65

On unix systems shell=False means the command provided cannot take any arguments, as it's the shell that does the argument splitting. I am guessing that this is OK, as we'll just never configure logtoprocess.shell as False on unix systems. Is that right?

lothiraldan added inline comments.Nov 16 2017, 4:09 AM
hgext/logtoprocess.py
65

Right, I was not aware of this limitation.

If a company would deploy the same configuration for both Windows and Linux, we might have shell=False on Linux system but it would work well if the script doesn't take any argument or with an intermediary script that injects all the arguments, I will document it in the commit message and extensions documentation.

yuja added a subscriber: yuja.Nov 16 2017, 8:19 AM

Can't we use openflags=CREATE_NO_WINDOW instead?

https://stackoverflow.com/a/7006424

shell=False also disables envar expansion, and I guess there would
be lots of trivial behavior changes.

In D1426#23797, @yuja wrote:

Can't we use openflags=CREATE_NO_WINDOW instead?
https://stackoverflow.com/a/7006424

I tried lots of things including openflags=CREATE_NO_WINDOW that didn't seem to work, unfortunately.

shell=False also disables envar expansion, and I guess there would
be lots of trivial behavior changes.

I'm not sure to understand what you mean, I tried this configuration:

[logtoprocess]
commandfinish = %HOMEPATH%/script.pl

And %HOMEPATH seemed to be expanded.

yuja added a comment.Nov 16 2017, 9:27 AM
In D1426#23797, @yuja wrote:

Can't we use openflags=CREATE_NO_WINDOW instead?
https://stackoverflow.com/a/7006424

I tried lots of things including openflags=CREATE_NO_WINDOW that didn't seem to work, unfortunately.

Right. cmd.exe does allocates new console because the process is "DETACHED" from
the parent's console. I have no idea if DETACH_PROCESS is what we wanted.

shell=False also disables envar expansion, and I guess there would
be lots of trivial behavior changes.

I'm not sure to understand what you mean, I tried this configuration:

[logtoprocess]
commandfinish = %HOMEPATH%/script.pl

And %HOMEPATH seemed to be expanded.

Maybe it's resolved while looking up the executable path. I suspect
script.pl %HOMEPATH% wouldn't work.

shell=False also disables envar expansion, and I guess there would
be lots of trivial behavior changes.

I'm not sure to understand what you mean, I tried this configuration:

[logtoprocess]
commandfinish = %HOMEPATH%/script.pl

And %HOMEPATH seemed to be expanded.

Maybe it's resolved while looking up the executable path. I suspect
script.pl %HOMEPATH% wouldn't work.

With further testing, I can confirm that script.pl %HOMEPATH% is not working on Windows with shell=False. I don't know an equivalent of shlex.split for Windows, so I guess that documenting it would be enough as we don't force shell=False.

I will document it.

yuja added a comment.Nov 29 2017, 7:40 AM

Do you think DETACH_PROCESS should remain specified? I believe that's
the source of the problem, but I can't say it's unnecessary with my little Windows-fu.

What I know is our win32.spawndetached() sets only CREATE_NO_WINDOW.

Boris, any updates on this?

I've access to a Windows machine this week, so you can expect a comment or new version before the end of the week.

Without DETACHED_PROCESS, no console is spawned either with shell=False or shell=True but Mercurial will wait for the log-to-process script to finish before ending.

So I would say to keep DETACHED_PROCESS.

The real question is how to do variables expansion with shell=False, I don't think that Python has a stdlib function for doing that.

Without DETACHED_PROCESS, no console is spawned either with shell=False or shell=True but Mercurial will wait for the log-to-process script to finish before ending.

No idea why, but a simple python script running subprocess.Popen() exits
immediately on my utterly old Windows XP.

import subprocess
import sys
creationflags = subprocess.CREATE_NEW_PROCESS_GROUP
subprocess.Popen('%s -c "import time; time.sleep(10)"' % sys.executable,
                 shell=True, creationflags=creationflags)

So I would say to keep DETACHED_PROCESS.
The real question is how to do variables expansion with shell=False, I don't think that Python has a stdlib function for doing that.

Emulating cmd.exe behavior wouldn't be easy. If we had to set shell=False,
we'd probably better to document the Windows-specific issue.

@mharbison72, do you have any idea?

Sorry, I'm not sure what the context is here. Is this all to resolve %FOO% type variables? I don't know of any library to do this either.

Earlier this year I tried fixing the logtoprocess test, which was leaving out chunks of output on Windows. The only notes I left were that the output showed up if DETACHED_PROCESS was removed, but then it flickered a bunch of cmd.exe windows. IDK how that works, so eventually I gave up and required no-windows. That said, Yuya's code snippet seems to work for me on Win7 in a python shell.

I don't have time to test tonight, but maybe this will help? I vaguely remember having luck with SW_HIDE.

https://www.codeproject.com/Articles/2537/Running-console-applications-silently

It works when passing CREATE_NEW_CONSOLE instead of DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP.

My concern now is about changing the behavior, do we want to keep the improvement under a config knob.

I've resent as https://phab.mercurial-scm.org/D1700 because I couldn't get phabsend to reproduce my linear updated stack on phabricator :(

lothiraldan abandoned this revision.Jun 19 2019, 2:31 AM