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

hoodmane · 2024-10-03T11:07:28Z

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.

Issue: Cross builds: distinguish between installation prefix and host prefix in configure.ac? #124932

…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`.

brettcannon · 2024-10-03T17:35:48Z

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 · 2024-10-03T17:37:00Z

configure.ac

+if test -z "$host_prefix"; then
+  AS_CASE([$ac_sys_system],
+    [Emscripten], [host_prefix=/],
+    [WASI], [host_prefix=/],


I haven't tried the PR out, but won't this forcibly anchor a WASI build to mount lib at /?

I presume the WASI situation is more akin to the iOS/Android case, where the runtime mount point depends on how/where the code is running, rather than knowing ahead of time that the code will be installed at a known fixed mount point?

Well it seems to me it would be better to have it default to / or /usr than to /buildmachine/path/to/checkout/of/cpython/cross-build/wasm32-wasi. If it's only known at runtime, we can control that by setting the PYTHONHOME environment variable or from the C api by setting the path directly. This only controls the final fallback case for determining the prefix, so it would also be possible to fix it by giving a value for program_name that makes sense in the file system visible to Python. Leaving this set based on the install target on the build machine doesn't seem that likely to be helpful in my opinion.

But I would also be perfectly happy to leave it with the default value in all cases, it is just nice to have the option of overriding it with a configure argument.

Well it seems to me it would be better to have it default to / or /usr than to /buildmachine/path/to/checkout/of/cpython/cross-build/wasm32-wasi.

That's entirely a checkout thing and not a release thing. For instance, to package up the code, I copy that file to the right place. So that's why I'm asking if it only impacts a checkout or not as you can currently mount the files anywhere inside the WASI host and everything works since Python looks from the file location up for lib/python3.NN. As long as that keeps working then I'm fine with the change.

This only controls the final fallback case for determining the prefix, so it would also be possible to fix it by giving a value for program_name that makes sense in the file system visible to Python.

This makes it sound like it won't break the case I want to preserve since step 5 in getpath.py will override:

cpython/Modules/getpath.py

Lines 143 to 147 in f474391

# Step 5. Try to find prefix and exec_prefix relative to executable_dir,

# backtracking up the path until it is exhausted. This is the most common

# step to succeed. Note that if prefix and exec_prefix are different,

# exec_prefix is more likely to be found; however if exec_prefix is a

# subdirectory of prefix, both will be found.

As such, I'm good with the change, although if this is to help with wasi.py can that also be updated as a test case in this PR?

As such, I'm good with the change, although if this is to help with wasi.py can that also be updated as a test case in this PR?

Yeah I'll try it out and see if it works the way I think.

It does not work the way I thought: wasi.py is mounting the checkout root into the wasi file system and using step 3 from getpath.py to run Python from the source checkout directory. The source checkout is clean so it's necessary to adjust the PYTHONPATH so that we pick up the sysconfigdata.
https://github.com/python/cpython/blob/main/Modules/getpath.py#L128-L135

But if I set --prefix={wasi_build_dir}, run make commoninstall instead of make all, and mount the wasi_build_dir/lib to /lib then this change helps. I'll push these changes to this branch to wasi.py to demonstrate and we can either keep them or revert them as you prefer.

freakboy3742

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.

freakboy3742 · 2024-10-03T23:17:59Z

configure.ac

+if test -z "$host_prefix"; then
+  AS_CASE([$ac_sys_system],
+    [Emscripten], [host_prefix=/],
+    [WASI], [host_prefix=/],


I presume the WASI situation is more akin to the iOS/Android case, where the runtime mount point depends on how/where the code is running, rather than knowing ahead of time that the code will be installed at a known fixed mount point?

bedevere-app · 2024-10-03T23:18:35Z

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.

Tools/wasm/wasi.py

brettcannon

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

Tools/wasm/wasi.py

brettcannon · 2024-10-08T16:21:28Z

Tools/wasm/wasi.py

@@ -264,7 +257,7 @@ def configure_wasi_python(context, working_dir):
 @subdir(lambda context: CROSS_BUILD_DIR / context.host_triple)
 def make_wasi_python(context, working_dir):
    """Run `make` for the WASI/host build."""
-    call(["make", "--jobs", str(cpu_count()), "all"],
+    call(["make", "--jobs", str(cpu_count()), "commoninstall"],


Doesn't this (attempt to) trigger an install of Python itself?

cpython/Makefile.pre.in

Lines 2220 to 2222 in fca5529

commoninstall: check-clean-src @FRAMEWORKALTINSTALLFIRST@ \

altbininstall libinstall inclinstall libainstall \

sharedinstall altmaninstall @FRAMEWORKALTINSTALLLAST@

That doesn't seem quite right to be triggering here as we aren't doing an install. For instance, commoninstall includes altbininstall which is used to install the interpreter:

cpython/Makefile.pre.in

Lines 2248 to 2251 in fca5529

# Install the interpreter with $(VERSION) affixed

# This goes into $(exec_prefix)

.PHONY: altbininstall

altbininstall: $(BUILDPYTHON) @FRAMEWORKPYTHONW@

Are you doing this to get sysconfig copied to lib/ since the install commands blindly copy what's in that directory?

I think doing an install in CI is good (as opposed to running the source directory) because I think downstream users are likely to want to do an install themselves. The CI is the most authoritative reference on how things should work for the downstream so in absence of other more important factors it's nice to make it helpful.

Because I set --prefix={working_dir} in the call to configure, it's installing into the build directory. Which is a bit sloppy but it works.

But what's actually needed here is that commoninstall implies libinstall and sharedinstall.

libinstall copies $CHECKOUT/Lib to $BUILDDIR/lib and also copies the sysconfigdata into lib here:

cpython/Makefile.pre.in

Lines 2620 to 2621 in 21c04e1

$(INSTALL_DATA) `cat pybuilddir.txt`/_sysconfigdata_$(ABIFLAGS)_$(MACHDEP)_$(MULTIARCH).py \

$(DESTDIR)$(LIBDEST); \

The other target that we use is sharedinstall which makes a lib-dynload folder and prevents getpath.py from printing 'Could not find platform dependent libraries <exec_prefix>'. This message is harmless, but it's nice to suppress it.

hoodmane requested review from erlend-aasland and corona10 as code owners October 3, 2024 11:07

bedevere-app bot added the awaiting review label Oct 3, 2024

bedevere-app bot mentioned this pull request Oct 3, 2024

Cross builds: distinguish between installation prefix and host prefix in configure.ac? #124932

Open

corona10 assigned brettcannon Oct 3, 2024

brettcannon added the OS-wasi label Oct 3, 2024

brettcannon reviewed Oct 3, 2024

View reviewed changes

brettcannon requested a review from freakboy3742 October 3, 2024 18:16

freakboy3742 requested changes Oct 3, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Oct 3, 2024

brettcannon assigned freakboy3742 Oct 4, 2024

Changes to wasi.py to use make sharedinstall and host_prefix

4ceaefd

hoodmane commented Oct 5, 2024

View reviewed changes

Tools/wasm/wasi.py Show resolved Hide resolved

brettcannon requested changes Oct 8, 2024

View reviewed changes

Fix CI

49ad12e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hoodmane commented Oct 3, 2024 •

edited by bedevere-app bot

Loading

brettcannon commented Oct 3, 2024

brettcannon Oct 3, 2024

freakboy3742 Oct 3, 2024

hoodmane Oct 4, 2024 •

edited

Loading

brettcannon Oct 4, 2024

hoodmane Oct 4, 2024

hoodmane Oct 5, 2024

freakboy3742 left a comment

freakboy3742 Oct 3, 2024

bedevere-app bot commented Oct 3, 2024

brettcannon left a comment

brettcannon Oct 8, 2024

hoodmane Oct 9, 2024

	# Step 5. Try to find prefix and exec_prefix relative to executable_dir,
	# backtracking up the path until it is exhausted. This is the most common
	# step to succeed. Note that if prefix and exec_prefix are different,
	# exec_prefix is more likely to be found; however if exec_prefix is a
	# subdirectory of prefix, both will be found.

	commoninstall: check-clean-src @FRAMEWORKALTINSTALLFIRST@ \
	altbininstall libinstall inclinstall libainstall \
	sharedinstall altmaninstall @FRAMEWORKALTINSTALLLAST@

	# Install the interpreter with $(VERSION) affixed
	# This goes into $(exec_prefix)
	.PHONY: altbininstall
	altbininstall: $(BUILDPYTHON) @FRAMEWORKPYTHONW@

	$(INSTALL_DATA) `cat pybuilddir.txt`/_sysconfigdata_$(ABIFLAGS)_$(MACHDEP)_$(MULTIARCH).py \
	$(DESTDIR)$(LIBDEST); \

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

Are you sure you want to change the base?

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

Conversation

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

brettcannon commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoodmane Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Oct 3, 2024

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoodmane commented Oct 3, 2024 •

edited by bedevere-app bot

Loading

hoodmane Oct 4, 2024 •

edited

Loading