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

Adds rattler-build recipe compatibility (and adds jolt-physics) #27243

Merged
merged 6 commits into from
Aug 12, 2024

Conversation

baszalmstra
Copy link
Member

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

This PR adds support for the new recipe format and adds the first such recipe for https://github.com/jrouwe/JoltPhysics

We already did a previous attempt with #27008 which we reverted because of some issues with conda-smithy. A new release is available which should fix this issue.

@conda-forge-webservices
Copy link

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 (recipes/example-new-recipe/recipe.yaml, recipes/jolt-physics/recipe.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/example-new-recipe/recipe.yaml:

This is a rattler-build recipe and not yet lintable. We are working on it!

For recipes/jolt-physics/recipe.yaml:

This is a rattler-build recipe and not yet lintable. We are working on it!

Copy link

Hi! Thanks for your contribution to conda-forge.
We appreciate your effort in improving our project.
However, it looks like some changes were made outside the recipes directory.
To ensure everything runs smoothly, please make sure that recipes are only added to the recipes folder and no other files are changed.
Please make sure that any changes are reverted before you submit your pull request for review.

If these changes are intentional (and you aren't submitting a recipe), please attach a maintenance label to the PR.
Thanks!

@baszalmstra
Copy link
Member Author

Im unsure what is wrong with the WIndows build? There doesnt seem to be an actual error?

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

This PR does too many things IMO.

  • format build_all.py
  • add rattler-build support
  • add a new rattler example recipe
  • add a genuinely new recipe

Proving the functionality would already with actually building the example recipe (perhaps with some additional safeguard that we don't create a feedstock), and then a follow-up can add jolt.

W.r.t. to the error on windows, it seems to be failing while trying to call inspect_artifacts.

Inspecting artifacts
"inspect_artifacts needs conda-forge-ci-setup >=4.6.0"
##[error]Cmd.exe exited with code '1'.

.ci_support/build_all.py Show resolved Hide resolved
Copy link

Hi! Thanks for your contribution to conda-forge.
We appreciate your effort in improving our project.
However, it looks like some changes were made outside the recipes directory.
To ensure everything runs smoothly, please make sure that recipes are only added to the recipes folder and no other files are changed.
Please make sure that any changes are reverted before you submit your pull request for review.

If these changes are intentional (and you aren't submitting a recipe), please attach a maintenance label to the PR.
Thanks!

@baszalmstra
Copy link
Member Author

Thanks for checking out this PR @h-vetinari

Note that this was already reviewed in #27008 by both @jaimegp and @wolfv but happy to make more changes.

The problem with the current setup is that there is no easy way to test this. The creation of the feedstock is performed by another repository so without merging this that code won't run and that code won't actually create a feedstock if there is no recipe, therefor we must add a recipe.

The build-all.py script is tested currently by running the CI, if that turns green it should be good to go. I don't see why this recipe should be different from testing the create feedstock code.

The example recipe is there as an example similar to the existing example recipe. I would be happy to add it in another PR but in the previous round of reviews, it was mentioned that this was missing from the PR.

I undid the formatting.

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Thanks for removing the formatting changes! Looks pretty good at first glance already. I left some comments/suggestions.

recipes/example-new-recipe/recipe.yaml Outdated Show resolved Hide resolved
recipes/example-new-recipe/recipe.yaml Outdated Show resolved Hide resolved
.github/workflows/correct_directory.yml Outdated Show resolved Hide resolved
rmtree(os.path.join(recipes_dir, example_recipe), ignore_errors=True)

# Determine the locations for the variant config files.
specs = OrderedDict()
Copy link
Member

Choose a reason for hiding this comment

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

This is less a question for this PR and more a topic overall across our infrastructure, but at least I thought I'd note it here: conda-forge/conda-smithy#2033

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
specs = OrderedDict()
specs = {}

@h-vetinari
Copy link
Member

Note that this was already reviewed in #27008 by both @jaimegp and @wolfv but happy to make more changes.

Yeah, fair enough, I don't want to complicate this process for you any further. Jaime would actually be a great person to ask for figuring out what (if anything) is going wrong with the artefact inspection. ;-)

Copy link

Hi! Thanks for your contribution to conda-forge.
We appreciate your effort in improving our project.
However, it looks like some changes were made outside the recipes directory.
To ensure everything runs smoothly, please make sure that recipes are only added to the recipes folder and no other files are changed.
Please make sure that any changes are reverted before you submit your pull request for review.

If these changes are intentional (and you aren't submitting a recipe), please attach a maintenance label to the PR.
Thanks!

Copy link

Hi! Thanks for your contribution to conda-forge.
We appreciate your effort in improving our project.
However, it looks like some changes were made outside the recipes directory.
To ensure everything runs smoothly, please make sure that recipes are only added to the recipes folder and no other files are changed.
Please make sure that any changes are reverted before you submit your pull request for review.

If these changes are intentional (and you aren't submitting a recipe), please attach a maintenance label to the PR.
Thanks!

@baszalmstra
Copy link
Member Author

The issue with inspect-artifacts should be fixed with conda-forge/conda-forge-ci-setup-feedstock#336

@jaimergp jaimergp merged commit e9049d9 into conda-forge:main Aug 12, 2024
2 of 6 checks passed
@jaimergp
Copy link
Member

YOLO 🚀

- python

tests:
# Some packages might need a `test/commands` key to check CLI.
Copy link
Contributor

Choose a reason for hiding this comment

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

this would now be, presumably, tests/0/script/. tests/0/files/source was also surprising: the glob needs e.g. tests/**/*, not just tests now.

Copy link
Member

Choose a reason for hiding this comment

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

True, that comment looks wrong. It is also script not command in the test section

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants