conda-forge / fiona-feedstock

A conda-smithy repository for fiona.
BSD 3-Clause "New" or "Revised" License
8 stars 16 forks source link

Rebuild for gdal33 #179

Closed regro-cf-autotick-bot closed 3 years ago

regro-cf-autotick-bot commented 3 years ago

This PR has been triggered in an effort to update gdal33.

Notes and instructions for merging this PR:

  1. Please merge the PR only after the tests have passed.
  2. Feel free to push to the bot's branch to update this PR if needed.

Please note that if you close this PR we presume that the feedstock has been rebuilt, so if you are going to perform the rebuild yourself don't close this PR until the your rebuild has been merged.

This package has the following downstream children:

And potentially more.

If this PR was opened in error or needs to be updated please add the bot-rerun label to this PR. The bot will close this PR and schedule another one. If you do not have permissions to add this label, you can use the phrase code>@<space/conda-forge-admin, please rerun bot in a PR comment to have the conda-forge-admin add it for you.

This PR was created by the regro-cf-autotick-bot. The regro-cf-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. If you would like a local version of this bot, you might consider using rever. Rever is a tool for automating software releases and forms the backbone of the bot's conda-forge PRing capability. Rever is both conda (conda install -c conda-forge rever) and pip (pip install re-ver) installable. Finally, feel free to drop us a line if there are any issues! This PR was generated by https://github.com/regro/autotick-bot/actions/runs/868193078, please use this URL for debugging

conda-forge-linter commented 3 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

weiji14 commented 3 years ago

Builds are failing on this error:

fiona.errors.DriverError: Driver GTM is considered for removal in GDAL 3.5. You are invited to convert any dataset in that format to another more common one .If you need this driver in future GDAL versions, create a ticket at https://github.com/OSGeo/gdal (look first for an existing one first) to explain how critical it is for you (but the GDAL project may still remove it), and to enable it now, set the GDAL_ENABLE_DEPRECATED_DRIVER_GTM configuration option / environment variable to YES

This is to do with GDAL deprecating the GPSTrackMaker (GTM) format (see https://lists.osgeo.org/pipermail/gdal-dev/2021-March/053590.html). Related to upstream issue at https://github.com/OSGeo/gdal/issues/3876. Should the GDAL_ENABLE_DEPRECATED_DRIVER_GTM config be set to YES so that the conda-build can proceed, or should this be handled upstream at https://github.com/Toblerity/Fiona?

ocefpaf commented 3 years ago

Failing tests on Linux:

tests/test_datetime.py::test_datefield[GPSTrackMaker-datetime] FAILED
tests/test_datetime.py::test_datefield_null[GPSTrackMaker-datetime] FAILED
tests/test_drvsupport.py::test_write_or_driver_error[GPSTrackMaker] FAILED
tests/test_drvsupport.py::test_append_or_driver_error[DGN] FAILED
tests/test_drvsupport.py::test_no_append_driver_cannot_append[GPSTrackMaker] FAILED
tests/test_drvsupport.py::test_no_append_driver_cannot_append[PCIDSK] FAILED
ocefpaf commented 3 years ago

@weiji14 for some reason your old comment did not appear to me when I added mine above!

I'm not sure we should rebuild gdal with that option at this point. Fiona is fine with older gdal and we don't really need it to be ready for latest one ASAP. We can wait for upstream to fix it. Or, we can just ignore those errors. What would be the damage in that case?

weiji14 commented 3 years ago

I'm not sure we should rebuild gdal with that option at this point. Fiona is fine with older gdal and we don't really need it to be ready for latest one ASAP. We can wait for upstream to fix it. Or, we can just ignore those errors. What would be the damage in that case?

Probably best to leave gdal as it is. My original idea was to set environment variable GDAL_ENABLE_DEPRECATED_DRIVER_GTM=YES in the fiona-feedstock build's test step, but not sure if that's possible? I'm not sure what the downstream effects would be, I assume that people using the GPSTrackMaker (GTM) would encounter the GDAL deprecation error and complain to GDAL accordingly and/or downgrade to GDAL 3.2 as a workaround.

weiji14 commented 3 years ago

I'm not sure we should rebuild gdal with that option at this point. Fiona is fine with older gdal and we don't really need it to be ready for latest one ASAP. We can wait for upstream to fix it. Or, we can just ignore those errors. What would be the damage in that case?

Probably best to leave gdal as it is. My original idea was to set environment variable GDAL_ENABLE_DEPRECATED_DRIVER_GTM=YES in the fiona-feedstock build's test step, but not sure if that's possible? I'm not sure what the downstream effects would be, I assume that people using the GPSTrackMaker (GTM) would encounter the GDAL deprecation error and complain to GDAL accordingly and/or downgrade to GDAL 3.2 as a workaround.

Gentle nudge. Can the environment variable be set somewhere at these lines in recipe/meta.yaml?

https://github.com/conda-forge/fiona-feedstock/blob/f4917945cae18326649045d150400345544bc2f7/recipe/meta.yaml#L62-L65

I know that export GDAL_ENABLE_DEPRECATED_DRIVER_GTM=YES can be used for Linux, but unsure of how to make it work cross-platform for Windows too. (interesting to note though that the Windows tests are all passing even without this config).

kyri-petrou commented 3 years ago

I've opened an issue earlier regarding Fiona 1.8.20 not being compatible with gdal33, but just came across this PR.

If you wish to set environment variables in the build, I believe that this can be done this way: https://docs.conda.io/projects/conda-build/en/latest/user-guide/environment-variables.html#inherited-environment-variables

kyri-petrou commented 3 years ago

Update on this:

I've created a local build using linux64 and Python 3.9 adding the following to recipe/meta.yaml and it fixes the GTM driver issue.

build:
  script_env:
    - GDAL_ENABLE_DEPRECATED_DRIVER_GTM=YES

Also, I believe this should be okay to use since they are also using this envvar in the Fiona CI/CD pipeline upstream.

However, there is one other test failing when building with libgdal 3.3+ that was not spotted earlier. I believe that the solution to this would be change the mode of the PCIDSK to "raw" from "rw" here. Should we take this issue upstream?

jorisvandenbossche commented 3 years ago

Should we take this issue upstream?

Yes, that would be welcome.

I will push your suggestion to this PR. Thanks for looking into it!

jorisvandenbossche commented 3 years ago

@conda-forge-admin, please rerender

jorisvandenbossche commented 3 years ago

Indeed, the one linux build I checked now only has the test_no_append_driver_cannot_append[PCIDSK] test failing, as you mentioned.

I am not sure if we could simply skip that test here for now?

kyri-petrou commented 3 years ago

Indeed, the one linux build I checked now only has the test_no_append_driver_cannot_append[PCIDSK] test failing, as you mentioned.

I am not sure if we could simply skip that test here for now?

It's possible to skip this test alone by adding -k "not test_no_append_driver_cannot_append[PCIDSK]" to the pytest command (e.g., pytest -k "not test_no_append_driver_cannot_append[PCIDSK]"), but I'm not sure where can we add this

jorisvandenbossche commented 3 years ago

Yes, that can be added in the run_test.sh/bat, there are already a bunch of other tests we are skipping as well.

So my question was mainly: is it OK to skip this and upload the package without "fixing" it (but didn't really look into the actual failing case, so not sure there is something that needs to be "fixed")

kyri-petrou commented 3 years ago

Good question. I think that skipping this test is likely okay until the issue is "fixed" upstream. My understanding of the issue is that the PCIDSK driver now also supports append mode in libgdal 3.3 (as well as read and write). The test indicates that Fiona is only allowing read and write. I would speculate that the test was put in place so that any new upstream GDAL driver features / changes are quickly adopted/fixed.

I don't see much of a harm in skipping the test for now, but I wouldn't swear on it either

jorisvandenbossche commented 3 years ago

My understanding of the issue is that the PCIDSK driver now also supports append mode in libgdal 3.3 (as well as read and write).

If that's the case, then yes skipping the test should indeed be fine. Will push a commit for that.

kyri-petrou commented 3 years ago

The OSX Python 3.9 build seems to have failed, but the log seems to be completely empty for the build. Perhaps a glitch in the pipeline? I think retriggering the pipeline might sort it out

ocefpaf commented 3 years ago

Let's try to restart it.

@conda-forge-admin, please restart ci

kyri-petrou commented 3 years ago

Looks like it worked!