conda-forge / conda-smithy

The tool for managing conda-forge feedstocks.
https://conda-forge.org/
BSD 3-Clause "New" or "Revised" License
149 stars 176 forks source link

feat: add initial integration of rattler-build #1876

Closed nichmor closed 3 months ago

nichmor commented 6 months ago

High overview of this integration:

Checklist

nichmor commented 6 months ago

Why are introducing a bunch of extra template files? I would assume we can use the ones we have already.

Good question! they will be deleted on final PR version - I'm using them in development process to clearly point what is needed new for rattler-build and will integrate it in the old templates

xhochy commented 6 months ago

It would be nice if this were coordinated together with the existing rattler-build implementation in https://github.com/conda-forge/polarify-feedstock/tree/rattler-build

wolfv commented 6 months ago

We should ping @0xbe7a :)

nichmor commented 6 months ago

It would be nice if this were coordinated together with the existing rattler-build implementation in https://github.com/conda-forge/polarify-feedstock/tree/rattler-build

Hey @xhochy ! Thanks for mentioning of polarify-feedstock. @wolfv shared with me the rattler-build branch early in the days so I tried to take polarify-feedstock/rattler-build as a "North Star" while working on rattler-build integration. This is the current version https://github.com/conda-forge/polarify-feedstock/pull/17 rendered by "new" conda-smithy. _( .ciconfig files naming issue will be fixed ) Let me know if you have any questions or suggestions. Thanks

pavelzw commented 6 months ago

The latest conda-smithy version that bela was working on is here: https://github.com/0xbe7a/conda-smithy/commit/1e818fc0ad4c7e5c9f3539ba9fa88cb70ed82707

nichmor commented 6 months ago

Hey @beckermr! I would like to ask for your review :) I've added some notes in the PR description to explain what & how integration was done - hope that it will be easier to review with these notes.

jakirkham commented 6 months ago

Feedback from the meeting earlier:

Others should feel welcome to jump in and add/revise anything above

jakirkham commented 6 months ago

May have missed this earlier, but with conda smithy init are we adding a flag to allow configuring whether conda-build or rattler-build is used? Or how is that handled?

Also there was a mention about a new variant.yaml. Do you have some more details about what that is and how it is being used?

Thanks for your help! 🙏

beckermr commented 6 months ago

Rename conda_smithy.rattler_build to something like conda_smithy.rattler_build_compat or similar

This is not what was mentioned on the call. The suggestion there as I understood it was to move that file into rattler build and rename it conda_compat or similar.

jakirkham commented 6 months ago

Ah ok. Thanks for clarifying! 🙏

I may not have understood the suggestion

wolfv commented 6 months ago

Currently rattler-build doesn't have any Python parts. I don't really see how it would be useful to move the rattler-build / conda-build integration anywhere else right now. If it's OK I would suggest to keep it in conda-smithy and we could revisit this when rattler-build has a Python API.

jakirkham commented 6 months ago

Right that's why I thought we settled on renaming the module for clarity. Maybe @isuruf can clarify the suggestion

isuruf commented 6 months ago

I meant a rattler_build.conda_build_compat module. I didn't know that rattler-build didn't have a python API. Still, I don't think we should be maintaining this code in conda-smithy. Maybe a small python package rattler-conda-build-compat would make sense here.

nichmor commented 6 months ago

I meant a rattler_build.conda_build_compat module. I didn't know that rattler-build didn't have a python API. Still, I don't think we should be maintaining this code in conda-smithy. Maybe a small python package rattler-conda-build-compat would make sense here.

hey @isuruf ! thanks a lot for your review!

I will extract the entire rattler_build package into a separate conda package that will be imported by conda-smithy and called directly rattler-conda-build.render, without the need of storing this code in conda-smithy. Let me know if it's sounds good for you

isuruf commented 6 months ago

Sounds great. Thanks for working on that.

nichmor commented 6 months ago

May have missed this earlier, but with conda smithy init are we adding a flag to allow configuring whether conda-build or rattler-build is used? Or how is that handled?

Also there was a mention about a new variant.yaml. Do you have some more details about what that is and how it is being used?

Thanks for your help! 🙏

hey @jakirkham ! thanks a lot for your input!

Please let me know if you have any other questions / or suggestions regarding this, I will be happy to respond to them.

thank you!

jakirkham commented 6 months ago

Thanks for the info! 🙏

during the execution of conda smithy init it will try to load first the meta.yaml. If it's not present, it switch to check if recipe.yaml can be located. If the recipe.yaml is present, during the creation of feedstock it writes to conda-forge.yaml conda_build_tool: rattler-build so this later would tell conda-smithy ( for example on rerender step ) what tool to use for building or rendering.

Right I mean, can we please make this configurable at conda smithy init time?

Also, at least until we have more experience with it, the default of conda smithy init should be to use conda-build. Would like this to be an opt-in experience until we have more familiarity

variants.yaml is a new configuration file used for rattler-build, similar how conda_build_config.yaml is for conda-build. The "good news" is that conda_build_config.yaml is still supported ( if variants.yaml is missing ) by conda-smithy even when using rattler-build and it will generate correctly the CI configuration files. In polarify-feedstock example we can see that recipe.yamlhave conda_build_config.yamland channel_targets are picked up correctly for azure configuration

Am curious why we have two files that are doing the same thing. Would it be possible to just use conda_build_config.yaml?

Or alternatively if we want to move this to a new naming convention, could we start a CEP to motivate that change?

cc @jezdez (who may be interested in this as well)

nichmor commented 6 months ago

Thanks for the info! 🙏

during the execution of conda smithy init it will try to load first the meta.yaml. If it's not present, it switch to check if recipe.yaml can be located. If the recipe.yaml is present, during the creation of feedstock it writes to conda-forge.yaml conda_build_tool: rattler-build so this later would tell conda-smithy ( for example on rerender step ) what tool to use for building or rendering.

Right I mean, can we please make this configurable at conda smithy init time?

Also, at least until we have more experience with it, the default of conda smithy init should be to use conda-build. Would like this to be an opt-in experience until we have more familiarity

variants.yaml is a new configuration file used for rattler-build, similar how conda_build_config.yaml is for conda-build. The "good news" is that conda_build_config.yaml is still supported ( if variants.yaml is missing ) by conda-smithy even when using rattler-build and it will generate correctly the CI configuration files. In polarify-feedstock example we can see that recipe.yamlhave conda_build_config.yamland channel_targets are picked up correctly for azure configuration

Am curious why we have two files that are doing the same thing. Would it be possible to just use conda_build_config.yaml?

Or alternatively if we want to move this to a new naming convention, could we start a CEP to motivate that change?

cc @jezdez (who may be interested in this as well)

Please let me know what you think about these points and how we can improve them if necessary

wolfv commented 6 months ago

To add to what @nichmor said: rattler-build does not understand the 'comments-as-selectors' in an arbitrary conda_build_config.yaml. We haven't implemented that and are not really planning to implement it. Thankfully conda-smithy produces the rendered conda_build_configs and they are easy to parse and work with rattler-build.

nichmor commented 5 months ago

hey @isuruf I would like to ask for review again : I've implemented missing linting checks and extracted all rattler-build related code to rattler-build-conda-compat.

hey @jakirkham ! I wanted to let you know that I've implemented the conda-specific checks ( quote: Can we confirm that linter case covers other conda-forge lints (like duplicates in staged-recipes, bioconda checks, etc.) here: https://github.com/nichmor/rattler-build-conda-compat/blob/main/src/rattler_build_conda_compat/lint.py#L424 and moved the schema outside ( into rattler-build-conda-compat module ) . Also extended the current test suite for testing linting of recipe to support recipe.yaml testing.

nichmor commented 5 months ago

hey @beckermr ! Additional templates were removed - so now we are extending existing ones. Let me know what do you think Thank you.

nichmor commented 5 months ago

Please rerender an existing feedstock that was rendered with latest released conda-smithy and make sure that there was no change.

hey @isuruf ! This is the result: https://github.com/nichmor/rattler-build-feedstock/pull/1 https://github.com/nichmor/py-rattler-feedstock/pull/1

there were some differences in amount of spaces in .sh scripts, so I've adjusted conda-smithy in this commit https://github.com/conda-forge/conda-smithy/pull/1876/commits/780a11596b7c1313e08f9b32e5c262a878921d50

let me know if there is anything else I can do

nichmor commented 5 months ago

hey @isuruf @beckermr ! I wanted to ask if there is anything else that need to be changed or improved. Thank you!

wolfv commented 4 months ago

Hey @beckermr - yes, we'll add support for the stdlib jinja function to rattler-build. But shouldn't this be orthogonal to this PR? I think we would want to ratify the CEPs before allowing rattler-build on conda-forge anyways. Ideally we could still already merge this PR?

beckermr commented 4 months ago

Thanks @wolfv! If we merge this PR, we are allowing support for rattler as soon as there is a release of conda-smithy. So, if we want to wait for the CEPS, which I agree we should, then we need to wait no matter what. So I think getting support in for the stdlib jinja and adding that to the tests should be able to happen before this is merged.

isuruf commented 4 months ago

Maybe we can disallow publishing to conda-forge main label with rattler-build and create an issue with things to be done before that restriction is taken off?

wolfv commented 4 months ago

I would very much prefer @isuruf suggestion so that we can move on from this big PR and iterate with a few smaller ones :)

beckermr commented 4 months ago

Yes I agree with @isuruf. That will work great! I'll finish the rest of my review.

wolfv commented 4 months ago

@beckermr is the "big change" you are talking about the linting move from rattler-build-conda-compat back into smithy? Or did you mean something else (that seems doable).

Thanks for the review!

beckermr commented 4 months ago

yes @wolfv - I meant the linting stuff in the packages.

nichmor commented 4 months ago

hey @isuruf ! I've made some refactor changes to reuse common code across meta and recipe.yaml linting as you suggested

nichmor commented 4 months ago

hey @isuruf ! Let me know if there is anything else you want me to refactor Thank you!

nichmor commented 3 months ago

As discussed with @isuruf, I've removed the linter from this PR. It will be added in the next one with proper refactoring.

nichmor commented 3 months ago

Looks good. Tests still fail though

fixed. rattler-build was cached in the CI on older version

isuruf commented 3 months ago

Looks good. Can you add a news entry?

nichmor commented 3 months ago

Looks good. Can you add a news entry?

Added

jaimergp commented 2 months ago

A piece was missing. Added in https://github.com/conda-forge/conda-smithy/pull/1977.