-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spack doesn't run site customization when started + vendor setuptools #9261
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
632d80a
to
adecc6d
Compare
This is ready for review. The strategy adopted to solve #9206 is to avoid running any Working on this made me aware of a lot of things I'd preferred not to know:
I tried this approach first, instead of tweaking |
adecc6d
to
04f3c7c
Compare
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. |
04f3c7c
to
0c4a91d
Compare
@tgamblin I splitted the first commit to make review easier in case. I'll try to submit an alternative PR that tweaks |
I can look at this on 10/9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofspack
(e.g. in the qa scripts)
@@ -31,4 +31,4 @@ fi | |||
spack config get compilers | |||
|
|||
# Run some build smoke tests, potentially with code coverage | |||
${coverage_run} bin/spack install ${SPEC} | |||
${coverage_run} bin/spack.py install ${SPEC} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
# - 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(minor) I'd rather this message appeared in the "except" block.
# | ||
# https://github.com/spack/spack/issues/9206 | ||
# | ||
# Unfortunately not running `import site` by default causes all sort of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@alalazo realized I should have pinged you |
@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#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).
0c4a91d
to
54412cc
Compare
fixes #9619
fixes #9206
fixes #9034
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 usepython -S
as an interpreter, instead of plainpython
.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).