Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-124932: Distinguish build prefix from host prefix in cross builds #124933

Merged
merged 2 commits into from
Oct 29, 2024
Merged

gh-124932: Distinguish build prefix from host prefix in cross builds #124933

merged 2 commits into from
Oct 29, 2024

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Oct 3, 2024 •
edited by bedevere-app bot
Loading

In Emscripten and wasi builds, and presumably for other cross builds, the build file system and the host file system look different. For instance, we may want to install into cross-build/$TARGET/lib and then mount that as /lib in the host file system. wasi.py has to mess around with setting PYTHONPATH because prefix is set to a path from the build machine. It would simplify this if we distinguish between:

  • prefix -- the path in the build file system where we want to install the files
  • host_prefix -- the path in the host file system where getpath.c will look for the files

And similarly for exec_prefix and host_exec_prefix.

@bedevere-app bedevere-app bot mentioned this pull request Oct 3, 2024
Closed
@brettcannon
Copy link
Member

I think the issue title is a bit misleading as this is only for WASI and Emscripten (which isn't officially tier 3 again yet).

brettcannon
brettcannon reviewed Oct 3, 2024
View reviewed changes
configure.ac Outdated Show resolved Hide resolved
freakboy3742
freakboy3742 requested changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Agreed the ability to differentiate between build-time and run-time general capability would be useful in other cross-build situations (notably, iOS and Android). One Q to resolve inline about WASI support; there's also a need for a NEWS entry.

configure.ac Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Oct 3, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

hoodmane
hoodmane commented Oct 5, 2024
View reviewed changes
Tools/wasm/wasi.py Outdated Show resolved Hide resolved
brettcannon
brettcannon requested changes Oct 8, 2024
View reviewed changes
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

I need clarification on the use of commoninstall as a make target.

Tools/wasm/wasi.py Outdated Show resolved Hide resolved
Tools/wasm/wasi.py Outdated Show resolved Hide resolved
Tools/wasm/wasi.py Outdated Show resolved Hide resolved
@brettcannon brettcannon removed their assignment Oct 9, 2024
brettcannon
brettcannon requested changes Oct 9, 2024
View reviewed changes
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Let's drop the WASI parts of this to keep it simple.

@hoodmane hoodmane force-pushed the host-prefix branch 2 times, most recently from 49b66c9 to abf8115 Compare October 9, 2024 19:45
@hoodmane
Copy link
Contributor Author

hoodmane commented Oct 9, 2024

Okay I removed all wasi-related stuff.

@erlend-aasland erlend-aasland changed the title gh-124932: Cross builds: Distinguish build prefix from host prefix gh-124932: Distinguish build prefix from host prefix in cross builds Oct 14, 2024
@hoodmane
Copy link
Contributor Author

@brettcannon I removed all wasi stuff, can you resolve the request-changes?

@freakboy3742 I added a news entry, I think that was your remaining request?

@hoodmane
Copy link
Contributor Author

Hmm but test failures look pretty related to the change.

@hoodmane
pythongh-124932: Cross builds: Distinguish between build prefix and h…
72f581e 
…ost prefix

In Emscripten and wasi builds, and presumably for other cross builds, the build
file system and the host file system look different. For instance, we may want
to install into `cross-build/$TARGET/lib` and then mount that as `/lib` in the
host file system. `wasi.py` has to mess around with setting `PYTHONPATH`
because `prefix` is set to a path from the build machine. It would simplify
this if we distinguish between:

* `prefix` -- the path in the build file system where we want to install the files
* `host_prefix` -- the path in the host file system where getpath.c will look for the files

And similarly for `exec_prefix` and `host_exec_prefix`.
@hoodmane
Add news entry
11d3793
@brettcannon brettcannon requested review from brettcannon and removed request for brettcannon October 25, 2024 19:59
brettcannon
brettcannon reviewed Oct 25, 2024
View reviewed changes
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Let's see if this resets my review ...

@brettcannon brettcannon self-requested a review October 25, 2024 20:02
@brettcannon
Copy link
Member

I manually reset my review state, so hopefully that will unblock this on my end.

@hoodmane
Copy link
Contributor Author

Thanks! Looks fixed to me.

freakboy3742
freakboy3742 approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

LGTM!

@freakboy3742
Copy link
Contributor

@brettcannon Just confirming you were happy with this, and the shenanigans around the review state is just Github being "helpful"?

@brettcannon
Copy link
Member

@freakboy3742 I just approved to help GH along with its "helpfulness". 😉

@freakboy3742 freakboy3742 merged commit b1f13bc into python:main Oct 29, 2024
41 of 42 checks passed
@hoodmane hoodmane deleted the host-prefix branch October 30, 2024 08:57
@hoodmane
Copy link
Contributor Author

Thanks @freakboy3742 and @brettcannon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brettcannon brettcannon brettcannon approved these changes

@freakboy3742 freakboy3742 freakboy3742 approved these changes

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

Assignees

@freakboy3742 freakboy3742

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoodmane @brettcannon @freakboy3742