Skip to content
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

Add gdal 3.5 and 3.6 to CI, update proj/gdal install scripts to use cmake, update drvsupport #1122

Merged
merged 28 commits into from
Dec 8, 2022

Conversation

rbuffat
Copy link
Contributor

@rbuffat rbuffat commented Jul 6, 2022

@rbuffat rbuffat marked this pull request as draft July 6, 2022 20:05
@mwtoews
Copy link
Member

mwtoews commented Jul 6, 2022

This pinned version of munch is incompatible with Python 3.11:

munch==2.3.2

not sure which one to pick, but there are several newer versions

@rbuffat rbuffat changed the title Enable append support for FlatGeobuf Add gdal 3.5 to CI, update proj/gdal install scripts to use cmake, update drvsupport Jul 8, 2022
@rbuffat
Copy link
Contributor Author

rbuffat commented Jul 8, 2022

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
Copy link
Contributor Author

rbuffat commented Jul 8, 2022

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 rbuffat marked this pull request as ready for review July 8, 2022 16:29
@sgillies
Copy link
Member

sgillies commented Aug 1, 2022

@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"?

@rbuffat
Copy link
Contributor Author

rbuffat commented Oct 12, 2022

@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
Copy link
Contributor Author

rbuffat commented Oct 12, 2022

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

@rbuffat rbuffat marked this pull request as draft October 12, 2022 18:56
@rbuffat
Copy link
Contributor Author

rbuffat commented Oct 19, 2022

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 rbuffat marked this pull request as ready for review November 18, 2022 12:47
@rbuffat rbuffat changed the title Add gdal 3.5 to CI, update proj/gdal install scripts to use cmake, update drvsupport Add gdal 3.5 and 3.6 to CI, update proj/gdal install scripts to use cmake, update drvsupport Nov 18, 2022
@rbuffat
Copy link
Contributor Author

rbuffat commented Nov 18, 2022

@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?

@@ -56,7 +56,7 @@
# GeoJSON GeoJSON Yes Yes Yes
("GeoJSON", "raw"),
# GeoJSONSeq GeoJSON sequences Yes Yes Yes
("GeoJSONSeq", "rw"),
("GeoJSONSeq", "raw"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"raw" only since GDAL 3.6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is limited in the driver_mode_mingdal dict:

'GeoJSONSeq': (3, 6, 0),

@zklaus
Copy link

zklaus commented Dec 2, 2022

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

@rbuffat
Copy link
Contributor Author

rbuffat commented Dec 4, 2022

@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)

@@ -8,6 +8,7 @@ filterwarnings =
markers =
iconv: marks tests that require gdal to be compiled with iconv
network: marks tests that require a network connection
wheel: marks test that only works when installed from wheel
wheel: marks tests that only works when installed from wheel
gdal: marks tests that are only dependent on GDAL functionality (e.g. for drvsupport)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@sgillies sgillies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rbuffat thank you!

@sgillies sgillies merged commit 6ca3f8f into Toblerity:maint-1.9 Dec 8, 2022
sgillies added a commit that referenced this pull request Dec 8, 2022
@rbuffat
Copy link
Contributor Author

rbuffat commented Dec 9, 2022

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

@sgillies
Copy link
Member

sgillies commented Dec 9, 2022

@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.

@zklaus
Copy link

zklaus commented Dec 9, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants