-
Notifications
You must be signed in to change notification settings - Fork 11
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
Build with wasm exception handling #81
base: main
Are you sure you want to change the base?
Changes from 24 commits
30cbe32
a02dda7
3186d20
8256e72
5e46696
adda8a5
b33d1c9
f2d72e7
3c3dc79
30517bd
0c88c0f
663e78e
529a583
a63c7b9
b50d1c1
3b1734b
c93aab1
036626d
7bbdde2
114e477
330e762
6a49a51
194c463
6fa5b9b
4cb167e
dc1c6a2
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 |
---|---|---|
|
@@ -77,6 +77,7 @@ class CrossCompileArgs(NamedTuple): | |
target_install_dir: str = "" # The path to the target Python installation | ||
pythoninclude: str = "" # path to the cross-compiled Python include directory | ||
exports: Literal["whole_archive", "requested", "pyinit"] | list[str] = "pyinit" | ||
abi: str = "" | ||
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. Let's add a comment about this parameter. |
||
|
||
|
||
def is_link_cmd(line: list[str]) -> bool: | ||
|
@@ -93,7 +94,7 @@ def is_link_cmd(line: list[str]) -> bool: | |
return False | ||
|
||
|
||
def replay_genargs_handle_dashl(arg: str, used_libs: set[str]) -> str | None: | ||
def replay_genargs_handle_dashl(arg: str, used_libs: set[str], abi: str) -> str | None: | ||
""" | ||
Figure out how to replace a `-lsomelib` argument. | ||
|
||
|
@@ -118,6 +119,9 @@ def replay_genargs_handle_dashl(arg: str, used_libs: set[str]) -> str | None: | |
if arg == "-lgfortran": | ||
return None | ||
|
||
if abi > "2025" and arg in ["-lfreetype", "-lpng"]: | ||
arg += "-wasm-sjlj" | ||
Comment on lines
+122
to
+123
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. Let's add a comment about the context of this if condition. |
||
|
||
# WASM link doesn't like libraries being included twice | ||
# skip second one | ||
if arg in used_libs: | ||
|
@@ -555,7 +559,7 @@ def handle_command_generate_args( # noqa: C901 | |
continue | ||
|
||
if arg.startswith("-l"): | ||
result = replay_genargs_handle_dashl(arg, used_libs) | ||
result = replay_genargs_handle_dashl(arg, used_libs, build_args.abi) | ||
elif arg.startswith("-I"): | ||
result = replay_genargs_handle_dashI(arg, build_args.target_install_dir) | ||
elif arg.startswith("-Wl"): | ||
|
@@ -578,7 +582,11 @@ def handle_command_generate_args( # noqa: C901 | |
# Better to fail at compile or link time. | ||
if is_link_cmd(line): | ||
new_args.append("-Wl,--fatal-warnings") | ||
new_args.extend(build_args.ldflags.split()) | ||
for arg in build_args.ldflags.split(): | ||
if arg.startswith("-l"): | ||
arg = replay_genargs_handle_dashl(arg, used_libs, build_args.abi) | ||
Comment on lines
+586
to
+587
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. Not a strong opinion, but as 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 have mixed feelings. I guess we can't fix static/dynamic libs not built with the python build system anyways so it's not that valuable. But it's pretty weird and annoying that linking So yeah probably it makes sense to remove this. 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.
Yeah, I think the problem is that we are not building libraries like Ideally, I think it is better to drop all emscripten ports and build libraries ourselves. I think this would fix the issue with the Emscripten version and library version being fixed, and it would also fix the variant issues like the wasm exception. 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. BTW, I noticed that the library suffices are changed in the recent version of Emscripten: https://github.com/emscripten-core/emscripten/blob/242af002f8dee0366e86002fec187d447dcd26e5/tools/ports/libpng.py#L12-L16 |
||
if arg: | ||
new_args.append(arg) | ||
new_args.extend(get_export_flags(line, build_args.exports)) | ||
|
||
if "-c" in line: | ||
|
@@ -638,6 +646,7 @@ def compiler_main(): | |
target_install_dir=PYWASMCROSS_ARGS["target_install_dir"], | ||
pythoninclude=PYWASMCROSS_ARGS["pythoninclude"], | ||
exports=PYWASMCROSS_ARGS["exports"], | ||
abi=PYWASMCROSS_ARGS["abi"], | ||
) | ||
basename = Path(sys.argv[0]).name | ||
args = list(sys.argv) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ | |
from pyodide_build import build_env | ||
from pyodide_build.build_env import BuildArgs | ||
from pyodide_build.common import ( | ||
download_and_unpack_archive, | ||
exit_with_stdio, | ||
extract_wheel_metadata_file, | ||
find_matching_wheels, | ||
|
@@ -700,26 +701,50 @@ def run(self, n_jobs: int, already_built: set[str]) -> None: | |
self.build_queue.put((job_priority(dependent), dependent)) | ||
|
||
|
||
def _run(cmd, *args, check=False, **kwargs): | ||
result = subprocess.run(cmd, *args, **kwargs, check=check) | ||
if result.returncode != 0: | ||
logger.error("ERROR: command failed %s", " ".join(cmd)) | ||
exit_with_stdio(result) | ||
return result | ||
|
||
|
||
def _ensure_rust_toolchain(): | ||
rust_toolchain = build_env.get_build_flag("RUST_TOOLCHAIN") | ||
result = subprocess.run( | ||
["rustup", "toolchain", "install", rust_toolchain], check=False | ||
) | ||
if result.returncode == 0: | ||
result = subprocess.run( | ||
_run(["rustup", "toolchain", "install", rust_toolchain]) | ||
_run(["rustup", "default", rust_toolchain]) | ||
|
||
url = build_env.get_build_flag("RUST_EMSCRIPTEN_TARGET_URL") | ||
if not url: | ||
# Install target with rustup target add | ||
_run( | ||
[ | ||
"rustup", | ||
"target", | ||
"add", | ||
"wasm32-unknown-emscripten", | ||
"--toolchain", | ||
rust_toolchain, | ||
], | ||
check=False, | ||
] | ||
) | ||
if result.returncode != 0: | ||
logger.error("ERROR: rustup toolchain install failed") | ||
exit_with_stdio(result) | ||
return | ||
|
||
# Install target from url | ||
result = _run( | ||
["rustup", "which", "--toolchain", rust_toolchain, "rustc"], | ||
capture_output=True, | ||
text=True, | ||
) | ||
toolchain_root = Path(result.stdout).parents[1] | ||
rustlib = toolchain_root / "lib/rustlib" | ||
install_token = rustlib / "wasm32-unknown-emscripten_install-url.txt" | ||
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. Is this file something you added manually in your custom toolchain? If so, let's add a comment about it. |
||
if install_token.exists() and install_token.read_text() == url: | ||
return | ||
shutil.rmtree(rustlib / "wasm32-unknown-emscripten", ignore_errors=True) | ||
download_and_unpack_archive( | ||
url, rustlib, "wasm32-unknown-emscripten target", exists_ok=True | ||
) | ||
install_token.write_text(url) | ||
|
||
|
||
def build_from_graph( | ||
|
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.
Let's mention the default rust toolchain update in the changelog.
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 change isn't really needed to make this work. Maybe I can revert it here and put it by itself in a separate pr as a follow-up.