-
Notifications
You must be signed in to change notification settings - Fork 31.6k
GH-131556: calculate PYBUILDDIR in the makefile instead of sysconfig #131761
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -195,11 +195,10 @@ def configure_emscripten_python(context, working_dir): | |
assert ( | ||
len(lib_dirs) == 1 | ||
), f"Expected a single lib.* directory in {python_build_dir}" | ||
lib_dir = os.fsdecode(lib_dirs[0]) | ||
pydebug = lib_dir.endswith("-pydebug") | ||
python_version = lib_dir.removesuffix("-pydebug").rpartition("-")[-1] | ||
_, python_version, abiflags = os.fsdecode(lib_dirs[0]).rsplit('-', maxsplit=2) | ||
sysconfig_data = ( | ||
f"{emscripten_build_dir}/build/lib.emscripten-wasm32-{python_version}" | ||
f"{emscripten_build_dir}/build/" | ||
f"lib.emscripten-wasm32-emscripten-{python_version}-{abiflags}" | ||
) | ||
if pydebug: | ||
sysconfig_data += "-pydebug" | ||
|
@@ -227,8 +226,13 @@ def configure_emscripten_python(context, working_dir): | |
"--enable-wasm-dynamic-linking", | ||
f"--prefix={PREFIX_DIR}", | ||
] | ||
if pydebug: | ||
configure.append("--with-pydebug") | ||
for flag in abiflags: | ||
if flag == 'd': | ||
configure.append("--with-pydebug") | ||
elif flag == 't': | ||
configure.append("--disable-gil") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what the state of Emscripten on free threading will be... I'm torn on whether it's better to leave the option enabled, and just be aware that it will quite possible break, or to error out if a free threaded build is used. Maybe the former with a warning message? Either way, we should probably handle it similar to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering the build-python installation built by this script is specifically meant to be used in the cross-compilation process for the Emscripten build, I don't think we need remove it. Having it here means the user specifically asked for it. Regarding the warning, I think it makes sense, but I think the build system is probably a more suitable place for it. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - I think the build system is a better place for that warning - assuming it's even needed. Its possible free threaded builds might work... especially given that there's no thread support in Emscripten. |
||
else: | ||
raise ValueError(f"Unknown ABI flag: {flag}") | ||
if context.args: | ||
configure.extend(context.args) | ||
call( | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
This won't quite work -
pydebug
is used in a couple of places (e.g., L203) that isn't covered by this change. I agree usingabiflags
is a better way to handle it - but we should probably use abiflags to setpydebug
.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.
Uh, I missed that line, I meant to do the exact same change I did to the WASI script. Since I changed the format of
pybuilddir.txt
, which is wheresysconfig_data
points to, we shouldn't be adding-pydebug
. My change to thesysconfig_data
variable already sets it to the correct value, I just forgot to remove the line below in the commit 😅.I don't remember seeing
pydebug
being used elsewhere, but I will check when I am back at the computer.That said, the script is broken as-is, meaning it must not be covered by the CI. We should probably fix that.
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.
Correct - is isn't, and yes, we should.
The machine that will be used for the Emscripten buildbot has been commissioned. Right now, Emscripten hard-crashes on a couple of tests when
-uall
is enabled, but once those are resolved, we should be able to add the Emscripten buildbot to the pool.