[3.6.x] Fix deactivate.sh

conda-forge / gdal-feedstock

A conda-smithy repository for gdal.

BSD 3-Clause "New" or "Revised" License

30 stars 63 forks source link

[3.6.x] Fix deactivate.sh #788

Closed xylar closed 1 year ago

xylar commented 1 year ago

Checklist

[x] Used a personal fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[x] Ensured the license file is being packaged.

conda-forge-webservices[bot] commented 1 year 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.

xylar commented 1 year ago

@conda-forge-admin, please rerender

xylar commented 1 year ago

Same here, feel free to merge once CI finishes unless there's another typo I missed.

github-actions[bot] commented 1 year ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/gdal-feedstock/actions/runs/5605025687.

rouault commented 1 year ago

Aren't those scripts tested by CI ? Perhaps a set -eu would be needed in them ?

xylar commented 1 year ago

@rouault, I'm not sure about a set -eu. That's certainly not a standard practice on conda-forge and I thought I had the sense that was a bad idea when you source a script as opposed to run it.

Aren't those scripts tested by CI ?

The activate scripts are certainly tested. The deactivate scripts maybe not or any errors they produce may simply be ignored, not sure about that. I guess that's where your set -eu would come in. I'll leave it to folks with more shell knowledge to decide if that can be added.

xylar commented 1 year ago

So looking into this further, I do think set -eu in a sourced script will cause the shell to exit completely when there's an error, see https://stackoverflow.com/a/24460931/7728169. So it doesn't seem like that's something we can add.

Anyone have any other input on this? Otherwise, I'd like to merge soon because this bug is likely to break deactivation for folks using this version of gdal.

rouault commented 1 year ago

Anyone have any other input on this?

Please merge the fix. And thanks for your valid points on why set -eu wouldn't be appropriate

xylar commented 1 year ago

Thanks @rouault