Install unisolated packages into the isolated build venv and overlay their cross-build files - #21
Conversation
…build into no-host-site
for more information, see https://pre-commit.ci
|
Sounds good to me. Relying on |
| return libdir, [ | ||
| str(f.relative_to(libdir)) for f in package_dir.rglob("*") if f.is_file() | ||
| ] |
There was a problem hiding this comment.
So this is trying to get cross-build-files? How does it work in tree? Shouldn't we look at cross-build-files in meta.yaml in that case?
There was a problem hiding this comment.
Also, site-packages-extras maybe should have been called site-packages-cross-files or something a bit more descriptive...
There was a problem hiding this comment.
So this is trying to get cross-build-files? How does it work in tree? Shouldn't we look at cross-build-files in meta.yaml in that case?
As you can see in the buildpkg.py change, only cross-build-files are installed under the hostsitepackages on in-tree build (I think there are some room to optimize this logic though).
Also, site-packages-extras maybe should have been called site-packages-cross-files or something a bit more descriptive...
Totally agree, but the folder site-packages-extras is generated when creating the xbuildenv, so we'll need to fix there too.
There was a problem hiding this comment.
site-packages-extras is well baked into the xbuildenv tarballs at this point. site-packages-cross-files would break our backwards compatibility, which we set up after this PR was created (in #167) and have surprisingly managed to maintain it well at this point. I am not opposed to changing things, but since it's separate, I guess it should be out of scope here.
| assert (tmp_path / version / ".installed").exists() | ||
| assert manager.current_version == version | ||
|
|
||
| def test_install_cross_build_packages( |
There was a problem hiding this comment.
It feels like we could use an updated version of this test?
There was a problem hiding this comment.
This comment should no longer be applicable, as the test is back again.
We also now have test_install_reqs and test_replace_unisolated_packages in test_pypabuild.py if you want to take a look.
hoodmane
left a comment
There was a problem hiding this comment.
Thanks @ryanking13! I made some comments but it generally looks good.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| if in_xbuildenv(): | ||
| unisolated_packages_file = PYODIDE_ROOT / ".." / "requirements.txt" | ||
|
|
||
| if unisolated_packages_file.exists(): |
There was a problem hiding this comment.
I am not fond of using assert statements outside tests, but yes, we can use a FileNotFoundError instead, assuming that's close to what you suggested.
| sitepackagesdir = Path(purelib) | ||
| for name in unisolated: | ||
| package_dir = get_cross_build_files_dir(name) | ||
| if not package_dir.is_dir(): |
There was a problem hiding this comment.
Why would this happen? Shouldn't we assert that it exists?
There was a problem hiding this comment.
Not every unisolated package has cross-build files. The package may only need to be pinned to the cross-build version (for its console scripts, for example) without any file overlay. numpy and scipy have cross-build files to be copied, but cffi and pycparser do not. Let me add a comment about this with this explanation.
There was a problem hiding this comment.
Yeah, I think it is good to be extra safe here as the format of xbuildenv files can change in the future, and we can make mistakes.
Co-Authored-By: Hood Chatham <roberthoodchatham@gmail.com>
Co-Authored-By: Hood Chatham <roberthoodchatham@gmail.com>
This reverts commit a8f63ff.
We were incorrectly pulling in the oldest-supported-numpy dependency at build time despite it being marked for usage by Shapely only for Python 3.9.
|
Thanks @hoodmane and @ryanking13 for the reviews! I adjusted the CHANGELOG entries and fixed a few bugs where we were not respecting platform markers when parsing dependencies, which were exposed by raising an explicit error when encountering |
|
Oops, found a problem... but it's SciPy, not us: SciPy 1.17.1 (when the CI was green in pyodide/pyodide-recipes#591) had SciPy 1.18.0 has |
|
So I think we can either pass another cross file, in flight (Meson supports multiple cross files), which would look like: [binaries]
numpy-config = '/tmp/build-env-abcdef/bin/numpy-config'
f2py = '...'and this won't be exposed to the user (but I don't think it's an elegant solution). Or (I chose this) we can use |
|
Updated fiona as it was using Edit: pyodide/pyodide-recipes#591 is all green now. Fiona was the only package using |
ryanking13
left a comment
There was a problem hiding this comment.
It looks like I cannot approve this as this was originally my PR, but you can consider this comment as my approval.
Thanks for reviving this PR!
| unisolated: set[str] = set() | ||
| for reqstr in reqs: | ||
| req = Requirement(reqstr) | ||
| if req.marker and not req.marker.evaluate(): |
There was a problem hiding this comment.
could please add a comment of what not req.marker.evaulate() mean?
There was a problem hiding this comment.
Yes, I'll add one. Noting it here where I encountered it: our current Shapely recipe here uses oldest-supported-numpy as a build-time dependency only for Python versions older than 3.9. So without evaluate()ing the PEP 508 markers, we would look at oldest-supported-numpy ; python_version < '3.9', find only oldest-supported-numpy and trip up, raising an error even though it shouldn't concern us..
| req = Requirement(reqstr) | ||
| if req.marker and not req.marker.evaluate(): | ||
| continue | ||
| if canonicalize_name(req.name) == "oldest-supported-numpy": |
There was a problem hiding this comment.
sounds okay to me, but could you please add a comment why we are not supporting it?
There was a problem hiding this comment.
The only reason for not supporting it is because @hoodmane asked 😄 #21 (comment). It is not strictly needed for this PR, though. It won't break anything with the functionality change here; it's more of a deadweight cleanup. I can split it into its own PR.
|
|
||
| # So far among all packages in pyodide-recipes, only NumPy ships | ||
| # a .pc file, but I don't want to hardcode that here as such | ||
| def _get_unisolated_pkgconfig_dirs(venv_path: str) -> list[str]: |
There was a problem hiding this comment.
Makes sense to me. I am curious how numpy adds these files to PKG_CONFIG_LIBDIR during the build
There was a problem hiding this comment.
Actually, NumPy's only task is to ship the .pc file. That is recognised by Meson during the dependency (dependency('numpy')) search as one of the files to look on the system for. We then point the PKG_CONFIG_LIBDIR environment variable to it ourselves.
Another way for Meson to find NumPy is to list numpy-config in the [binaries] section of the Meson cross file. Right now:
numpy-config binary missing from cross or native file, or env var undefined.
Default target is not allowed for cross use
numpy-config found: NO
Meson does find numpy-config, but it refuses to use it because it's a host-machine tool. Meson assumes it may contain incorrect include/library paths for WASM, and we need to alter the cross file and add numpy-config to make it not do so.
However, I felt the pkg-config approach was cleaner, and that's what was recommended, so I went ahead with it.
|
Thanks for reviewing, both of you! |
This changes how we handle unisolated packages and the hostsitepackages directory.
The current mechanism of unisolated packages works as follows:
This mechanism caused a few issues, such as the one discussed in pyodide/pyodide#5012 (comment). In particular, build time scripts that these packages ship, like
f2pyandnumpy-configfrom NumPy, never actually made it into the isolated build environment, so they weren't available on PATH during the build.This PR changes it in the following way:
.pxdfiles) with the WASM-compatible versions by grabbing them from the cross-build environment.We keep the existing mechanism that installs cross-build packages into
HOSTSITEPACKAGES.PYTHONPATHis no longer pointed at it, though, because Python-level access during the build (such as importingnumpy, callingnumpy.get_include(), runningf2pyornumpy-config, and so on) now comes straight from the isolated venv.Some more info on why we kept
HOSTSITEPACKAGESaround is noted below, in the "Testing" section.[EDIT: reverted this one, will split it up into its own PR]: Another change is that
oldest-supported-numpyis no longer silently ignored when it appears as a build dependency and instead raises an error. It has been deprecated since NumPy 2.0, so packages that still depend on it should move to a plainnumpydependency. If this causes trouble for a specific recipe, it can still be ignored by settingignored_build_requirementsto "patchelf oldest-supported-numpy"` of course, but IMO we don't need to bother about it anymore. In pyodide/pyodide-recipes#591, only Fiona was using it. I updated its version to one that no longer uses it, and things are working fine there.Regarding the
PKG_CONFIG_LIBDIRchange to findnumpy.pc: this is a new issue, as SciPy 1.17.1 used to not requirenumpyexplicitly (https://github.com/scipy/scipy/blob/527eb7fd7953a1de068f94bf8b322f249b9405ae/scipy/meson.build#L37), but when I merged the PR for SciPy 1.18.0 (pyodide/pyodide-recipes#586), it has madenumpya hard requirement: https://github.com/scipy/scipy/blob/54ef5423f2e4376230ec3bfda6912a07a50958e3/scipy/meson.build#L34 (and scipy/scipy#24119 (comment)). We use this environment variable to scan for NumPy's pkg-config file and add it to the build.PKG_CONFIG_LIBDIRis not additive as such, but we respect it if it's already set by a package. My reason for doing so is noted here: scipy/scipy#24119 (comment).This also adds a
numpy-scripts-exampleintegration test recipe. I wrote a small extension module built with meson-python that relies onnumpy-configandf2pybeing on PATH to exercise the new mechanism. We don't compile anything Fortran-related.Testing
This was tested in pyodide/pyodide-recipes#591, where all 325 package builds and tests passed.
The first run turned up an issue with building RobotRaconteur, and opencv-python would have had the same issue. Their builds are CMake-based, and they need direct access to NumPy's
ndarrayobject.hand other headers innumpy/_core/include/numpy/, which used to work becausePYTHONPATHpointed to a WASM-patched NumPy install. With that mechanism gone, those headers were nowhere to be found, and the builds failed.A tempting fix for us would have been to add the entire
numpy/_core/include/numpy/tree (which contains roughly 20 header files, plus thelibdivideandrandomfolders) to NumPy'scross-build-files. I decided against doing that, because those files change often between versions and more importantly are not really platform-specific, so listing them would be a maintenance burden and would stretch what ourcross-build-filesfield is meant for (as I noted in pyodide/pyodide-recipes#591 (comment)). Instead, we kept the existing step that installs cross-build packages intoHOSTSITEPACKAGES, so packages that access these files directly from the filesystem, rather than through the isolated build venv, continue to work exactly as before. I expected more packages to have these issues, but only RobotRaconteur and opencv-python did. However, more packages which are based on C/C++ libraries with Python bindings exist in the wild (such as CasADi, which I maintain, though it doesn't need NumPy headers). We need to support such types of builds for packages for better adoption of Pyodide's build machinery.With that in place, everything builds successfully!