-
Notifications
You must be signed in to change notification settings - Fork 18
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: Fix MacOS vcpkg libgeotiff build #69
Conversation
Something about the update of libgeotiff from Contents of
|
I can't directly help with the build failure on Mac (I would maybe report the build failure upstream at https://github.com/microsoft/vcpkg, since it worked before; it's a bit a pity that the dynamic osx build it not one of the built-in triplets, which would make it less likely to get broken). But, maybe we should try to pin the build to an exact "baseline" for which we know it works (https://devblogs.microsoft.com/cppblog/take-control-of-your-vcpkg-dependencies-with-versioning-support/#baselines), and then only from time to time manually update the version. |
This should now be using the vcpkg manifest approach for all builds, which includes a baseline commit in the vcpkg repository (set to just before libgeotiff 1.7.1 was merged), and a pinned version of libgeotiff that works. We'll occasionally need to bump this version / baseline commit. The instructions weren't particularly clear; given the basline that I set, it shouldn't have tried to install libgeotiff 1.7.1 at all, but it did so until I added an override to pin that to a prior version. I couldn't reproduce the libgeotiff error locally, so hopefully that will be fixed at a later point. I'm not seeing anything obvious to call attention to in the vcpkg setup for it, so I'm not sure what we would log out there as an issue other than it doesn't build in our specific environment. |
Since this worked on last test of building the wheels, I'm going to go ahead and merge this so we can get closer to cutting an 0.4.0 release that builds wheels. We can add future PRs to fix things here if needed. |
Sorry for the late review here, and thanks a lot for fixing this! Adding a few questions inline. |
In any case it is a nice solution! |
Until it broke on CI, at time of merge. Because CI is... fun... (investigating now, further fixes will be a in a different PR) |
run: | | ||
vcpkg install gdal[core] --overlay-triplets=./ci/custom-triplets | ||
echo "VCPKG_INSTALLATION_ROOT: ${VCPKG_INSTALLATION_ROOT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a debugging left-over? (the variable is documented by github actions about what it is)
--overlay-ports=opt/vcpkg/custom-ports \ | ||
--feature-flags="versions,manifests" \ | ||
--x-manifest-root=opt/vcpkg \ | ||
--x-install-root=opt/vcpkg/installed && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: the reason that it is now needed to mention the install-root, is that because using manifest mode changes that? (it would now try to install inside the project, instead of in the vcpkg root?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, using manifest mode changes this, and would have put it in ci/vcpkg_installed/
which then would have required other updates here to use that path.
"overrides": [ | ||
{ | ||
"name": "libgeotiff", | ||
"version": "1.7.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected that only the baseline would be sufficient? (assuming the baseline is from a commit where libgeotiff is still at 1.7.0?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided I did it correctly (I picked commit immediately prior to 1.7.0 being merged), I would have expected the same. However, I found it went ahead and tried to install 1.7.1 anyway, without setting this override.
I don't know that it is necessarily a problem for us to explicitly list it as a pinned version though - if only for documentation purposes, since this was the root of the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that it is necessarily a problem for us to explicitly list it as a pinned version though - if only for documentation purposes, since this was the root of the problem.
Indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorisvandenbossche @brendan-ward I am finding vcpkg manifests difficult to use. Do you know if there is a vcpkg mode that will fail if no vcpkg.json is found? I suspect it's continuing without one in my case, but can't verify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgillies I think you are missing a "manifests" feature flag. Assuming this is about rasterio/rasterio-wheels#90, will comment over there
But the Mac build passed :) |
The GDAL data files probably changed again location on windows .. |
No description provided.