Build wheels for Windows with vcpkg and cibuildwheel on github actions

[jorisvandenbossche]

See #48. This is a first attempt to put that workflow in a github action.

[jorisvandenbossche]

Updated this to copy in the GDAL/PROJ data, and the tests are passing!!

I think we don't have a test where we actually require GDAL_DATA (I think we need to test some other file format for this), but for sure the PROJ database was found.

[brendan-ward]

Added direct tests for GDAL/PROJ data in #53 , which I think is likely our best way to have tests that will fail if those are not available. Eventually we may want to add an indirect test using a format that specifically requires GDAL data, but I'm not yet clear on when that is actually used.

Thanks for all your work on this - super exciting that wheels build and tests pass on Windows!

[jorisvandenbossche]

Eventually we may want to add an indirect test using a format that specifically requires GDAL data, but I'm not yet clear on when that is actually used.

I checked in gdal/ogr/ogrsf_frmts for CPLFindFile, and then you can see some example of formats that use (in a certain case) one of the data files: https://github.com/search?q=CPLFindFile+repo%3AOSGeo%2Fgdal+path%3Aogr%2Fogrsf_frmts&type=Code&ref=advsearch&l=&l=
For example writing the DXF format requires it (that's the file we check to see if we correctly found the data directory). DXF is built-in by default (https://gdal.org/drivers/vector/index.html), and so we should be able to write to it (whether this makes sense for tabular data, no idea though)

[jorisvandenbossche]

Updated the PR to builds wheels for Python 3.8 - 3.10.
The latest version can be downloaded from https://github.com/geopandas/pyogrio/suites/5620607087/artifacts/183019252

Do we also want 32bit wheels for windows? (x86) It should be possible since both vcpkg and cibuildwheel support that, but no idea how important this still is nowadays.

[jorisvandenbossche]

I didn't directly find a way to use an env variable here, so this -add-path is currently hardcoded (in practice, I don't think it will be common that people use this config to build wheels locally using cibuildwheel, so that's probably not such a problem)

[brendan-ward]

This seems fine.

[martinfleis]

Do we also want 32bit wheels for windows?

Since Windows 11 has no 32-bit support, I wouldn't bother. It will be a very fringe case now.

[brendan-ward]

This looks great pending a few comments - thank you so much for working on this @jorisvandenbossche !

This is a huge step forward and far simpler than more manual builds of GDAL on Windows.

[brendan-ward]

Suggested change

	name: Build wheels
	name: Build Windows wheels

suggestion: also change filename to build_windows_wheels.yml; I think even if we can use much of this same approach for MacOS / Linux, that we will probably want to keep them in separate files & top-level builds.

[brendan-ward]

I think these will fail if GDAL is a valid install (e.g., on MacOS / Linux) , but env vars are not set?

Suggest a little restructuring here, and raise exceptions if the paths are not found: PYOGRIO_PACKAGE_DATA signals intent that we expect to package these, so we should hard fail if we can't.

Untested code:

gdal_data = os.environ.get("GDAL_DATA")
if not gdal_data:
    gdal_data = find_gdal_data()
if not gdal_data and os.path.exists(gdal_data):
    raise Exception("Could not find GDAL data files for packaging")

proj_data = os.environ.get("PROJ_LIB")
if not proj_data:
    proj_data = find_proj_data()
if not proj_data and os.path.exists(proj_data):
    raise Exception("Could not find PROJ data files for packaging")    

log.info(f"Copying GDAL data from {gdal_data} and PROJ data from {proj_data}")

copy_data_tree(gdal_data, "pyogrio/gdal_data")
copy_data_tree(proj_data, "pyogrio/proj_data")
package_data = {"pyogrio": ["gdal_data/*", "proj_data/*"]}


def find_gdal_data():
    gdal_config = os.environ.get("GDAL_CONFIG", "gdal-config")
    return read_response([gdal_config, "--datadir"])
   
def find_proj_data():
    return read_response(['projinfo', "--searchpaths"]).split('\n')[-1]

find_proj_data is arguably less robost (find_gdal_data reuses things we already check), so it is reasonable to expect us to set PROJ_LIB on platforms where that is in a nonstandard location or not easily found.

Alternatively, if you find that falling back to finding them too complex, it is not unreasonable for us to insist that GDAL_DATA and PROJ_LIB be set in order to create wheels.

[jorisvandenbossche]

We could also leave this extra logic until it is needed / requested? Right now, we only use this for wheel building and on windows, in which case those env variables are always set. Even for wheel building on linux/mac, it might be that we end up defining those variables.

I suppose this would mostly be for external people who would want to build wheels themselves? (although I would say that it doesn't seem such a burden to ask them to define those variables, if they want to include data. They would need to set the PYOGRIO_PACKAGE_DATA flag anyway)

But I can certainly change it to error if they are not set.

[brendan-ward]

We could also leave this extra logic until it is needed / requested?

It is fine to leave it for now.

[jorisvandenbossche]

@brendan-ward do you have more comments here? Otherwise I would propose to merge this, and then I can update the other PR based on those changes

[martinfleis]

Let me grab my win machine for a quick test.

[brendan-ward]

I don't have a way of testing these directly, but otherwise things look good, so feel free to merge when ready. We can always iterate further as we splice in the MacOS / Linux builds...

[martinfleis]

The wheels seem to work on my side! Great job!