CI: Fix vcpkg Windows and Linux builds

[brendan-ward]

No description provided.

[brendan-ward]

Looks like Windows may not be installing packages in the way we expect in manifest mode.
Trying to vcpkg list them returns nothing: https://github.com/geopandas/pyogrio/runs/6102618748?check_suite_focus=true#step:5:630

Presumably we didn't hit this before was because we were using a vcpkg cache created from before enabling manifest mode.

[brendan-ward]

And now our linux wheels are failing because they get assigned an incorrect filename: pyogrio-0+unknown-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl instead of pyogrio-0.3.0+47.g9dd1b49-cp38-cp38-linux_x86_64.whl, so installing them in our tests falls back to the sdist instead of the binary wheel.

The only things that changed for linux was enabling manifest mode for vcpkg, which should not have affected wheel names. Madness!

[jorisvandenbossche]

And now our linux wheels are failing because they get assigned an incorrect filename:

Looking at the manylinux images, the most recent change (pypa/manylinux#1308) updated some dependencies, which also included an update of setuptools. That might have changed something in how it is (not) picking up the version for the name tag.

You fixed this for now by pinning to a previous manylinux docker build?

[jorisvandenbossche]

Actually, cibuildwheel still installs setuptools in a build environment itself, so I don't fully understand how that is influenced by the docker image ..

But in any case, comparing the logs of the succeeded build of a week ago, the failing build in this PR, and then the last one that succeeded again in this PR, a noticeable difference is that the failing one is installing setuptools 62.0.0, while the working builds are using 61.1.0.
And in those working builds you see a "setuptools/dist.py:516: UserWarning: Normalizing 'v0.3.0+52.g4b7433a' to '0.3.0+52.g4b7433a' " warning, which is no longer present in the failing build (and then it is using the unkonwn version instead).

[jorisvandenbossche]

I tested locally to build pyogrio (but without using the docker image) and both setuptools 61.1 and 62.0 work fine and correctly find the version. So the setuptools change in version is probably unrelated, and it's still something else that changed in the docker image that triggered this.

[jorisvandenbossche]

So for the Windows side, I didn't comment about that yet, but I pushed a small commit just to test the hypothesis that it were the way of using ${..} instead of $.. for the env variable that might not be working (I know that I struggled before to understand how you need to specify env variables in the different places (because in some places in the github actions yaml you also need to do {{ env.VAR }} instead ..).

It seems to have worked.

Maybe we should also specify the default "shell"? Because on Windows that should be supposed to be powershell and not bash, and actually according to the docs we would then need to use $env:VAR instead of $VAR ... (https://docs.github.com/en/actions/learn-github-actions/environment-variables#about-environment-variables)

[brendan-ward]

Thanks for testing that locally, and for trying another test with unquoted variable. Since that used a pre-existing cache with vcpkg stuff installed in the default location, we needed to test that again without a cache. Which unfortunately looks like it didn't work because VCPKG_INSTALLATION_ROOT now seems to be pointing at D drive. But maybe we aren't using that properly (lacking the $env. prefix) - even though this seems to have worked before??

[jorisvandenbossche]

Since that used a pre-existing cache with vcpkg stuff installed in the default location, we needed to test that again without a cache

Ah, so the reason it seemed to work is because it did install GDAL (but in some other location), and then the vcpkg list listed the old cached version ..

[jorisvandenbossche]

because VCPKG_INSTALLATION_ROOT now seems to be pointing at D drive. But maybe we aren't using that properly (lacking the $env. prefix)

Yes, I think that is indeed the cause, the $VAR gets ignored and so basically it was only /installed, which seems to be interpreted as on D:

even though this seems to have worked before??

Before #69, we actually never used an env variable inside a run: section of the workflow, so I think that's the reason it seems this way

[jorisvandenbossche]

With powershel syntax it seems to work! Out of curiosity, I also pushed a commit to see if it would work with using bash shell on windows

[jorisvandenbossche]

Actually, we should probably also have changes the cache key for every test?

[brendan-ward]

bash shell on windows

I can't tell if this worked? Looks like ls failed but logs are a little jumbled...

we should probably also have changes the cache key for every test

Yeah, we probably should just disable cache when we are testing build locations. Once those are stable, we can re-enable it. Will disable now and try again without failing ls.

[brendan-ward]

I added the asset paths with hardcoded links because they didn't work properly when using the $VCPKG_INSTALLATION_ROOT form, but I don't see a big downside to having them hardcoded regardless.

[brendan-ward]

This and BUILDKIT_PROGRESS were added in attempt to get the Docker build to emit more logs during build, so we can see what vcpkg packages get installed. I'm not sure if both are needed, but didn't want to invest the build time in trying to game these.

[brendan-ward]

All builds are passing again! Partly due to my ignorance and partly due to long feedback loops, the programmer time per line of code ratio on this one is really out of wack.

A few takeaways:

• using bash shell on Windows allowed us to use --x-install-root=$VCPKG_INSTALLATION_ROOT/installed flag for vcpkg without running into issues or needing to hard-code the absolute, windows specific paths in matrix variables
• hard-coding absolute paths for log files and cache folders allows us to deal with those correctly for different OS's; otherwise these didn't generalize nicely and likely weren't working properly before

[jorisvandenbossche]

hard-coding absolute paths for log files and cache folders allows us to deal with those correctly for different OS's; otherwise these didn't generalize nicely and likely weren't working properly before

Since the cache did work in the past, I suppose those paths have been working somehow?
But anyway, the current solution also looks good :)