Skip to content

Conversation

@regro-cf-autotick-bot
Copy link
Contributor

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

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
Copy link
Member

weiji14 commented May 25, 2021

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 OSGeo/gdal#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
Copy link
Member

ocefpaf commented May 25, 2021

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
Copy link
Member

ocefpaf commented May 26, 2021

@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
Copy link
Member

weiji14 commented May 26, 2021

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
Copy link
Member

weiji14 commented Jun 2, 2021

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?

commands:
- fio --help
- fio ls test_data/test.shp
- fio info test_data/test.shp

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

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

kyri-petrou commented Jun 14, 2021

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
Copy link
Member

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
Copy link
Member

@conda-forge-admin, please rerender

@jorisvandenbossche
Copy link
Member

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

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
Copy link
Member

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

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
Copy link
Member

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

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
Copy link
Member

ocefpaf commented Jun 14, 2021

Let's try to restart it.

@conda-forge-admin, please restart ci

@kyri-petrou
Copy link

Looks like it worked!

@ocefpaf ocefpaf merged commit f0a93cf into conda-forge:master Jun 14, 2021
@regro-cf-autotick-bot regro-cf-autotick-bot deleted the rebuild-gdal33-0-1_hee1323 branch June 14, 2021 22:38
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.

6 participants