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: allow uploading on main channel for v1 recipes #337

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

nichmor
Copy link
Contributor

@nichmor nichmor commented Aug 19, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

now that we have implemented all rattler-build lints checks and we have a working feedstock: https://github.com/conda-forge/jolt-physics-feedstock
I would like to uplift restrictions of uploading rattler-build to main channel.

Should be merged after release of: conda-forge/conda-smithy#2037

@nichmor nichmor requested a review from a team as a code owner August 19, 2024 09:25
@conda-forge-webservices
Copy link
Contributor

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/meta.yaml) and found it was in an excellent condition.

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.

We need to wait a bit imho for the dust to settle on the linter. There are a few PRs left. Also the bot is not yet finished, which might break things.

@nichmor
Copy link
Contributor Author

nichmor commented Aug 19, 2024

We need to wait a bit imho for the dust to settle on the linter. There are a few PRs left. Also the bot is not yet finished, which might break things.

Hey ! Thanks for the review!

Can we make an actionable plan for linter work?
I know that there is conda-forge/conda-smithy#2028 work that needs to be done.
Do you have anything else on your radar?

Regarding the bot work- can it block us from allowing us to upload on the main channel? I think that even we break things we do it on bot side, uploaded packages are not affected? Do you have something on your mind how we can achieve this?

@beckermr
Copy link
Member

It depends on how the bot breaks. If people convert feedstocks that need ABI migrations or version migrations, we'll break more than just a single feedstock. Things that affect the whole ecosystem like this have to move slowly enough for folks to adapt.

@baszalmstra
Copy link
Member

baszalmstra commented Aug 20, 2024

@beckermr I completely understand where you are coming from but I feel the issue of packages not being uploaded is a bit of a chicken-and-egg problem. We have already had people (in the chat) wanting to convert their feedstock but holding off because the packages would not get uploaded. That makes it hard for the dust to settle. I know @jaimergp also had different ideas about this.

From my perspective, these are the issues that are left open:

conda-smithy:

bot:

Am I missing issues or pull-requests?

edit: These items have either been deemed not needed or done in other places.

@beckermr
Copy link
Member

The ABI migrations run mini migrators on top of the recipes. We'll need to merge the prs to skip those or make them work on the new format.

@baszalmstra
Copy link
Member

The ABI migrations run mini migrators on top of the recipes. We'll need to merge the prs to skip those or make them work on the new format.

Is there already an issue for this so we can keep track of it?

@beckermr
Copy link
Member

beckermr commented Aug 20, 2024

On the bot repo it is issues 2918 2920 and 2924

@baszalmstra
Copy link
Member

Thanks I updated the overview. 👍

@beckermr
Copy link
Member

Another issue is that the solver checks repo does not currently work for v1 recipes: regro/conda-forge-feedstock-check-solvable#54

Do we have a tracking issue on the website repo for this? If not we should.

@wolfv
Copy link
Member

wolfv commented Sep 10, 2024

Some smaller progress was made wrt to the CODEOWNERS file - a change in rattler-build-conda-compat should have made it work.

@wolfv
Copy link
Member

wolfv commented Sep 25, 2024

I bumped the version to 4.9.5 - would love to see this merged :)

recipe/meta.yaml Outdated Show resolved Hide resolved
@beckermr
Copy link
Member

@conda-forge/core shall we go ahead with this? I am a +1.

@beckermr beckermr changed the title feat: allow uploading on main channel feat: allow uploading on main channel for v1 recipes Sep 25, 2024
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Am in favor of trying this

Would suggest we do this on a non-weekend adjacent day (so not Monday or Friday). For example we could do this tomorrow (Thursday morning) or some time next week (Tuesday or later). Mention this as most people likely don't want to spend their weekends debugging and fixing issues. So picking some time where people will be around and ready to work on issues would be best

@beckermr
Copy link
Member

I'm around for the next four hours so I'll merge this now and watch things on the heroku server.

@beckermr beckermr merged commit 2523297 into conda-forge:main Sep 25, 2024
52 checks passed
@jakirkham
Copy link
Member

Sounds good. Thanks Matt and thanks everyone working on this upgrade! 🙏

@jakirkham
Copy link
Member

jakirkham commented Sep 25, 2024

Noticed the conda-forge-ci-setup cross-compiled architectures are failing (for example)

__main__.py: error: argument --sysroot: expected one argument

Do we need to bootstrap this change somehow (wait for native builds and restart)? Or is there something missing in those cases?

@beckermr
Copy link
Member

That appears to be a cross-python issue unrelated to this package, right?

cc @conda-forge/core @conda-forge/cross-python

@jakirkham
Copy link
Member

Yeah came to the same conclusion

That package has this logic in its activation script:

    $BUILD_PREFIX/bin/python -m crossenv $PREFIX/bin/python \
        --sysroot $CONDA_BUILD_SYSROOT \
        --without-pip $BUILD_PREFIX/venv \
        --sysconfigdata-file "$sysconfigdata_fn" \
	--machine ${machine} \
        --cc ${CC:-@CC@} \
        --cxx ${CXX:-@CXX@}

Am guessing $CONDA_BUILD_SYSROOT is undefined when building with rattler-build. Though maybe there is another change or explanation that I'm missing

@beckermr
Copy link
Member

We don't build with rattler build in ci-setup, so it is something else.

@jakirkham
Copy link
Member

jakirkham commented Sep 25, 2024

AFAIK we don't set CONDA_BUILD_SYSROOT here (with the exception of macOS)

Note this error happens for the CPU-only ARM/PPC builds here (not CUDA ones). In case that provides any clues

Separately: Maybe we should switch conda-forge-ci-setup to use rattler-build? It would be good to have a check here to confirm that we are able to build with rattler-build before publishing new conda-forge-ci-setup packages

@beckermr
Copy link
Member

beckermr commented Sep 25, 2024

I think this is conda-build 24.9

I get the same error with 24.7.1

@beckermr
Copy link
Member

Something shipped today broke everything. The CI ran fine this morning.

@jakirkham
Copy link
Member

Could this be related ( conda-forge/binutils-feedstock#76 )?

@jakirkham jakirkham mentioned this pull request Sep 25, 2024
1 task
@jakirkham
Copy link
Member

Discussion continues in PR: #349

@jakirkham
Copy link
Member

In any event, think we agree this is not related to the changes in this PR. Just external changes that are now affecting conda-forge-ci-setup

Given that, decided to kick the tires on this change. Recently, while I was out, my colleagues added a brand new feedstock numba-cuda to conda-forge, which uses the new recipe format. Of course no packages built from that at the time. Started a fresh CI build to see if it would upload packages. It appears to have worked! 🎉 Will work with them on testing these packages out

Also have started a fresh build of jolt-physics. Let's keep an eye on how that goes

@jakirkham
Copy link
Member

Also have started a fresh build of jolt-physics. Let's keep an eye on how that goes

This unfortunately failed. Though for reasons that appear to have to do with that feedstock itself

Am guessing it just needs PR ( conda-forge/jolt-physics-feedstock#2 ) to fix it

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.

6 participants