Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
main: build in a virtual env by default #18
main: build in a virtual env by default #18
Changes from all commits
a22e7b0
283b490
da91bdc
62cc3c2
e5efbcc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this
'''
style already consistent across the whole package? It's very unusual to use single-quotes for docstrings, even among code-bases that exclusively use single-quotes for strings (I even have them highlighted differently from'''
strings in my syntax highlighter).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.
+1 on this
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.
It is. I am open to change it, but it should come in a separate PR.
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 this test should run in an isolated python environment (one that has no pip/setuptools/etc installed) so there's really no chance things are being pulled from some other path on the python path. E.g. is this past failing if you change from
sys. get_scheme_names
to that manual thing you defined before? It should if it's an accurate test.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.
That "manual thing" I had before was hardcoding the scheme names, it should work the same. Why would this be failing?
What things being pulled? There is nothing being pulled. In this test I check if the environment is correct, after that I check if pip is called correctly, but we never actually call pip. I look at the path and make sure it is correct.
For eg. I import
sysconfig
before entering the isolated environment and make surepurelib
orplatlib
are not there.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.
It should have been failing on pypy, as pypy uses different scheme names then what you hardcoded. The fact that it did not, shows something is not totally correct in this sense.
To be fair I'm not sure this is enough. I'd expect we'd isolate from any potential PYTHONPATH the user might have set, or additional paths the user injects via user/sitecustomize.
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.
It shouldn't.
If you read the code you'll notice I remove both the default paths and the different schemes.
https://github.com/FFY00/python-build/blob/e5efbcca2df30b7de5e14477cdbd4fb83f1d932c/build/env.py#L55
The test is correct and working as expected.