Patchwork [STABLE] build: include a dummy $PATH in the custom environment used by build.py

login
register
mail settings
Submitter Gábor Stefanik
Date Oct. 28, 2016, 3:55 p.m.
Message ID <3cda0b069802af8b4dbd.1477670109@GSTEFANIK.NavnGo.local>
Download mbox | patch
Permalink /patch/17211/
State Changes Requested
Headers show

Comments

Gábor Stefanik - Oct. 28, 2016, 3:55 p.m.
# HG changeset patch
# User Gábor Stefanik <gabor.stefanik@nng.com>
# Date 1477669468 -7200
#      Fri Oct 28 17:44:28 2016 +0200
# Branch stable
# Node ID 3cda0b069802af8b4dbdf9f5598965a522a566b3
# Parent  3afde791dce192f38d8a228ed8e49397e353837e
build: include a dummy $PATH in the custom environment used by build.py

This is required for building with pypiwin32, the pip-installable replacement
for pywin32.
Gábor Stefanik - Oct. 28, 2016, 4:56 p.m.
>



--------------------------------------------------------------------------
This message, including its attachments, is confidential. For more information please read NNG's email policy here:
http://www.nng.com/emailpolicy/
By responding to this email you accept the email policy.


-----Original Message-----
> From: Mercurial-devel [mailto:mercurial-devel-bounces@mercurial-scm.org]

> On Behalf Of Gábor Stefanik

> Sent: Friday, October 28, 2016 5:55 PM

> To: mercurial-devel@mercurial-scm.org

> Subject: [PATCH STABLE] build: include a dummy $PATH in the custom

> environment used by build.py

>

> # HG changeset patch

> # User Gábor Stefanik <gabor.stefanik@nng.com> # Date 1477669468 -7200

> #      Fri Oct 28 17:44:28 2016 +0200

> # Branch stable

> # Node ID 3cda0b069802af8b4dbdf9f5598965a522a566b3

> # Parent  3afde791dce192f38d8a228ed8e49397e353837e

> build: include a dummy $PATH in the custom environment used by build.py

>

> This is required for building with pypiwin32, the pip-installable replacement

> for pywin32.


Just to clarify, without this, merely installing pypiwin32 completely breaks setup.py,
even for targets that don't try to use pywin32 for anything.

>

> diff -r 3afde791dce1 -r 3cda0b069802 setup.py

> --- a/setup.pyThu Oct 27 20:06:33 2016 +0200

> +++ b/setup.pyFri Oct 28 17:44:28 2016 +0200

> @@ -167,7 +167,8 @@

>  # to not use any hgrc files and do no localization.

>  env = {'HGMODULEPOLICY': 'py',

>         'HGRCPATH': '',

> -       'LANGUAGE': 'C'}

> +       'LANGUAGE': 'C',

> +       'PATH': '.'}

>  if 'LD_LIBRARY_PATH' in os.environ:

>      env['LD_LIBRARY_PATH'] = os.environ['LD_LIBRARY_PATH']  if

> 'SystemRoot' in os.environ:

> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 28, 2016, 5:03 p.m.
On 10/28/2016 05:55 PM, Gábor Stefanik wrote:
> # HG changeset patch
> # User Gábor Stefanik <gabor.stefanik@nng.com>
> # Date 1477669468 -7200
> #      Fri Oct 28 17:44:28 2016 +0200
> # Branch stable
> # Node ID 3cda0b069802af8b4dbdf9f5598965a522a566b3
> # Parent  3afde791dce192f38d8a228ed8e49397e353837e
> build: include a dummy $PATH in the custom environment used by build.py
>
> This is required for building with pypiwin32, the pip-installable replacement
> for pywin32.

What does the '.' value means here?

>
> diff -r 3afde791dce1 -r 3cda0b069802 setup.py
> --- a/setup.py	Thu Oct 27 20:06:33 2016 +0200
> +++ b/setup.py	Fri Oct 28 17:44:28 2016 +0200
> @@ -167,7 +167,8 @@
>  # to not use any hgrc files and do no localization.
>  env = {'HGMODULEPOLICY': 'py',
>         'HGRCPATH': '',
> -       'LANGUAGE': 'C'}
> +       'LANGUAGE': 'C',
> +       'PATH': '.'}
>  if 'LD_LIBRARY_PATH' in os.environ:
>      env['LD_LIBRARY_PATH'] = os.environ['LD_LIBRARY_PATH']
>  if 'SystemRoot' in os.environ:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Gábor Stefanik - Oct. 28, 2016, 5:13 p.m.
>



--------------------------------------------------------------------------
This message, including its attachments, is confidential. For more information please read NNG's email policy here:
http://www.nng.com/emailpolicy/
By responding to this email you accept the email policy.


-----Original Message-----
> From: Pierre-Yves David [mailto:pierre-yves.david@ens-lyon.org]

> Sent: Friday, October 28, 2016 7:03 PM

> To: Gábor STEFANIK <Gabor.STEFANIK@nng.com>; mercurial-

> devel@mercurial-scm.org

> Subject: Re: [PATCH STABLE] build: include a dummy $PATH in the custom

> environment used by build.py

>

>

>

> On 10/28/2016 05:55 PM, Gábor Stefanik wrote:

> > # HG changeset patch

> > # User Gábor Stefanik <gabor.stefanik@nng.com> # Date 1477669468 -7200

> > #      Fri Oct 28 17:44:28 2016 +0200

> > # Branch stable

> > # Node ID 3cda0b069802af8b4dbdf9f5598965a522a566b3

> > # Parent  3afde791dce192f38d8a228ed8e49397e353837e

> > build: include a dummy $PATH in the custom environment used by

> > build.py

> >

> > This is required for building with pypiwin32, the pip-installable

> > replacement for pywin32.

>

> What does the '.' value means here?


It is supposed to mean $(pwd). Maybe setting to an empty string would be safer?

>

> >

> > diff -r 3afde791dce1 -r 3cda0b069802 setup.py

> > --- a/setup.pyThu Oct 27 20:06:33 2016 +0200

> > +++ b/setup.pyFri Oct 28 17:44:28 2016 +0200

> > @@ -167,7 +167,8 @@

> >  # to not use any hgrc files and do no localization.

> >  env = {'HGMODULEPOLICY': 'py',

> >         'HGRCPATH': '',

> > -       'LANGUAGE': 'C'}

> > +       'LANGUAGE': 'C',

> > +       'PATH': '.'}

> >  if 'LD_LIBRARY_PATH' in os.environ:

> >      env['LD_LIBRARY_PATH'] = os.environ['LD_LIBRARY_PATH']  if

> > 'SystemRoot' in os.environ:

> > _______________________________________________

> > Mercurial-devel mailing list

> > Mercurial-devel@mercurial-scm.org

> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

> >

>

> --

> Pierre-Yves David
Gábor Stefanik - Oct. 28, 2016, 5:21 p.m.
>



--------------------------------------------------------------------------
This message, including its attachments, is confidential. For more information please read NNG's email policy here:
http://www.nng.com/emailpolicy/
By responding to this email you accept the email policy.


-----Original Message-----
> From: Mercurial-devel [mailto:mercurial-devel-bounces@mercurial-scm.org]

> On Behalf Of Gábor STEFANIK

> Sent: Friday, October 28, 2016 7:13 PM

> To: Pierre-Yves David <pierre-yves.david@ens-lyon.org>; mercurial-

> devel@mercurial-scm.org

> Subject: RE: [PATCH STABLE] build: include a dummy $PATH in the custom

> environment used by build.py

>

> >

>

>

> --------------------------------------------------------------------------

> This message, including its attachments, is confidential. For more information

> please read NNG's email policy here:

> http://www.nng.com/emailpolicy/

> By responding to this email you accept the email policy.

>

>

> -----Original Message-----

> > From: Pierre-Yves David [mailto:pierre-yves.david@ens-lyon.org]

> > Sent: Friday, October 28, 2016 7:03 PM

> > To: Gábor STEFANIK <Gabor.STEFANIK@nng.com>; mercurial-

> > devel@mercurial-scm.org

> > Subject: Re: [PATCH STABLE] build: include a dummy $PATH in the custom

> > environment used by build.py

> >

> >

> >

> > On 10/28/2016 05:55 PM, Gábor Stefanik wrote:

> > > # HG changeset patch

> > > # User Gábor Stefanik <gabor.stefanik@nng.com> # Date 1477669468 -

> 7200

> > > #      Fri Oct 28 17:44:28 2016 +0200

> > > # Branch stable

> > > # Node ID 3cda0b069802af8b4dbdf9f5598965a522a566b3

> > > # Parent  3afde791dce192f38d8a228ed8e49397e353837e

> > > build: include a dummy $PATH in the custom environment used by

> > > build.py

> > >

> > > This is required for building with pypiwin32, the pip-installable

> > > replacement for pywin32.

> >

> > What does the '.' value means here?

>

> It is supposed to mean $(pwd). Maybe setting to an empty string would be

> safer?


The problem is that pypiwin32 does os.environ['PATH'] += ... in a .pth file (loaded by site.py),
which fails if 'PATH' is not in os.environ.

The original pywin32 doesn't have this issue, but it can only be obtained by downloading
a Windows .exe installer from SourceForge, a site known to have previously injected
malware into .exe installers it hosts.

>

> >

> > >

> > > diff -r 3afde791dce1 -r 3cda0b069802 setup.py

> > > --- a/setup.pyThu Oct 27 20:06:33 2016 +0200

> > > +++ b/setup.pyFri Oct 28 17:44:28 2016 +0200

> > > @@ -167,7 +167,8 @@

> > >  # to not use any hgrc files and do no localization.

> > >  env = {'HGMODULEPOLICY': 'py',

> > >         'HGRCPATH': '',

> > > -       'LANGUAGE': 'C'}

> > > +       'LANGUAGE': 'C',

> > > +       'PATH': '.'}

> > >  if 'LD_LIBRARY_PATH' in os.environ:

> > >      env['LD_LIBRARY_PATH'] = os.environ['LD_LIBRARY_PATH']  if

> > > 'SystemRoot' in os.environ:

> > > _______________________________________________

> > > Mercurial-devel mailing list

> > > Mercurial-devel@mercurial-scm.org

> > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

> > >

> >

> > --

> > Pierre-Yves David

> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 29, 2016, 12:18 a.m.
On 10/28/2016 07:21 PM, Gábor STEFANIK wrote:
> -----Original Message-----
>> From: Mercurial-devel [mailto:mercurial-devel-bounces@mercurial-scm.org]
>> On Behalf Of Gábor STEFANIK
>> Sent: Friday, October 28, 2016 7:13 PM
>> To: Pierre-Yves David <pierre-yves.david@ens-lyon.org>; mercurial-
>> devel@mercurial-scm.org
>> Subject: RE: [PATCH STABLE] build: include a dummy $PATH in the custom
>> environment used by build.py
>>
>>>
>>
>>
>> --------------------------------------------------------------------------
>> This message, including its attachments, is confidential. For more information
>> please read NNG's email policy here:
>> http://www.nng.com/emailpolicy/
>> By responding to this email you accept the email policy.
>>
>>
>> -----Original Message-----
>>> From: Pierre-Yves David [mailto:pierre-yves.david@ens-lyon.org]
>>> Sent: Friday, October 28, 2016 7:03 PM
>>> To: Gábor STEFANIK <Gabor.STEFANIK@nng.com>; mercurial-
>>> devel@mercurial-scm.org
>>> Subject: Re: [PATCH STABLE] build: include a dummy $PATH in the custom
>>> environment used by build.py
>>>
>>>
>>>
>>> On 10/28/2016 05:55 PM, Gábor Stefanik wrote:
>>>> # HG changeset patch
>>>> # User Gábor Stefanik <gabor.stefanik@nng.com> # Date 1477669468 -
>> 7200
>>>> #      Fri Oct 28 17:44:28 2016 +0200
>>>> # Branch stable
>>>> # Node ID 3cda0b069802af8b4dbdf9f5598965a522a566b3
>>>> # Parent  3afde791dce192f38d8a228ed8e49397e353837e
>>>> build: include a dummy $PATH in the custom environment used by
>>>> build.py
>>>>
>>>> This is required for building with pypiwin32, the pip-installable
>>>> replacement for pywin32.
>>>
>>> What does the '.' value means here?
>>
>> It is supposed to mean $(pwd). Maybe setting to an empty string would be
>> safer?

My question is more "What is the meaning and effect of adding "." here? 
I'm fine with trying to fix pypiwin32 but we have to be aware of the 
actual consequence. A related quest is "What is happening is 'PATH' is 
already in the envirement? are we overwriting it?

> The problem is that pypiwin32 does os.environ['PATH'] += ... in a .pth file (loaded by site.py),
> which fails if 'PATH' is not in os.environ.

/me shake fist at .pth…

Can you point us at the full line to help us to find an appropriate 
solution ?

> The original pywin32 doesn't have this issue, but it can only be obtained by downloading
> a Windows .exe installer from SourceForge, a site known to have previously injected
> malware into .exe installers it hosts.

This seems sad? Are you sure there is no other download link? Maybe you 
could ping the developer.

Cheers
Jun Wu - Nov. 1, 2016, 3:39 p.m.
Excerpts from Pierre-Yves David's message of 2016-10-29 02:18:56 +0200:
> My question is more "What is the meaning and effect of adding "." here? 
> I'm fine with trying to fix pypiwin32 but we have to be aware of the 
> actual consequence. A related quest is "What is happening is 'PATH' is 
> already in the envirement? are we overwriting it?

For Windows, PATH='.' is equivalent to PATH='' since Windows searches for
PWD by design.

For *nix, PATH='' is better. PATH='.' has undesired side-effects.

I checked the code, if PATH is already set, it will be dropped by our code
because we created a new "env" dict and pass it to "runhg" -> "runcmd" ->
"subprocess.Popen(..., env=env)" so the new process does not inherit the old
PATH.

We rely on "sys.executable" to be correct to find the correct Python.

I think having an empty value, or inherit it from os.environ are both
acceptable solutions. But it's pypiwin32 to blame anyway.

> > The problem is that pypiwin32 does os.environ['PATH'] += ... in a .pth file (loaded by site.py),
> > which fails if 'PATH' is not in os.environ.
> 
> /me shake fist at .pth…
> 
> Can you point us at the full line to help us to find an appropriate 
> solution ?
> 
> > The original pywin32 doesn't have this issue, but it can only be obtained by downloading
> > a Windows .exe installer from SourceForge, a site known to have previously injected
> > malware into .exe installers it hosts.
> 
> This seems sad? Are you sure there is no other download link? Maybe you 
> could ping the developer.
> 
> Cheers
>
Pierre-Yves David - Nov. 1, 2016, 3:56 p.m.
On 11/01/2016 04:39 PM, Jun Wu wrote:
> Excerpts from Pierre-Yves David's message of 2016-10-29 02:18:56 +0200:
>> My question is more "What is the meaning and effect of adding "." here?
>> I'm fine with trying to fix pypiwin32 but we have to be aware of the
>> actual consequence. A related quest is "What is happening is 'PATH' is
>> already in the envirement? are we overwriting it?
>
> For Windows, PATH='.' is equivalent to PATH='' since Windows searches for
> PWD by design.
>
> For *nix, PATH='' is better. PATH='.' has undesired side-effects.
>
> I checked the code, if PATH is already set, it will be dropped by our code
> because we created a new "env" dict and pass it to "runhg" -> "runcmd" ->
> "subprocess.Popen(..., env=env)" so the new process does not inherit the old
> PATH.
>
> We rely on "sys.executable" to be correct to find the correct Python.
>
> I think having an empty value, or inherit it from os.environ are both
> acceptable solutions. But it's pypiwin32 to blame anyway.

It seem slike we should at least inherit a value if one exists. This 
patch does not seems ready for inclusion. I'm dropping it from patchwork 
and we'll revisit post release.
Gábor Stefanik - Nov. 2, 2016, 1:40 p.m.
>



--------------------------------------------------------------------------
This message, including its attachments, is confidential. For more information please read NNG's email policy here:
http://www.nng.com/emailpolicy/
By responding to this email you accept the email policy.


-----Original Message-----
> From: Mercurial-devel [mailto:mercurial-devel-bounces@mercurial-scm.org]

> On Behalf Of Pierre-Yves David

> Sent: Tuesday, November 1, 2016 4:56 PM

> To: Jun Wu <quark@fb.com>

> Cc: mercurial-devel@mercurial-scm.org

> Subject: Re: [PATCH STABLE] build: include a dummy $PATH in the custom

> environment used by build.py

>

>

>

> On 11/01/2016 04:39 PM, Jun Wu wrote:

> > Excerpts from Pierre-Yves David's message of 2016-10-29 02:18:56 +0200:

> >> My question is more "What is the meaning and effect of adding "." here?

> >> I'm fine with trying to fix pypiwin32 but we have to be aware of the

> >> actual consequence. A related quest is "What is happening is 'PATH'

> >> is already in the envirement? are we overwriting it?

> >

> > For Windows, PATH='.' is equivalent to PATH='' since Windows searches

> > for PWD by design.

> >

> > For *nix, PATH='' is better. PATH='.' has undesired side-effects.

> >

> > I checked the code, if PATH is already set, it will be dropped by our

> > code because we created a new "env" dict and pass it to "runhg" ->

> > "runcmd" -> "subprocess.Popen(..., env=env)" so the new process does

> > not inherit the old PATH.

> >

> > We rely on "sys.executable" to be correct to find the correct Python.

> >

> > I think having an empty value, or inherit it from os.environ are both

> > acceptable solutions. But it's pypiwin32 to blame anyway.

>

> It seem slike we should at least inherit a value if one exists. This patch does

> not seems ready for inclusion. I'm dropping it from patchwork and we'll

> revisit post release.


I believe the point of using a custom environment here is precisely to avoid passing the real $PATH.
Inheriting from the system environment would defeat this.

The only reason for including a PATH entry here is so site.py can do os.environ['PATH'] instead of os.environ.get('PATH').
Unfortunately some python modules (like pypiwin32) assume that a $PATH environment variable must always exist.

>

> --

> Pierre-Yves David

> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Nov. 2, 2016, 2:17 p.m.
On 11/02/2016 02:40 PM, Gábor STEFANIK wrote:
>>
>
>
> --------------------------------------------------------------------------
> This message, including its attachments, is confidential. For more information please read NNG's email policy here:
> http://www.nng.com/emailpolicy/
> By responding to this email you accept the email policy.
>
>
> -----Original Message-----
>> From: Mercurial-devel [mailto:mercurial-devel-bounces@mercurial-scm.org]
>> On Behalf Of Pierre-Yves David
>> Sent: Tuesday, November 1, 2016 4:56 PM
>> To: Jun Wu <quark@fb.com>
>> Cc: mercurial-devel@mercurial-scm.org
>> Subject: Re: [PATCH STABLE] build: include a dummy $PATH in the custom
>> environment used by build.py
>>
>>
>>
>> On 11/01/2016 04:39 PM, Jun Wu wrote:
>>> Excerpts from Pierre-Yves David's message of 2016-10-29 02:18:56 +0200:
>>>> My question is more "What is the meaning and effect of adding "." here?
>>>> I'm fine with trying to fix pypiwin32 but we have to be aware of the
>>>> actual consequence. A related quest is "What is happening is 'PATH'
>>>> is already in the envirement? are we overwriting it?
>>>
>>> For Windows, PATH='.' is equivalent to PATH='' since Windows searches
>>> for PWD by design.
>>>
>>> For *nix, PATH='' is better. PATH='.' has undesired side-effects.
>>>
>>> I checked the code, if PATH is already set, it will be dropped by our
>>> code because we created a new "env" dict and pass it to "runhg" ->
>>> "runcmd" -> "subprocess.Popen(..., env=env)" so the new process does
>>> not inherit the old PATH.
>>>
>>> We rely on "sys.executable" to be correct to find the correct Python.
>>>
>>> I think having an empty value, or inherit it from os.environ are both
>>> acceptable solutions. But it's pypiwin32 to blame anyway.
>>
>> It seem slike we should at least inherit a value if one exists. This patch does
>> not seems ready for inclusion. I'm dropping it from patchwork and we'll
>> revisit post release.
>
> I believe the point of using a custom environment here is precisely to avoid passing the real $PATH.
> Inheriting from the system environment would defeat this.
>
> The only reason for including a PATH entry here is so site.py can do os.environ['PATH'] instead of os.environ.get('PATH').
> Unfortunately some python modules (like pypiwin32) assume that a $PATH environment variable must always exist.

Ha true, when set, 'env' does not inherit any variable. So setting the 
path to '' should be fine. That said, the comment this env dictionary is 
quite specific about its goal.

# Execute hg out of this directory with a custom environment which
# takes care to not use any hgrc files and do no localization.

We should add a note about the why windows required PATH in some case.

I think we are clear enough now. (Jun do you agree?)

Can you send a V2 with an empty PATH and an updated comment?

Cheers,

Patch

diff -r 3afde791dce1 -r 3cda0b069802 setup.py
--- a/setup.py	Thu Oct 27 20:06:33 2016 +0200
+++ b/setup.py	Fri Oct 28 17:44:28 2016 +0200
@@ -167,7 +167,8 @@ 
 # to not use any hgrc files and do no localization.
 env = {'HGMODULEPOLICY': 'py',
        'HGRCPATH': '',
-       'LANGUAGE': 'C'}
+       'LANGUAGE': 'C',
+       'PATH': '.'}
 if 'LD_LIBRARY_PATH' in os.environ:
     env['LD_LIBRARY_PATH'] = os.environ['LD_LIBRARY_PATH']
 if 'SystemRoot' in os.environ: