Submitter | phabricator |
---|---|
Date | Nov. 15, 2017, 3:44 p.m. |
Message ID | <differential-rev-PHID-DREV-a53npuv6e6grnk7vdzji-req@phab.mercurial-scm.org> |
Download | mbox | patch |
Permalink | /patch/25585/ |
State | Superseded |
Headers | show |
Comments
mbthomas accepted this revision. mbthomas added inline comments. INLINE COMMENTS > logtoprocess.py:65 > subprocess.Popen( > - script, shell=True, env=env, close_fds=True, > + script, shell=shell, env=env, close_fds=True, > creationflags=_creationflags) 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? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: mbthomas, mercurial-devel
lothiraldan added inline comments. INLINE COMMENTS > mbthomas wrote in 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? 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: mbthomas, mercurial-devel
yuja added a comment. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: yuja, mbthomas, mercurial-devel
lothiraldan added a comment. In https://phab.mercurial-scm.org/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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: yuja, mbthomas, mercurial-devel
yuja added a comment. In https://phab.mercurial-scm.org/D1426#23803, @lothiraldan wrote: > In https://phab.mercurial-scm.org/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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: yuja, mbthomas, mercurial-devel
lothiraldan added a comment. >>> `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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: yuja, mbthomas, mercurial-devel
yuja added a comment. 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`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: yuja, mbthomas, mercurial-devel
durin42 added a comment. Boris, any updates on this? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: durin42, yuja, mbthomas, mercurial-devel
lothiraldan added a comment. I've access to a Windows machine this week, so you can expect a comment or new version before the end of the week. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: durin42, yuja, mbthomas, mercurial-devel
lothiraldan added a comment. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: durin42, yuja, mbthomas, mercurial-devel
yuja added a subscriber: mharbison72. yuja added a comment. In https://phab.mercurial-scm.org/D1426#28695, @lothiraldan wrote: > 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? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: mharbison72, durin42, yuja, mbthomas, mercurial-devel
mharbison72 added a comment. 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 REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: mharbison72, durin42, yuja, mbthomas, mercurial-devel
lothiraldan added a comment. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: mharbison72, durin42, yuja, mbthomas, mercurial-devel
lothiraldan added a comment. I've resent as https://phab.mercurial-scm.org/D1700 because I couldn't get phabsend to reproduce my linear updated stack on phabricator :( REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1426 To: lothiraldan, #hg-reviewers, mbthomas Cc: mharbison72, durin42, yuja, mbthomas, mercurial-devel
Patch
diff --git a/mercurial/configitems.py b/mercurial/configitems.py --- a/mercurial/configitems.py +++ b/mercurial/configitems.py @@ -566,6 +566,9 @@ coreconfigitem('logtoprocess', 'develwarn', default=None, ) +coreconfigitem('logtoprocess', 'shell', + default=True, +) coreconfigitem('logtoprocess', 'uiblocked', default=None, ) diff --git a/hgext/logtoprocess.py b/hgext/logtoprocess.py --- a/hgext/logtoprocess.py +++ b/hgext/logtoprocess.py @@ -58,14 +58,14 @@ DETACHED_PROCESS = 0x00000008 _creationflags = DETACHED_PROCESS | subprocess.CREATE_NEW_PROCESS_GROUP - def runshellcommand(script, env): + def runshellcommand(script, env, shell=True): # we can't use close_fds *and* redirect stdin. I'm not sure that we # need to because the detached process has no console connection. subprocess.Popen( - script, shell=True, env=env, close_fds=True, + script, shell=shell, env=env, close_fds=True, creationflags=_creationflags) else: - def runshellcommand(script, env): + def runshellcommand(script, env, shell=True): # double-fork to completely detach from the parent process # based on http://code.activestate.com/recipes/278731 pid = os.fork() @@ -82,7 +82,7 @@ # connect stdin to devnull to make sure the subprocess can't # muck up that stream for mercurial. subprocess.Popen( - script, shell=True, stdin=open(os.devnull, 'r'), env=env, + script, shell=shell, stdin=open(os.devnull, 'r'), env=env, close_fds=True, **newsession) finally: # mission accomplished, this child needs to exit and not @@ -124,7 +124,8 @@ env = dict(itertools.chain(encoding.environ.items(), msgpairs, optpairs), EVENT=event, HGPID=str(os.getpid())) - runshellcommand(script, env) + shell = self.configbool('logtoprocess', 'shell') + runshellcommand(script, env, shell) return super(logtoprocessui, self).log(event, *msg, **opts) # Replace the class for this instance and all clones created from it: