-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[build.ps1] don't use host Python any more #76594
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 there a way to just perform the installation assuming it is not present without any adverse effects?
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 don’t think that’s a good idea, but it’s certainly doable.
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.
Had a simple try and found that we cannot easily reuse
Extract-ZipFile
becauseExpand-Archive
requires.zip
as file extension, and wheels aren’t extracted to standalone directories. Given thatpip
is already included (no download required) and simply just not installed, it shouldn’t be a real problem.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 that using
pip
is fine, just trying to see if we can simplify some of the logic here by just unconditionally doingpython -I -m ensurepip -U --default-pip
.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.
Running conditionally should work, but I intended to log precisely about what’s happening. Also need to notice that this command will output some noise when you already have pip installed.
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.
Thanks for working on this @stevapple! The move away from host Python solved an issue for me accessing modules in LLDB tests. And I can re-use your approach for installing modules: 942538f What else do you think will be necessary to merge this 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.
@weliveindetail Glad to hear that it helped you! Let’s wait for @compnerd to kick off a new CI run and see if there’s any other problem.
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.
Great. Then I will wait for it to land, rebase my changes on top of it and add my Python modules for lldb tests. Maybe I will have to factor out a function or so, that would be no problem to do. What do you think @compnerd?