Patchwork D1426: logtoprocess: add the possibility to not start a shell

login
register
mail settings
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

phabricator - Nov. 15, 2017, 3:44 p.m.
lothiraldan created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1426

AFFECTED FILES
  hgext/logtoprocess.py
  mercurial/configitems.py

CHANGE DETAILS




To: lothiraldan, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 15, 2017, 5:18 p.m.
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
phabricator - Nov. 16, 2017, 9:09 a.m.
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
phabricator - Nov. 16, 2017, 1:19 p.m.
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
phabricator - Nov. 16, 2017, 1:42 p.m.
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
phabricator - Nov. 16, 2017, 2:27 p.m.
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
phabricator - Nov. 28, 2017, 7:18 p.m.
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
phabricator - Nov. 29, 2017, 12:40 p.m.
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
phabricator - Dec. 11, 2017, 6:11 p.m.
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
phabricator - Dec. 12, 2017, 8:43 a.m.
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
phabricator - Dec. 13, 2017, 9:03 a.m.
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
phabricator - Dec. 13, 2017, 2:06 p.m.
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
phabricator - Dec. 14, 2017, 6:17 a.m.
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
phabricator - Dec. 14, 2017, 5:03 p.m.
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
phabricator - Dec. 15, 2017, 10:17 a.m.
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: