isciences / exactextract

Fast and accurate raster zonal statistics
Apache License 2.0
233 stars 31 forks source link

add cibuildwheel #87

Closed vincentsarago closed 4 months ago

vincentsarago commented 4 months ago

ref #84

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.59%. Comparing base (0883cd5) to head (1a5e579).

:exclamation: Current head 1a5e579 differs from pull request most recent head 7d7a5d3. Consider uploading reports for the commit 7d7a5d3 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #87 +/- ## ======================================= Coverage 87.59% 87.59% ======================================= Files 68 68 Lines 3709 3709 ======================================= Hits 3249 3249 Misses 460 460 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vincentsarago commented 4 months ago

The wheels building fails for MacOS with

        /Users/runner/work/exactextract/exactextract/src/map_feature.h:52:21: error: 'visit<exactextract::Feature::FieldTypeGetter &, const std::variant<std::string, double, int, long long, exactextract::Feature::Array<double>, exactextract::Feature::Array<int>, exactextract::Feature::Array<long long>> &, void>' is unavailable: introduced in macOS 10.13
dbaston commented 4 months ago

The wheels building fails for MacOS

Could try this? (in the root CMakeLists.txt)

if (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
  # workaround for MacOS 10.13 and below which does not fully support std::variant
  add_compile_definitions(_LIBCPP_DISABLE_AVAILABILITY)  
endif()

from https://github.com/cbielow/OpenMS/commit/871a48ee30cc3bed7756de1a42db40e909360985

dbaston commented 4 months ago

Are there potential runtime problems if a user also has shapely loaded, maybe with a different version of GEOS?

vincentsarago commented 4 months ago

Are there potential runtime problems if a user also has shapely loaded, maybe with a different version of GEOS?

no real idea but this a commun things that is happening

that's a risk of providing wheels, but the gain of it is much bigger IMO

dbaston commented 4 months ago

That's fine. I'm new to Python packaging, just trying to understand the issues. I was thinking along the lines of making shapely a package requirement and linking against its copy of GEOS. But it looks like shapely and rasterio, for example, are both including their own copies of GEOS. I guess that's just not how it's done.

vincentsarago commented 4 months ago

So close 😭

Wheels building works for all arch but repair wheels (the process that move the C lib within the python package) is failing for Mac OS arm64 :-(

I have no idea what's going on and this might be tricky to debug.

shamelessly tagging @jorisvandenbossche which might have more knowledge than myself 😄

error log:

``` delocate-wheel --require-archs arm64 -w /private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/repaired_wheel -v /private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/built_wheel/exactextract-0.2.0.dev0-cp39-cp39-macosx_11_0_arm64.whl ERROR:delocate.libsana: @rpath/libgeos_c.1.dylib not found: Needed by: /private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/tmpnaa18fon/wheel/exactextract/_exactextract.cpython-39-darwin.so Search path: ERROR:delocate.libsana: @rpath/libgeos_c.1.dylib not found: Needed by: /private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/tmpnaa18fon/wheel/exactextract/_exactextract.cpython-39-darwin.so Search path: ERROR:delocate.libsana:@rpath/libgeos_c.1.dylib not found, requested by /private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/tmpnaa18fon/wheel/exactextract/_exactextract.cpython-39-darwin.so Fixing: /private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/built_wheel/exactextract-0.2.0.dev0-cp39-cp39-macosx_11_0_arm64.whl Traceback (most recent call last): File "/private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/build/venv/bin/delocate-wheel", line 8, in sys.exit(main()) File "/private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/build/venv/lib/python3.9/site-packages/delocate/cmd/delocate_wheel.py", line 92, in main copied = delocate_wheel( File "/private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/build/venv/lib/python3.9/site-packages/delocate/delocating.py", line 675, in delocate_wheel copied_libs = delocate_path( File "/private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/build/venv/lib/python3.9/site-packages/delocate/delocating.py", line 490, in delocate_path lib_dict = tree_libs_from_directory( File "/private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/build/venv/lib/python3.9/site-packages/delocate/libsana.py", line 379, in tree_libs_from_directory return _tree_libs_from_libraries( File "/private/var/folders/1k/qq3pcbf12vb6vyblh81736p40000gn/T/cibw-run-80e_0y61/cp39-macosx_arm64/build/venv/lib/python3.9/site-packages/delocate/libsana.py", line 320, in _tree_libs_from_libraries raise delocate.delocating.DelocationError( delocate.delocating.DelocationError: Could not find all dependencies. ```
jorisvandenbossche commented 4 months ago

Hmm, I don't directly see anything .. I assume it's mostly a verbatim copy of the shapely set up? Do you know of any place where you might have deviated?

jorisvandenbossche commented 4 months ago

The built GEOS libs are definitly installed according to the logs:

   [100%] Built target geosop
  -- Install configuration: "Release"
  -- Installing: /Users/runner/work/_temp/geos-3.12.1/lib/libgeos.3.12.1.dylib
  -- Installing: /Users/runner/work/_temp/geos-3.12.1/lib/libgeos.dylib
  -- Installing: /Users/runner/work/_temp/geos-3.12.1/lib/libgeos_c.1.18.1.dylib
  -- Installing: /Users/runner/work/_temp/geos-3.12.1/lib/libgeos_c.1.dylib
  -- Installing: /Users/runner/work/_temp/geos-3.12.1/lib/libgeos_c.dylib
...

So maybe it's something with the CMake of exactextract itself that needs a tweak for Mac arm to correctly find the lib? Did you have CI running on that architecture before that is working?

vincentsarago commented 4 months ago

thanks so much for looking @jorisvandenbossche 🙏

I assume it's mostly a verbatim copy of the shapely set up?

Yes 🙈

Do you know of any place where you might have deviated?

the GEOS install step are the same, so the only difference are within the python module

jorisvandenbossche commented 4 months ago

Another thing I am wondering. There is LDFLAGS=-Wl,-rpath,${{ runner.temp }}/geos-${{ env.GEOS_VERSION }}/lib for Mac. Maybe setuptools is handling that env variable differently compared to scikit_build_core?

vincentsarago commented 4 months ago

thanks @jorisvandenbossche I think that's a good thing to look at 🙏

jorisvandenbossche commented 4 months ago

There is a

ld: warning: duplicate -rpath '/Users/runner/work/_temp/geos-3.12.1/lib' ignored

that is printed in the Mac arm build, but not in the x86_64 one.

Could you maybe add a set( CMAKE_VERBOSE_MAKEFILE on ) somewhere to the python CMakeLists file, to make the output of CMake itself also a bit more verbose (so we can see the compile command it is using and what the other rpath flag is)?

jorisvandenbossche commented 4 months ago

That didn't seem to have worked, potentially because scikit-build-core is using ninja and not make. But it has an option for it that might translate better to ninja:

[tool.scikit-build]
cmake.verbose = true

Can you try that by adding it to the pyproject.toml config?

dbaston commented 4 months ago

Wondering if this could be related to https://github.com/libgeos/geos/issues/964

jorisvandenbossche commented 4 months ago

FWIW the verbose option now worked with the last commit, but so the verbose logs don't directly reveal any issue to me .. (the compile commands are not different compared to the x86_64 version)

Wondering if this could be related to libgeos/geos#964

That might indeed be an option, although I still find it very strange in that case why we don't see this in shapely. Maybe it has something to do with the fact that the python extension module here also links against libexactextract which is linked with GEOS, while in shapely it's only the python extension module itself that directly links with GEOS, don't know if that might explain the difference)

jorisvandenbossche commented 4 months ago

I just realize: in shapely our setup.py set ups a bunch of library and include dirs (-L, -I, -l), which here needs to be done in the CMake file I suppose. That might explain why there are differences with the shapely wheel build.

vincentsarago commented 4 months ago

I've been reading multiple threads about changes between Xcode 13 (x86_64) and Xcode 15 (arm64).

ref: https://indiestack.com/2023/10/xcode-15-duplicate-library-linker-warnings/

jorisvandenbossche commented 4 months ago

The "duplicate" warning might have been a red herring. Based on the verbose logs, -Wl,-rpath,/Users/runner/work/_temp/geos-3.12.1/lib is being passed twice. And that now triggers this warning, but I assume that apart from showing that warning it still works the same as before, and that we can ignore that warning for now.

jorisvandenbossche commented 4 months ago

Maybe one other thing to try is to replace

LDFLAGS=-Wl,-rpath,${{ runner.temp }}/geos-${{ env.GEOS_VERSION }}/lib

with

LDFLAGS="$LDFLAGS -Wl,-rpath,${{ runner.temp }}/geos-${{ env.GEOS_VERSION }}/lib"

to ensure LDFLAGS is appended and not overwritten, in case it was already set to something by cmake

thomascoquet commented 3 months ago

Just passing by to say thank you for this awesome package and the wheel building :heart: