Fiona icon indicating copy to clipboard operation
Fiona copied to clipboard

Add gdal 3.5 to CI, update proj/gdal install scripts to use cmake, update drvsupport

Open rbuffat opened this issue 2 years ago • 4 comments

See https://github.com/Toblerity/Fiona/issues/1099#issuecomment-1176004721

rbuffat avatar Jul 06 '22 19:07 rbuffat

This pinned version of munch is incompatible with Python 3.11: https://github.com/Toblerity/Fiona/blob/9d06389755928a3ae4ffa55d4156700f5260ec1c/requirements.txt#L5 not sure which one to pick, but there are several newer versions

mwtoews avatar Jul 06 '22 21:07 mwtoews

Looks like I opened a can of worms.

This PR:

  • adds GDAL 3.5 to the CI matrix.
  • updates install scripts to use cmake. This is necessary for Proj 9, and will be necessary for GDAL 3.6 (which is on master already).
  • Update drvsupport: flatgeobuf gained support to append with gdal 3.5, openfilegdb will gain support for write and append with gdal 3.6

The tests testing GDAL functionality (e.g. read / write / append support of drivers or support of drivers for time / datetime) are now marked with @pytest.mark.gdal. These tests should only be executed when adding the --test_gdal flag to pytest.

Thanks @mwtoews for having a look at this PR.

rbuffat avatar Jul 08 '22 16:07 rbuffat

There are still some regressions:

The cmake configuration step emits the following warning and I'm not exactly sure why:

CMake Warning:
  Ignoring extra path from command line:
   "/"

The FileGDB driver is not included with the cmake install script. But as gdal 3.6 seems to support writing GDB files with the OpenFileGDB driver this seems not be that important.

rbuffat avatar Jul 08 '22 16:07 rbuffat

@rbuffat can we do without the --test_gdal option? Is there anything about it that isn't possible with pytest -m gdal" or pytest -m "not gdal"?

sgillies avatar Aug 01 '22 21:08 sgillies

@sgillies Sorry I forgot about this PR.

@rbuffat can we do without the --test_gdal option? Is there anything about it that isn't possible with pytest -m gdal" or pytest -m "not gdal"?

What I intended to implement is that the gdal marked tests are disabled by default and must be enabled manually. So that we can enable it on our CI and we can avoid situations where others have issues with them when they fail on exotic platforms. Based on https://stackoverflow.com/questions/66315234/exclude-some-tests-by-default it should work also without an additional command line argument. Lets see if this works.

rbuffat avatar Oct 12 '22 18:10 rbuffat

That seems not to work as intended. I will have a look at it later.

rbuffat avatar Oct 12 '22 18:10 rbuffat

With the current changes, when pytest is run using pytest -m "not wheel" ... or pytest ... (not explicitly including the gdal marker) all tests marked with @pytest.mark.gdal are skipped:

tests/test_datetime.py ...sssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss.                                                                                                                             
tests/test_drvsupport.py ssssssssssssssssssssssssssssssssssssssssssssssssss. 

When using pytest -m "not wheel or gdal" (explicitly including the gdal marker), these tests are not skipped and are included:

tests/test_datetime.py .................................................................................................                                                                                                                             
tests/test_drvsupport.py ...............................x...................           

@sgillies I'm not sure this is the way this should be done. It looks too complicated, which often indicates that something is done wrong. I will look at it again when I have more brain capacity left. Or what is your opinion, should we avoid meddling with the markers in conftest.py/pytest_collection_modifyitems?

rbuffat avatar Oct 19 '22 19:10 rbuffat

@sgillies This PR should now be ready for a review.

  • If you don't like the changes in conftest.py/pytest_collection_modifyitems, let's just remove them.
  • pytest.mark.gdal is not the best name, maybe pytest.mark.ci_only would be better?

rbuffat avatar Nov 18 '22 12:11 rbuffat

@rbuffat, friendly ping. Is this something you see moving forward soon?

zklaus avatar Dec 02 '22 08:12 zklaus

@zklaus The PR is ready to be reviewed. It is currently only for the 1.9 branch, but the same changes should also apply for the 1.8 branch.

This PR does not change the functionality of Fiona (expect that without it Fiona limits some driver capabilities that GDAL gained with version 3.6)

rbuffat avatar Dec 04 '22 21:12 rbuffat

@sgillies should this be backported to maint-1.8 for a 1.8.23 release?

rbuffat avatar Dec 09 '22 12:12 rbuffat

@rbuffat if people use Fiona 1.8.22 with 3.6.0 they will see a few failing tests, but no breakage, right? I'm inclined to not backport and not make a new release.

sgillies avatar Dec 09 '22 14:12 sgillies

Fyi, I have effectively backported this in conda-forge/fiona-feedstock#203

zklaus avatar Dec 09 '22 22:12 zklaus