This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1426

logtoprocess: add the possibility to not start a shell
AbandonedPublic

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

Download Raw Diff

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptNov 15 2017, 10:44 AM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

lothiraldan added a child revision: D1427: logtoprocess: add a test to show pager and ltp bad interaction.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.

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.

In D1426#23803, @lothiraldan wrote:

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.

lothiraldan added a child revision: D1534: logtoprocess: add an option to parse script command.Nov 28 2017, 3:36 PM

lothiraldan updated this revision to Diff 3914.

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.

In 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?

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

Revision Contents
Changeset List

			Path	Packages
M			hgext/logtoprocess.py (11 lines)
M			mercurial/configitems.py (3 lines)

Status	Author	Revision
Abandoned	lothiraldan	D1428 logtoprocess: connect all fds to /dev/null to avoid bad interaction with pager
Abandoned	lothiraldan	D1427 logtoprocess: add a test to show pager and ltp bad interaction
Abandoned	lothiraldan	D1534 logtoprocess: add an option to parse script command
Abandoned	lothiraldan	D1426 logtoprocess: add the possibility to not start a shell
Closed	lothiraldan	D1425 logtoprocess: clean-up old comment

Diff 3914

hgext/logtoprocess.py

	def uisetup(ui):			def uisetup(ui):
	if pycompat.iswindows:			if pycompat.iswindows:
	# no fork on Windows, but we can create a detached process			# no fork on Windows, but we can create a detached process
	# https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx			# https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx
	# No stdlib constant exists for this value			# No stdlib constant exists for this value
	DETACHED_PROCESS = 0x00000008			DETACHED_PROCESS = 0x00000008
	_creationflags = DETACHED_PROCESS \| subprocess.CREATE_NEW_PROCESS_GROUP			_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			# 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.			# need to because the detached process has no console connection.
	subprocess.Popen(			subprocess.Popen(
	script, shell=True, env=env, close_fds=True,			script, shell=shell, env=env, close_fds=True,
				mbthomasUnsubmitted Not Done 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? mbthomas: On unix systems `shell=False` means the command provided cannot take any arguments, as it's the…
				lothiraldanAuthorUnsubmitted Not Done 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. lothiraldan: Right, I was not aware of this limitation. If a company would deploy the same configuration…
	creationflags=_creationflags)			creationflags=_creationflags)
	else:			else:
	def runshellcommand(script, env):			def runshellcommand(script, env, shell=True):
	# double-fork to completely detach from the parent process			# double-fork to completely detach from the parent process
	# based on http://code.activestate.com/recipes/278731			# based on http://code.activestate.com/recipes/278731
	pid = os.fork()			pid = os.fork()
	if pid:			if pid:
	# parent			# parent
	return			return
	# subprocess.Popen() forks again, all we need to add is			# subprocess.Popen() forks again, all we need to add is
	# flag the new process as a new session.			# flag the new process as a new session.
	if sys.version_info < (3, 2):			if sys.version_info < (3, 2):
	newsession = {'preexec_fn': os.setsid}			newsession = {'preexec_fn': os.setsid}
	else:			else:
	newsession = {'start_new_session': True}			newsession = {'start_new_session': True}
	try:			try:
	# connect stdin to devnull to make sure the subprocess can't			# connect stdin to devnull to make sure the subprocess can't
	# muck up that stream for mercurial.			# muck up that stream for mercurial.
	subprocess.Popen(			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)			close_fds=True, **newsession)
	finally:			finally:
	# mission accomplished, this child needs to exit and not			# mission accomplished, this child needs to exit and not
	# continue the hg process here.			# continue the hg process here.
	os._exit(0)			os._exit(0)

	class logtoprocessui(ui.__class__):			class logtoprocessui(ui.__class__):
	def log(self, event, msg, *opts):			def log(self, event, msg, *opts):
	for i, m in enumerate(messages, 1))			for i, m in enumerate(messages, 1))
	# keyword arguments get prefixed with OPT_ and uppercased			# keyword arguments get prefixed with OPT_ and uppercased
	optpairs = (			optpairs = (
	('OPT_{0}'.format(key.upper()), str(value))			('OPT_{0}'.format(key.upper()), str(value))
	for key, value in opts.iteritems())			for key, value in opts.iteritems())
	env = dict(itertools.chain(encoding.environ.items(),			env = dict(itertools.chain(encoding.environ.items(),
	msgpairs, optpairs),			msgpairs, optpairs),
	EVENT=event, HGPID=str(os.getpid()))			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)			return super(logtoprocessui, self).log(event, msg, *opts)

	# Replace the class for this instance and all clones created from it:			# Replace the class for this instance and all clones created from it:
	ui.__class__ = logtoprocessui			ui.__class__ = logtoprocessui

mercurial/configitems.py

	default=None,			default=None,
	)			)
	coreconfigitem('logtoprocess', 'command',			coreconfigitem('logtoprocess', 'command',
	default=None,			default=None,
	)			)
	coreconfigitem('logtoprocess', 'develwarn',			coreconfigitem('logtoprocess', 'develwarn',
	default=None,			default=None,
	)			)
				coreconfigitem('logtoprocess', 'shell',
				default=True,
				)
	coreconfigitem('logtoprocess', 'uiblocked',			coreconfigitem('logtoprocess', 'uiblocked',
	default=None,			default=None,
	)			)
	coreconfigitem('merge', 'checkunknown',			coreconfigitem('merge', 'checkunknown',
	default='abort',			default='abort',
	)			)
	coreconfigitem('merge', 'checkignored',			coreconfigitem('merge', 'checkignored',
	default='abort',			default='abort',

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	3528		Nov 15 2017, 10:44 AM	★	★
Diff 2	3914		Nov 28 2017, 3:36 PM	★	★