Skip to content

[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 3 commits into from
Dec 2, 2024

Conversation

stevapple
Copy link
Contributor

@stevapple stevapple commented Sep 20, 2024

Per request of #76568 (comment), this PR cherry-picks d7057c9 which replaces the use of host Python with the cached version.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm liking this direction!

} catch {
Write-Output "Installing pip ..."
Invoke-Program -OutNull $Python '-I' -m ensurepip -U --default-pip
}
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 because Expand-Archive requires .zip as file extension, and wheels aren’t extracted to standalone directories. Given that pip is already included (no download required) and simply just not installed, it shouldn’t be a real problem.

Copy link
Member

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 doing python -I -m ensurepip -U --default-pip.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

@compnerd
Copy link
Member

@swift-ci please test Windows platform

@stevapple
Copy link
Contributor Author

Although a workaround is applied, I do think we were misusing T: in the CI config. It is a relative path on Windows, so appending any component to it will result in another relative path, which is most likely not what we want for a build script.

@compnerd
Copy link
Member

@swift-ci please test Windows platform

@compnerd
Copy link
Member

@swift-ci please test

@weliveindetail
Copy link
Member

@stevapple Are you fine to merge this?

@stevapple
Copy link
Contributor Author

@stevapple Are you fine to merge this?

Ofc, let's go ahead:)

@stevapple stevapple requested a review from compnerd December 2, 2024 18:08
@compnerd compnerd merged commit ab04b3a into swiftlang:main Dec 2, 2024
5 checks passed
@stevapple stevapple deleted the no-host-python branch December 2, 2024 18:47
hjyamauchi added a commit to hjyamauchi/swift that referenced this pull request Feb 4, 2025
hjyamauchi added a commit to hjyamauchi/swift that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants