conda-forge / fiona-feedstock

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

Fix dependencies and add pip check #220

Closed xylar closed 10 months ago

xylar commented 10 months ago

Add xylar as a maintainer

Checklist

conda-forge-webservices[bot] commented 10 months 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 10 months ago

@conda-forge/fiona, please forgive me if this PR seems unfriendly. I noticed in a downstream PR: https://github.com/conda-forge/constructive_geometries-feedstock/pull/5 that our pip check was failing because of a missing certifi dependency. My initial intention was just to fix that but it seems like a lot of dependencies are out of sync with the upstream specifications. It seems worth adding pip check.

xylar commented 10 months ago

@conda-forge-admin, please rerender

akrherz commented 10 months ago

thanks @xylar , should these warnings be cleaned up then?

 WARNING (fiona): interpreted library (Python) package conda-forge::certifi-2023.11.17-pyhd8ed1ab_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
 WARNING (fiona): interpreter (Python) package conda-forge::python-3.12.0-hab00c5b_0_cpython in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
 WARNING (fiona): run-exports library package conda-forge::numpy-1.26.2-py312heda63a1_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
 WARNING (fiona): interpreted library (Python) package conda-forge::setuptools-68.2.2-pyhd8ed1ab_0 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
xylar commented 10 months ago

@akrherz, that does seem odd. Upstream certainly seems to suggest that all of these are build dependencies. Any idea why we're getting these warnings?

xylar commented 10 months ago

We're also getting some deprecation warnings:

DEPRECATION: --build-option and --global-option are deprecated. pip 24.0 will enforce this behaviour change. A possible replacement is to use --config-settings. Discussion can be found at https://github.com/pypa/pip/issues/11859
WARNING: Implying --no-binary=:all: due to the presence of --build-option / --global-option.

So it might be worth updating build.sh to more closely match https://fiona.readthedocs.io/en/stable/install.html#advanced-installation

xylar commented 10 months ago

I'm mistaken about certifi. It's not a build dependency so I took it out. I find the warnings confusing for the others because they're not primarily libraries that get linked, they're tools that may be involved in the build. So the warning seems misguided to me but I will freely admin that I'm not super expert in these warnings.

xylar commented 10 months ago

@conda-forge-admin, please rerender

xylar commented 10 months ago

Funny how fixing pip check turns into a whole can of worms...

xylar commented 10 months ago

Hmm, I don't think adding ignore_run_exports is a good idea. I don't really understand the 3 warnings for numpy, python and setuptools (see https://github.com/conda-forge/fiona-feedstock/pull/220#issuecomment-1829049150 above) but ignoring them seems safer to me than any solution I can come up with to suppress them.

@akrherz, please let me know what you'd suggest. I need to let this be for now.

akrherz commented 10 months ago

please let me know what you'd suggest.

I am unsure, sorry :(

xylar commented 10 months ago

@akrherz, next question is if you know whether these warnings are new. If they are not or we don't know, I would suggest we let them be unless someone thinks they're harmful or risky.

jorisvandenbossche commented 10 months ago

I don't know the exact state of using certifi in conda-forge, but so this was explicitly removed a while ago (well, restricted to older python versions, and now it could be removed): https://github.com/conda-forge/fiona-feedstock/pull/188

xylar commented 10 months ago

@jorisvandenbossche, okay, but then pip check fails because upstream requires certifi for all python versions. I'm fine with removing it but then we should patch to remove it from the pyproject.toml, too, so pip check can pass.

xylar commented 10 months ago

Thanks for pointing me to the context. I tried to look around for it but didn't find that discussion.

xylar commented 10 months ago

@conda-forge/fiona, I think this is ready for review. @akrherz and @jorisvandenbossche, please let me know if your concerns have been addressed, if others come up, and if there are changes you'd still like to see.

xylar commented 10 months ago

@jorisvandenbossche, do you have further comments?

xylar commented 10 months ago

Anyone up for merging this one? It seems like the specific concerns have been addressed.