Transient Windows test failures

[FUJIWARA Katsunori]

At Tue, 20 Jun 2017 00:30:28 +0900,
Yuya Nishihara wrote:
> 
> On Sun, 18 Jun 2017 22:19:29 -0400, Augie Fackler wrote:
> > 
> > > On Jun 16, 2017, at 22:02, Matt Harbison <mharbison72 at gmail.com> wrote:
> > > 
> > > On Fri, 16 Jun 2017 09:59:30 -0400, Augie Fackler <raf at durin42.com> wrote:
> > > 
> > >> On Fri, Jun 16, 2017 at 12:18:18AM -0400, Matt Harbison wrote:
> > >>> So apparently, this is a symptom of not having %SystemRoot% in the
> > >>> environment when calling CreateProcess().
> > >>> 
> > >>> https://bugs.python.org/issue13524
> > >>> https://jpassing.com/2009/12/28/the-hidden-danger-of-forgetting-to-specify-systemroot-in-a-custom-environment-block/
> > >>> 
> > >>> I see that setup.py special cases this variable.  I did a search for 'env
> > >>> =', and it looks like hooks and pager start with empty environments, so they
> > >>> must not inherit this.  IDR if any recent changes were made that start with
> > >>> an empty environment.
> > >>> 
> > >>> The thing I can't get my mind around is the hit and miss nature of the
> > >>> error, if this is really the problem.
> > >> 
> > >> It sounds like it should be harmless to just always forward
> > >> %SystemRoot% - should we just do that?
> > > 
> > > Seems reasonable, but run-tests._getenv() already does an os.environ.copy(), so it should be there?
> > > 
> > > It does seem like a good idea to do it for hooks and other things executed, where the environment is built from scratch.  The question is where?  There's util.popen[2-4](), plus some direct calls to subprocess.Popen(), and an os.system().  I considered util.shellenviron(), but there are far fewer calls to this than places where processes are spawned.
> 
> (+CC foozy since he has Windows)
> 
> Is the problem only seen in tests? I don't think environment variables are
> cleared in hg side.
> 

I can't reproduce this issue on my Windows VM, yet :-<

I'm using 32bit Python 2.7.9 on 64bit Windows7, but is 64bit or more
recent Python (like as your environment) required to reproduce this ?

BTW, on hg side, there is no code path, which causes replacement or
removal of SystemRoot environment variable, AFAIK.

  - util.popen

    This involves os.popen(), and always inherits current environment
    variables.

  - util.popen2/3/4

    This involves subprocess.Popen() with 'env=env', but (1) default
    value of 'env' is None (= inherit current ones), and (2) all
    existing callers in sources below invoke without explicit env
    argument.

    - hgext/convert/*.py
    - mercurial/sshpeer.py

  - util.system

    This involves subprocess.Popen() with result of
    shellenviron(environ), which updates existing variables or adds
    new variables. And there is no replacement of SystemRoot on caller
    sides.

    Execution of external hook, editor, and so on uses this API via
    ui.system().

  - os.popen (for other than Mercurial APIs above)

    This always inherits current environment variables.

    (not used directly in mercurial/** and hgext/**)

  - os.system (for other than Mercurial APIs above)

    This always inherits current environment variables.

    - hgext/convert/gnuarch.py
    - hgext/factotum.py
    - mercurial/statprof.py
    - mercurial/util.py (for tempfilter)

  - subprocess.Popen (for other than Mercurial APIs above)

    - hgext/convert/common.py
    - hgext/fsmonitor/pywatchman/__init__.py
    - mercurial/util.py (for pipefilter)
      no explicit env

    - hgext/logtoprocess.py
    - mercurial/subrepo.py
      using copy of encoding.environ

    - mercurial/ui.py (for pager)
      using util.shellenviron() with rcutil.defaultpagerenv()

On run-tests.py side, subprocess.Popen() is sometimes used with
explicit env argument, but the result of Test._getenv() or None is
used in such case. This doesn't cause replacement or removal of
SystemRoot (at least, with current implementation).

Any of intermediate layers (MinGW or Python ?) causes issue ?

-- 
----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp