Spack doesn't run site customization when started + vendor setuptools by alalazo · Pull Request #9261 · spack/spack

alalazo · 2018-09-17T17:52:16Z

closes #9702 (alternative solution to the same issue)
closes #9725 (alternative solution to the same issue)

A possible solution to #9206 is to avoid running import site and all the initialization procedures that this entails. This requires us to use python -S as an interpreter, instead of plain python.

Of course things can't be that simple. In Linux (but not in BSD or MacOS) the shebang pass a single string argument to the interpreter. More details on the subject are here:

but the bottom line is that this requires us to have a wrapper bash script that invokes the interpreter with the correct option.

Finally, as now we are not picking up anything from the 'site', we need to vendor setuptools (which was still missing).

alalazo · 2018-09-18T15:49:54Z

This is ready for review. The strategy adopted to solve #9206 is to avoid running any .pth file by using python -S as the interpreter. This allows us to isolate spack from any site package that might be present in the environment, including ruamel.yaml.

Working on this made me aware of a lot of things I'd preferred not to know:

Virtualenvs differ in many subtle details from a system installation of the same python version. To name a few: what is considered a site extension or a built-in module might change, site.py in virtualenv might come from a different version of Python than the one used.
/usr/bin/env behavior is different under Linux and BSD, MacOS. @tgamblin was surely already aware of this looking at spack-python in bin

I tried this approach first, instead of tweaking sys.modules because we are ensured that no other references to a different ruamel.yaml are left around. That said, please have a close look at the PR as it touches a few "dark corners" of the python interpreter (the commits should be well factored, just skip the first one that copies the vendored setuptools). Waiting for comments.

alalazo · 2018-10-01T14:14:41Z

@ax3l @tgamblin

ax3l · 2018-10-03T17:38:51Z

tested and works for me, but the changes are quite massive for me to grasp and ack as those are too deep inside spack for my knowledge of it.
that said, I am a big fan of python -S if we want to fully isolate.

alalazo · 2018-10-04T11:35:34Z

@tgamblin I splitted the first commit to make review easier in case. I'll try to submit an alternative PR that tweaks sys.modules asap.

scheibelp · 2018-10-04T19:36:13Z

I can look at this on 10/9

scheibelp

To summarize: it appears that python -S has almost no negative effects other than that the __future__ and inspect modules may not be available for virtual environments created from Python versions Spack is advertised to support, in spite of the fact that all such versions do in fact include these modules.

I prefer your suggestion from #9206 (comment) because it doesn't depend as much on understanding python import mechanisms and setup (that's more of a core-developer concern though so not a deal-breaker).

That being said, I think this approach is "safe" with the following caveat: This would interfere with Spack packages that are written to take advantage of additional functionality which they load into the Python instance they use to run Spack. I don't know of any packages which do that now, but I think it's a reasonable behavior to support.

If you don't see issues with your suggestion in #9206 (comment), and if you are time-constrained, I could add that to my plate. If You prefer this approach, I'd be interested to hear about the downsides of the alternative. I also have a couple suggestions:

The explanatory comment for the import handling in spack.py needs to be clarified
I think there may be an issue with using spack.py in place of spack (e.g. in the qa scripts)

scheibelp · 2018-10-09T23:01:02Z

share/spack/qa/run-build-tests


 # Run some build smoke tests, potentially with code coverage
-${coverage_run} bin/spack install ${SPEC}
+${coverage_run} bin/spack.py install ${SPEC}


Why is spack.py invoked vs. Spack? If the Travis VMs start using a newer version of Debian wouldn't they run into the same problem as #9206?

The issue is not tied to Debian, but to how ruamel.yaml is packaged. The trigger of the bug is to have a recent enough version of ruamel.yaml installed in the site directory of your interpreter. More info in #9206 (comment)

We need to have the python script up here because coverage run expects a python script as its argument (it can't go through arbitrary layers of other languages). We are ensured that we won't hit the issue because we control the test environment and we don't:

$ pip install ruamel.yaml

scheibelp · 2018-10-09T23:05:13Z

bin/spack.py

+# - the `site.py` used in Python 2.7 virtual environments is that of Python 2.6
+#   (https://github.com/pypa/virtualenv/issues/355)
+#
+# The hack below is needed to restore built-in modules from the system if


(minor) I'd rather this message appeared in the "except" block.

scheibelp · 2018-10-09T23:07:35Z

bin/spack.py

+#
+# https://github.com/spack/spack/issues/9206
+#
+# Unfortunately not running `import site` by default causes all sort of


The phrase "all sorts of inconsistencies" is causing me stress: can we be more precise? Are there more than these two that you would expect to be relevant to imports? I wouldn't think so given https://docs.python.org/2/library/site.html

Honestly I don't know, because these things are undocumented and I just happened to hit those 'inconsistencies' by running Spack under different conditions. Would you expect python -S to behave differently for imports of standard libraries and the same exact python version, depending on whether you run it from the system or from a virtualenv? I found this behavior surprising to put it mildly 😄 That said, I am open to any better wording you want to suggest.

scheibelp · 2018-10-09T23:09:17Z

bin/spack.py

+#   among python versions and might not include `__future__` or `inpect`
+#   (this means that `python -S` might prevent importing those modules)
+#
+# - the `site.py` used in Python 2.7 virtual environments is that of Python 2.6


I think the comment should be clearer that this second issue is not addressed and the first issue is. It so happens we don't depend on any of the functionality from the 2.7 site.py.

In fact, is the second issue really an issue given that we are invoking python -S? We aren't making use of site.py at all in that case and therefore shouldn't have this issue.

scheibelp · 2018-10-11T01:49:43Z

@alalazo realized I should have pinged you

alalazo · 2018-10-11T05:17:53Z

@scheibelp Will react to review asap. I'll also implement the alternatives, so we can compare them and see which one looks better.

fixes spack#9034

fixes spack#9206 fixes spack#9034 A possible solution to spack#9206 is to avoid running `import site` and all the initialization procedures that this entails. This requires us to use `python -S` as an interpreter, instead of plain `python`. Of course things can't be that simple. In Linux (but not in BSD or MacOS) the shebang pass a single string argument to the interpreter. More details on the subject are here: http://sambal.org/2014/02/passing-options-node-shebang-line/ https://github.com/smikes/node/blob/minus-x-switch/doc/Minus-X-Switch-Proposal.md#shebang-interpretation but the bottom line is that this requires us to have a wrapper bash script that invokes the interpreter with the correct option. Finally, as now we are not picking up anything from the 'site', we need to vendor `setuptools` (which was still missing).

alalazo added bug Something isn't working broken-develop don't-merge-yet hotfix labels Sep 17, 2018

alalazo requested review from adamjstewart, ax3l, scheibelp and tgamblin September 17, 2018 17:52

This comment has been minimized.

Sign in to view

alalazo mentioned this pull request Sep 17, 2018

Debian ruamel.yaml 0.15: Infinite Recoursion On Setup #9206

Closed

This comment has been minimized.

Sign in to view

alalazo force-pushed the fixes/ruamel_yaml branch from 632d80a to adecc6d Compare September 18, 2018 13:17

alalazo force-pushed the fixes/ruamel_yaml branch from adecc6d to 04f3c7c Compare October 1, 2018 12:53

alalazo force-pushed the fixes/ruamel_yaml branch from 04f3c7c to 0c4a91d Compare October 4, 2018 10:53

scheibelp self-assigned this Oct 4, 2018

scheibelp requested changes Oct 9, 2018

View reviewed changes

alalazo added 3 commits October 21, 2018 20:45

Vendor setuptools

b4e9690

fixes spack#9034

QA on Travis invokes python script directly, instead of the bash wrapper

54412cc

alalazo force-pushed the fixes/ruamel_yaml branch from 0c4a91d to 54412cc Compare October 21, 2018 18:56

willfurnass mentioned this pull request Oct 23, 2018

Python RecursionError when sourcing setup-env.sh #9619

Closed

This was referenced Nov 1, 2018

The vendored ruamel.yaml is imported under the namespace 'external' #9702

Closed

Delete references to ruamel.yaml at Spack start-up, if they are present #9725

Merged

tgamblin closed this in #9725 Nov 7, 2018

adamjstewart mentioned this pull request Mar 6, 2020

Vendor setuptools dependency #15369

Closed

tgamblin mentioned this pull request Mar 21, 2020

vendoring: remove dependency on setuptools from vendored pytest #15612

Merged

1 task

Conversation

alalazo commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

alalazo commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Oct 1, 2018

Uh oh!

ax3l commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Oct 4, 2018

Uh oh!

scheibelp commented Oct 4, 2018

Uh oh!

scheibelp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

alalazo Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

scheibelp Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

scheibelp Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

alalazo Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

scheibelp Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

scheibelp commented Oct 11, 2018

Uh oh!

alalazo commented Oct 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alalazo commented Sep 17, 2018 •

edited

Loading

alalazo commented Sep 18, 2018 •

edited

Loading

ax3l commented Oct 3, 2018 •

edited

Loading

scheibelp left a comment •

edited

Loading