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
Fix replacing local include paths #4136
Conversation
I can confirm that with these changes For completeness, regarding your PR description, I believe the issue is not specific to conda environments, it also happens when not using Python system (and not using a venv created from Python system). |
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.
Mostly out of curiosity, I have a few questions
@@ -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 |
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.
You removed the site-packages
case (where there was no replacement before this PR), I am not sure whether this was useful or not ...
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.
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_) |
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.
OK makes sense, thanks!
and "site-packages" not in arg | ||
): | ||
return arg.replace("-I" + sys.prefix, "-I" + target_install_dir) | ||
|
||
# Don't include any system directories | ||
if arg[2:].startswith("/usr"): |
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.
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.
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.
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 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) |
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.
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 ''
?
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.
Oh, indeed we are setting it to an empty string in out-of-tree build.
target_install_dir="", |
@hoodmane is it intended? I think it is incorrect.
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.
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.
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.
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.
"arg, expected", | ||
[ | ||
("-I/usr/include", None), | ||
(f"-I{sys.prefix}/include/python3.11", "-I/target/include/python3.11"), |
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.
Could we money patch base_prefix and prefix for the test? Or do you think that's overkill?
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.
I think it is a good idea. I'll add update a test
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.
Thanks for fixing it @ryanking13 and @lesteve for investigating! LGTM.
Great thanks for the fix! In a ideal world, this would be included in 0.24.1, when it makes sense for you to release 0.24.1. |
Yes, we'll backport it. |
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Description
Resolve #4118
When running
pyodide build
in conda env,sys.prefix
is set to pypa/build virtualenv (/tmp/...
)sys.base_prefix
is set to conda directory (/opt/conda/...
)We need to replace the include path of both cases with our target install directory.
I locally checked with
simplejson
package that the error happened in #4118 is resolved with this fix.cc: @lesteve
Checklists