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

Fix replacing local include paths #4136

Merged
merged 6 commits into from Sep 20, 2023
Merged

Fix replacing local include paths #4136

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d565cb
Replace both sys.prefix and sys.base_prefix
ryanking13 Sep 13, 2023
f74736e
Update changelog
ryanking13 Sep 13, 2023
2051773
Merge remote-tracking branch 'upstream/main' into pywasmcross-include
ryanking13 Sep 16, 2023
fd2ce2d
Set target install dir for out-of-tree build
ryanking13 Sep 16, 2023
d2b81a0
Monkeypatch prefix and base_prefix in tests
ryanking13 Sep 16, 2023
91c5f38
Merge branch 'main' into pywasmcross-include
rth Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/project/changelog.md
View file
Open in desktop
Expand Up @@ -14,6 +14,11 @@ myst:

# Change Log

## Unreleased

- {{ Fix }} Fixed `LONG_BIT definition appears wrong for platform` error happened in out-of-tree build.
{pr}`4136`

## Version 0.24.0

_September 13, 2023_
Expand Down
5 changes: 4 additions & 1 deletion pyodide-build/pyodide_build/out_of_tree/build.py
View file
Open in desktop
Expand Up @@ -15,6 +15,9 @@
cxxflags += f" {os.environ.get('CXXFLAGS', '')}"
ldflags = build_env.get_build_flag("SIDE_MODULE_LDFLAGS")
ldflags += f" {os.environ.get('LDFLAGS', '')}"
target_install_dir = os.environ.get(

Check warning on line 18 in pyodide-build/pyodide_build/out_of_tree/build.py

View check run for this annotation

Codecov / codecov/patch

pyodide-build/pyodide_build/out_of_tree/build.py#L18 

Added line #L18 was not covered by tests
"TARGETINSTALLDIR", build_env.get_build_flag("TARGETINSTALLDIR")
)
env = os.environ.copy()
env.update(build_env.get_build_environment_vars())

Expand All @@ -24,7 +27,7 @@
cflags=cflags,
cxxflags=cxxflags,
ldflags=ldflags,
target_install_dir="",
target_install_dir=target_install_dir,
exports=exports,
)

Expand Down
15 changes: 10 additions & 5 deletions pyodide-build/pyodide_build/pywasmcross.py
View file
Open in desktop
Expand Up @@ -226,14 +226,19 @@ def replay_genargs_handle_dashI(arg: str, target_install_dir: str) -> str | None
The new argument, or None to delete the argument.
"""
assert arg.startswith("-I")
if (
str(Path(arg[2:]).resolve()).startswith(sys.prefix + "/include/python")
and "site-packages" not in arg
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed the site-packages case (where there was no replacement before this PR), I am not sure whether this was useful or not ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that part is not useful, as site-packages directory is in /lib/site-packages and not in /include/python3.11/....

For header files in site-packages directories, we cover them by copying header files (e.g. numpy header files) into the virtual env directory:

pyodide/pyodide-build/pyodide_build/buildpkg.py

Lines 534 to 537 in e492392

cross_build_files = build_metadata.cross_build_files
if cross_build_files:
for file_ in cross_build_files:
shutil.copy((wheel_dir / file_), host_site_packages / file_)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK makes sense, thanks!

):
return arg.replace("-I" + sys.prefix, "-I" + target_install_dir)

# Don't include any system directories
if arg[2:].startswith("/usr"):
Copy link
Contributor

@lesteve lesteve Sep 13, 2023 •
edited

Choose a reason for hiding this comment

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

Just curious, could this system directories case can be removed since it is already tackled by the sys.base_prefix case?

Maybe there are other reasons to remove system directories than the Python include though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure to be honest, we will have to check. Some codes in pywasmcross.py are very old so no one remembers the context 😅

return None

# Replace local Python include paths with the cross compiled ones
include_path = str(Path(arg[2:]).resolve())
if include_path.startswith(sys.prefix + "/include/python"):
return arg.replace("-I" + sys.prefix, "-I" + target_install_dir)

if include_path.startswith(sys.base_prefix + "/include/python"):
return arg.replace("-I" + sys.base_prefix, "-I" + target_install_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

And another question while I am at it. In my case (and it seems in out-of-tree builds in general), target_install_dir is '', so
'-I/home/lesteve/micromamba/envs/pyodide-0.24.0a1/include/python3.11'
gets replaced by
'-I/include/python3.11'

/include/python3.11 does not exist on my machine and in general I would not expect it to exist so I am not too sure about the goal of the replacement besides disabling the include directory ... I guess maybe this replacement is useful for in-tree builds where target_install_dir is not ''?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, indeed we are setting it to an empty string in out-of-tree build.

pyodide/pyodide-build/pyodide_build/out_of_tree/build.py

Line 27 in e492392

target_install_dir="",

@hoodmane is it intended? I think it is incorrect.

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 I just didn't know what this variable does or how to fill it, and setting it empty didn't break anything so I left it like that. Since something is breaking now it you probably know what should actually go there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'll update that part to set the correct target_install_dir. I think it didn't break since we append cpython include all the time.


return arg


Expand Down
25 changes: 25 additions & 0 deletions pyodide-build/pyodide_build/tests/test_pywasmcross.py
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@
get_library_output,
handle_command_generate_args,
replay_f2c,
replay_genargs_handle_dashI,
)


Expand Down Expand Up @@ -145,6 +146,30 @@ def test_handle_command_ldflags(build_args):
)


def test_replay_genargs_handle_dashI(monkeypatch):
import sys

mock_prefix = "/mock_prefix"
mock_base_prefix = "/mock_base_prefix"
monkeypatch.setattr(sys, "prefix", mock_prefix)
monkeypatch.setattr(sys, "base_prefix", mock_base_prefix)

target_dir = "/target"
target_cpython_include = "/target/include/python3.11"

assert replay_genargs_handle_dashI("-I/usr/include", target_dir) is None
assert (
replay_genargs_handle_dashI(f"-I{mock_prefix}/include/python3.11", target_dir)
== f"-I{target_cpython_include}"
)
assert (
replay_genargs_handle_dashI(
f"-I{mock_base_prefix}/include/python3.11", target_dir
)
== f"-I{target_cpython_include}"
)


def test_f2c():
assert f2c_wrap("gfortran test.f") == "gcc test.c"
assert f2c_wrap("gcc test.c") is None
Expand Down