conda-forge / wetterdienst-feedstock

A conda-smithy repository for wetterdienst.
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

Update list of runtime dependencies in `recipe/meta.yml` #71

Closed amotl closed 1 year ago

amotl commented 1 year ago

This patch just follows the suggestions by regro-cf-autotick-bot about packages found by source code inspection but not in the meta.yaml vs. packages found in the meta.yaml but not found by source code inspection.

It addresses our discussion at:

conda-forge-linter 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

@amotl, I wouldn't make these changes even though the bot suggested them. It is important that the dependencies are the same as in the original package: https://github.com/earthobservations/wetterdienst/blob/v0.48.0/pyproject.toml#L91-L117 If there are missing dependencies, you need to report them to the developers rather than just changing them in the conda-forge recipe. Sometimes the "missing" dependencies are only in tests or code for development, not the package for general use.

amotl commented 1 year ago

Dear @xylar,

thanks for looking into this. Indeed, most of the added dependencies are optional dependencies ^1. Is there any way to specify them within the Anaconda metadata file?

Otherwise, is the general recommendation to not include them into the baseline meta.yml, and let the user select additional/optional packages instead?

With kind regards, Andreas.

xylar commented 1 year ago

@amotl, yes, there is a way to handle optional dependencies. It's a little complicated. I'll send you an example tomorrow and we can discuss further.

amotl commented 1 year ago

Thanks. I've just added e7e2e945 to this patch, which updates the list of dependencies to be more sensible as a default choice. If @gutzbenj agrees, I think it will be good to go, without the need to overengineer things.

xylar commented 1 year ago

Okay here's the example https://github.com/conda-forge/airflow-feedstock/blob/main/recipe/meta.yaml The extras are separate packages that include the base package and are of the form <main>-with-<extra>. This will only work well if extras are grouped together. We don't want a gagillion additional packages. It's only worth it if users really want these extras.

amotl commented 1 year ago

I see, thank you. Based on this information, I think it is not worth to add additional maintenance overhead to the wetterdienst-feedstock package.

How would we proceed with this patch, then? Cherry-pick it into #70? I will be happy about any advises.

xylar commented 1 year ago

I will look at this tomorrow but even if these are the right dependencies, they are missing version constraints from the original package and they are very important to include. I will make suggestions to #70 and ask for your feedback.

amotl commented 1 year ago

I will make suggestions to https://github.com/conda-forge/wetterdienst-feedstock/pull/70 and ask for your feedback.

Apologies. I've just merged the release PR, in order to get the release out of the door, without mixing in unrelated changes.

I will look at this tomorrow but even if these are the right dependencies, they are missing version constraints from the original package and they are very important to include.

Ah - version pinning for Anaconda? I will be happy to learn more about writing a better meta.yml. Feel free to submit your suggestions as a patch right away. Thank you already!

xylar commented 1 year ago

@amotl, hmm, I wish you hadn't merged that yet. The dependencies are not correct in #70 and need to be fixed. That is a lot harder to do after the fact. It is also nice to fix at a new release.

amotl commented 1 year ago

Closing this in favor of #76. Thank you very much, @xylar!