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). |
lesteve
left a comment
lesteve
left a comment
There was a problem hiding this comment.
Mostly out of curiosity, I have a few questions
| 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.
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.
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
| 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.
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.
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.
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.
Oh, indeed we are setting it to an empty string in out-of-tree build.
@hoodmane is it intended? I think it is incorrect.
There was a problem hiding this comment.
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.
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.
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.
I think it is a good idea. I'll add update a test
rth
left a comment
rth
left a comment
There was a problem hiding this comment.
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 <[email protected]>
Co-authored-by: Roman Yurchak <[email protected]>
4 participants
Description
Resolve #4118
When running
pyodide buildin conda env,sys.prefixis set to pypa/build virtualenv (/tmp/...)sys.base_prefixis 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
simplejsonpackage that the error happened in #4118 is resolved with this fix.cc: @lesteve
Checklists