Last Comment Bug 782847 - Pymake native commands don't pass the correct environment to subprocesses
: Pymake native commands don't pass the correct environment to subprocesses
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on:
Blocks: 593585
  Show dependency treegraph
 
Reported: 2012-08-14 17:19 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2012-08-22 02:45 PDT (History)
4 users (show)
sid.bugzilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use clear() and update() instead (3.07 KB, patch)
2012-08-14 18:19 PDT, Siddharth Agarwal [:sid0] (inactive)
gps: review+
Details | Diff | Review

Description Siddharth Agarwal [:sid0] (inactive) 2012-08-14 17:19:44 PDT
http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/file/7d221562bc4a/pymake/process.py#l209 sets os.environ to a new dictionary. However, os.environ is actually a magic dictionary which calls os.putenv and affects the environment of subprocesses, while this one doesn't.

Oh, Python...
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2012-08-14 18:19:58 PDT
Created attachment 651965 [details] [diff] [review]
use clear() and update() instead
Comment 2 Gregory Szorc [:gps] 2012-08-21 12:58:45 PDT
Comment on attachment 651965 [details] [diff] [review]
use clear() and update() instead

Review of attachment 651965 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

Please note in the commit message that this is backwards incompatible but it brings native command behavior inline with what it should have been.

::: tests/pycmd.py
@@ +9,5 @@
>      f.write(os.environ[args[1]])
>  
> +def writesubprocessenvtofile(args):
> +  with open(args[0], 'w') as f:
> +    p = subprocess.Popen([sys.executable, "-c", 

whitespace.

@@ +12,5 @@
> +  with open(args[0], 'w') as f:
> +    p = subprocess.Popen([sys.executable, "-c", 
> +                          "import os; print os.environ['%s']" % args[1]],
> +                          stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +    stdout, stderr = p.communicate()

Shouldn't there be a p.wait() here?
Comment 3 Siddharth Agarwal [:sid0] (inactive) 2012-08-21 13:06:13 PDT
(In reply to Gregory Szorc [:gps] from comment #2)
> @@ +12,5 @@
> > +  with open(args[0], 'w') as f:
> > +    p = subprocess.Popen([sys.executable, "-c", 
> > +                          "import os; print os.environ['%s']" % args[1]],
> > +                          stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> > +    stdout, stderr = p.communicate()
> 
> Shouldn't there be a p.wait() here?

communicate implies wait: http://docs.python.org/library/subprocess.html#subprocess.Popen.communicate
Comment 4 Siddharth Agarwal [:sid0] (inactive) 2012-08-21 14:28:58 PDT
Pushed to the Pymake repository.
http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/8360595070d6

I'll push to mozilla-inbound once it reopens.
Comment 5 Siddharth Agarwal [:sid0] (inactive) 2012-08-21 15:38:45 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/dfe6a63f3ae9
Comment 6 Ed Morley [:emorley] 2012-08-22 02:45:44 PDT
https://hg.mozilla.org/mozilla-central/rev/dfe6a63f3ae9

Note You need to log in before you can comment on or make changes to this bug.