Build wheels using vcpkg for Linux/MacOS #55
Conversation
| uses: pypa/[email protected] | ||
| env: | ||
| # should be automatically detected? | ||
| GDAL_INCLUDE_PATH: "/usr/local/include" |
There was a problem hiding this comment.
I don't think those will be automatically detected, because vcpkg does not a "system-wide" install AFAIK (so I would find it strange that this gets installed in /usr/local/include, but no idea for linux; on windows this is installed in a vcpkg specific directory)
There was a problem hiding this comment.
According to https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#environment-variables, it is actually in /usr/local where vcpkg itself gets installed, but not fully sure where exactly it would install binaries.
There was a problem hiding this comment.
These were placeholders from my more manual attempt at building GDAL; will be watching the GH Action to see where these get placed...
pyproject.toml
Outdated
|
|
||
| [tool.cibuildwheel.linux.environment] | ||
| GDAL_INCLUDE_PATH = "/vcpkg/packages/gdal_x64-linux/include" | ||
| GDAL_LIBRARY_PATH = "/vcpkg/packages/gdal_x64-linux/lib" |
There was a problem hiding this comment.
If works similar as on windows, the path might also be something like /vcpkg/installed/x64-linux/lib
There was a problem hiding this comment.
Indeed, that looks to be it.
|
Well, after much hacking about to get vcpkg working with cache (missed a few simple and obvious things), it looks like we actually need to install vcpkg and do the entire build within the Linux docker container used for cibuildwheel (docs). |
brendan-ward
changed the title
|
I think this is ready to squash & merge. Linux builds are dropped from here and will be added via #57. I made a few minor tweaks to Windows builds to get wheel tests to pass and fix name of test job. |
|
Do you know the behaviour of If it has not been tested, I can try that on my partner's laptop. |
|
Even though we're only building on |
|
@brendan-ward you removed the workflow file for Linux, but not all other changes for Linux (the env variables, the triplet file, the dockerfile). Now, I think it is also OK to actually leave the Linux support in here (in the end all the relevant discussion is also here). The current version is already creating valid, working wheels, they are only manylinux_2_27 wheels instead of the more compatible 2_17 (2014) wheels. (or otherwise remove everything linux related) |
brendan-ward
changed the title
|
To be clear, I am also fine with merging Mac support separately, it was just that now it was a bit in between.
My original intention was just to experiment with the custom zlib port, and doing that separately to not mess up this PR because I had no idea whether it would work. It only turned out to be working very nicely from the first try :) Now, keeping that separate might still have value, so that the zlib port is a nicely focused PR, so that it makes it easier to understand later what we did there. |
MANIFEST.in
Outdated
| @@ -2,4 +2,7 @@ include versioneer.py | |||
| include pyogrio/_version.py | |||
| include pyogrio/*.pyx pyogrio/*.pxd | |||
| exclude pyogrio/*.c | |||
| include LICENSE | |||
| include pyproject.toml | |||
| exclude MANIFEST.in | |||
There was a problem hiding this comment.
I am going to change this back (I think it is a left-over from some experimentation to get the tests working?), as my understanding is that this should not influence wheels but only sdist. This might still be a valid change for the sdist (didn't check that here), but then let's do that in a separate PR focused on sdist.
There was a problem hiding this comment.
That's fine; I added that while trying to get the pytest tests working, based on adapting what we have in pygeos. Certainly fine to deal with it in later PR.
|
I tried to clean up a few things, but not with that much of success (the github caching didn't seem to work with a direct docker buildx call, so reverted back to the docker-build-push action, and we can't define global and platform specific env variables at the same time in pyproject.toml). But so if we do the consolidation in a follow up PR, I think this is indeed ready to merge. |
|
OK, merged this! Will open a follow-up to consolidate in a single file, and update #57 |
Based off #49