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

refactor build_with_mambabuild bool with conda_build_tool choice #1732

Merged
merged 10 commits into from
Sep 6, 2023

Conversation

jaimergp
Copy link
Member

@jaimergp jaimergp commented Apr 26, 2023

Checklist

  • Added a news entry

Refactoring the build_with_mambabuild boolean option with a more future-proof conda_build_tool "list of choices". I have added support for:

  • mambabuild
  • conda-build with conda-libmamba-solver
  • conda-build with (forced) classic solver
  • vanilla conda-build (no specific solver configuration)

In a hypothetical future, this setup would also allow us to have boa and rattler-build, for example.

build_with_mambabuild=True maps to conda_build_tool=mambabuild. The false equivalent maps to conda_build_tool=conda-build.

Note that the default behaviour (using mambabuild) has not changed.

Why this change now?

This just adds opt-in support for conda-libmamba-solver. I am working on improving the integration with conda-build in conda/conda-libmamba-solver#194. While the current released implementation works for single output recipes, I think it might not work correctly with multi-output stuff, or maybe with multi-recipe calls.

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.

I do not see any obvious problems. We'll need to test this live before merging.

jaimergp added a commit to jaimergp/dav1d-feedstock that referenced this pull request Aug 6, 2023
jaimergp added a commit to jaimergp/heavydb-ext-feedstock that referenced this pull request Aug 6, 2023
jaimergp added a commit to jaimergp/rdkit-feedstock that referenced this pull request Aug 6, 2023
@jaimergp
Copy link
Member Author

jaimergp commented Aug 6, 2023

A few DNM PRs to test this live:

Edit: Interestingly, all Windows builds reported an exit code of 1 despite having passed the build and tests successfully 🤔

@jakirkham
Copy link
Member

Think it might help for us to ship conda-libmamba-solver in Miniforge ( conda-forge/miniforge#284 )

Maybe this simplifies the install commands (or even removes the need for them)

Comment on lines +76 to +77
set "CONDA_SOLVER=libmamba"
conda.exe build "{{ recipe_dir }}" -m .ci_support\%CONFIG%.yaml --suppress-variables %EXTRA_CB_OPTIONS%
Copy link
Member

Choose a reason for hiding this comment

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

Would it makes sense to either specify this by the --solver flag or better yet add this to the .condarc

Copy link
Member Author

Choose a reason for hiding this comment

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

conda-build doesn't have the --solver flag so the other alternative is writing to .condarc, but that will have global impact on the other uses of conda so I initially wanted to constrain the scope as much as possible (in case something breaks).

I am happy to revisit that in the future as we debug it step by step.

@jakirkham
Copy link
Member

Thanks Jaime! 🙏

Had a couple questions above

@jaimergp
Copy link
Member Author

The strange Windows exit_code=1 issues are gone if we use conda 23.7 and conda-build 3.26 🤷

@jaimergp jaimergp marked this pull request as ready for review August 31, 2023 19:34
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.

Lgtm! Some comments on repetition in the templates but also the code blocks are clear so I think we should merge!

@beckermr
Copy link
Member

beckermr commented Sep 6, 2023

LGTM!

@jaimergp jaimergp mentioned this pull request Sep 6, 2023
1 task
@jaimergp jaimergp merged commit 5ae950e into conda-forge:main Sep 6, 2023
2 checks passed
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.

4 participants