Add a test which enables PEX_VERBOSE and checks whether enabling inhe… #224
Conversation
…rit_path results in site-packages being not scrubbed. Since this fails, add processing of inherit_path to the pex environment booting.
| with open(os.path.join(td, 'exe.py'), 'w') as fp: | ||
| fp.write(exe_contents) | ||
|
|
||
| pb = PEXBuilder(path=td, preamble=COVERAGE_PREAMBLE if coverage else None) |
There was a problem hiding this comment.
This line doesn't fail only because coverage is never passed and defaults to False, otherwise boom on COVERAGE_PREAMBLE which is not a symbol in-scope. Killing the unused dists and coverage params would be good.
There was a problem hiding this comment.
Vars killed.
|
A few things to fix above, but also Travis-CI should be green as well. |
|
Am looking at Travis fails. |
| @@ -314,7 +317,9 @@ def execute(self): | |||
| """ | |||
| teardown_verbosity = self._vars.PEX_TEARDOWN_VERBOSE | |||
| try: | |||
| with self.patch_sys(): | |||
| pex_inherit_path = True if ( | |||
| self._vars.PEX_INHERIT_PATH or self._pex_info.inherit_path) else False | |||
There was a problem hiding this comment.
Just the bool expression itself would be more idiomatic and I think clearer:
pex_inherit_path = self._vars.PEX_INHERIT_PATH or self._pex_info.inherit_path
|
The last 3 comments are all debateable nits only, this change LGTM. Let me know on each, your call. I'll then merge this in. Thanks for this fix! |
|
I glossed over this when reading the PR description, but it sounds like this fix will be consumed via pants so I'm linking this to the 1.1.5 release ticket #237. Would you all be insterested in contributing Grapeshot to the powered-by page?: http://pantsbuild.github.io/powered_by.html |
|
Oh, yes, it's Pantsbuild we're actually using to compile things. (I'm I'm pretty sure the CEO will do that big happy grin of his to see us listed I'll send an email. On 20 April 2016 at 18:56, John Sirois [email protected] wrote:
|
|
Oh, we should do this properly. I haven't done any OS contribs in yonks I hadn't realised the pex writer took options, which does sort of collapse On 20 April 2016 at 18:53, John Sirois [email protected] wrote:
|
…inheriting inside the pex bootstrap. Remove code from test pex because we only want the verbose startup messages.
|
Thanks @KatieLucas-Grapeshot! This has been patched into master as 1 squashed commit keeping the initial description as the commit message here: adefd7e |
Grapeshot uses Pants to build PEX objects. We install MySQL interface code in site-packages and hence would like PEXes to collect this path on start-up. We have discovered the PANTS option to do this doesn't work. Fault traced to the environment scrubbing not honouring the setting or the equiv env variable.
PR adds a test which build such a PEX, enables PEX_VERBOSE, launches PEX and checks whether the enabling of inherit_path results in site-packages being not scrubbed. Since this fails, add processing of inherit_path to the pex environment booting. Test now passes.