Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add initial integration of rattler-build #1876

Merged
merged 67 commits into from
Jul 5, 2024

Conversation

nichmor
Copy link
Contributor

@nichmor nichmor commented Mar 21, 2024

High overview of this integration:

  • conda_build_tool from conda-forge now support rattler-build tool. If recipe.yaml is present when running conda-smithy init it will populate the new conda-forge.yaml with { 'conda_build_tool': 'rattler-build' }
  • A new class MetaData is created in rattler_build.build which inherit from conda_build.Metadata and overwrite some necessary methods for itself ( get_used_var, get_used_vars). The rational for this is to keep API the same as much as possible.
  • get_package_variants function is copied from conda_build with a few changes for rattler-build ( it's main usage is to support generation of different configs files )
  • rattler_build.loader module contains a class that is used as a custom yaml loader which will evaluate rattler-build variants.yaml selectors.
  • rattler_build.utils contains find_recipe function conda_build with similar API flow, the only necessary change is that VALID_METAS contain recipe.yaml
  • build_steps.sh.tmp, run_osx_build.sh.tmpl, run_win_build.bat.tmpl contains addition of rattler-build command which is added in template using jinja if {%- if conda_build_tool != "rattler-build" %}.
  • tests are little refactored - the config_yaml fixture now contains 2 params = conda_build and rattler_build so each test for conda_build now is extend to test and rattler-build. To make navigation and creation of test recipes and configuration more easy , I've extracted them in separate tests/recipes folders. ( example: recipes/py_recipe )
  • conda-smithy lint also support linting recipe.yaml using it's schema.json ( which is added in data/rattler-recipe-schema.json )
  • a practical example of how it will look live: boltons-feedstock with new recipe https://github.com/conda-forge/boltons-feedstock/pull/24/files#diff-ad6aed43f0cf479348ccccec8e7185c4b70582192329f11908dd22a731771702
    polarify-feedstock: feat: pre-render with new conda smithy with rattler build support polarify-feedstock#17

Checklist

  • Added a news entry

@nichmor nichmor requested a review from a team as a code owner March 21, 2024 15:27
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nichmor
Copy link
Contributor Author

nichmor commented Mar 21, 2024

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

@nichmor nichmor marked this pull request as draft March 21, 2024 15:42
@xhochy
Copy link
Member

xhochy commented Mar 21, 2024

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

wolfv commented Mar 21, 2024

We should ping @0xbe7a :)

@h-vetinari h-vetinari changed the title feat: add initional integration of rattler-build feat: add initial integration of rattler-build Mar 21, 2024
@nichmor
Copy link
Contributor Author

nichmor commented Mar 22, 2024

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 conda-forge/polarify-feedstock#17 rendered by "new" conda-smithy.
( .ci_config files naming issue will be fixed )
Let me know if you have any questions or suggestions.
Thanks

@pavelzw
Copy link
Member

pavelzw commented Mar 22, 2024

The latest conda-smithy version that bela was working on is here: 0xbe7a@1e818fc

@nichmor
Copy link
Contributor Author

nichmor commented May 29, 2024

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

lint_empty_line_of_the_file(recipe_dir, recipe_fname, lints)

# 12a: License family must be valid (conda-build checks for that)
rattler_linter.lint_has_recipe_file(about_section, lints)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rattler_linter.lint_has_recipe_file(about_section, lints)
rattler_linter.lint_has_license_file(about_section, lints)

Comment on lines 1235 to 1236
# 15: Check if we are using legacy patterns
rattler_linter.lint_legacy_patterns(requirements_section)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this different from conda-build?

build_reqs = requirements_section.get("build", None)
lints.append(rattler_linter.lint_legacy_compilers(build_reqs))

# 22: Single space in pinned requirements
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@nichmor
Copy link
Contributor Author

nichmor commented Jun 19, 2024

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

@nichmor nichmor requested a review from isuruf June 20, 2024 05:47
Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Tests still fail though

@nichmor
Copy link
Contributor Author

nichmor commented Jun 25, 2024

Looks good. Tests still fail though

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

@nichmor nichmor requested a review from isuruf June 25, 2024 19:40
@isuruf
Copy link
Member

isuruf commented Jun 27, 2024

Looks good. Can you add a news entry?

@nichmor
Copy link
Contributor Author

nichmor commented Jul 5, 2024

Looks good. Can you add a news entry?

Added

@isuruf isuruf merged commit 917070c into conda-forge:main Jul 5, 2024
2 checks passed
@jaimergp jaimergp mentioned this pull request Jul 15, 2024
2 tasks
@jaimergp
Copy link
Member

A piece was missing. Added in #1977.

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.

8 participants