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.

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

CI/RLS: upgrade to libspatialindex-2.0.0 #316

Merged
merged 7 commits into from
Jul 9, 2024

Conversation

mwtoews
Copy link
Member

@mwtoews mwtoews commented Jun 11, 2024

Towards #315.

This PR does the following:

  • Upgrade libspatialindex from 1.9.3 to 2.0.0
  • Remove the unneeded ci/CMakeLists.txt
  • Use cmake install, removing the unneeded lib subdirs
  • Remove the soversion number and symlinks for the library name (via CMAKE_PLATFORM_NO_VERSIONED_SONAME=ON)
  • It turns out the cibuildwheel tests on different python versions (via tox) was misleading, as it was not testing the built-wheel but the local source tree and sidx library build. This is fixed with pytest --import-mode=importlib
  • Consequently, the musllinux wheels on PyPI are "broken", as verified with docker run -it --rm alpine:3.18 /bin/sh --login then apk add py3-pip, pip install rtree, python -c "import rtree" to observe error. This is resolved by setting rpath for libspatialindex described next.
  • Use CMAKE_INSTALL_RPATH only for macOS. On Linux, $ORIGIN seems to confuse auditwheel and prevents creation of Rtree.libs. It want's LD_LIBRARY_PATH set while repairing the wheel. But the $ORIGIN rpath is still needed for musllinux, so manually add this last step in repair_wheel.py
  • Simplify copying directory trees in setup.py with copy_tree

@hobu
Copy link
Member

hobu commented Jun 26, 2024

Waiting on you to merge this @mwtoews. Should we fire a release afterward?

@mwtoews
Copy link
Member Author

mwtoews commented Jun 26, 2024

there one thing that I'm sorting out before merge, as I want to keep the linux lib dir in Rtree.libs, since I see downstream packages rely on this.

@mwtoews
Copy link
Member Author

mwtoews commented Jul 9, 2024

There is yet another big list of changes on the build-side, updated in the PR description. This is ready to review by anyone, I'll probably merge it in another day after I've had a chance to carefully review the artefacts.

@mwtoews mwtoews merged commit 86d4e28 into Toblerity:master Jul 9, 2024
23 checks passed
@mwtoews mwtoews deleted the update-spidx-2.0.0 branch July 9, 2024 21:50
@EwoutH
Copy link
Contributor

EwoutH commented Jul 11, 2024

This looks like an amazing clean-up!

One thing I noticed, is that in the build CI vcpython27 is installed on Windows:

- name: Run Windows Preinstall Build
if: startsWith(matrix.os, 'windows')
run: |
choco install vcpython27 -f -y
ci\install_libspatialindex.bat

Is this still needed, or can it be updated?

@mwtoews
Copy link
Member Author

mwtoews commented Jul 11, 2024

It looks like vcpython27 was added in 2020 with #174, at the time when MSBuild was used. Perhaps the existing ilammy/msvc-dev-cmd is sufficient, since cl.exe is needed for cmake -G Ninja (in ci/install_libspatialindex.bat).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants