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

BUG fix zipping for c stdlib #5592

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I've started rerendering the recipe as instructed in #5591.

If I find any needed changes to the recipe, I'll push them to this PR shortly. Thank you for waiting!

Here's a checklist to do before merging.

  • Bump the build number if needed.

Fixes #5591

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

conda-forge-webservices[bot] and others added 2 commits March 5, 2024 12:42
@beckermr beckermr changed the title MNT: rerender BUG fix zipping for c stdlib Mar 5, 2024
@hmaarrfk
Copy link
Contributor

hmaarrfk commented Mar 5, 2024

how is this proposed logic any different than before? they seem equivalent.

@beckermr
Copy link
Member

beckermr commented Mar 5, 2024

Two changes

  • it covers platforms like s390x etc. these not getting covered caused rendering issues.
  • it zips these keys with the cdt ones since both need to match IIUIC

@beckermr beckermr merged commit 0eafa07 into conda-forge:main Mar 5, 2024
4 checks passed
@h-vetinari
Copy link
Member

I was hoping we wouldn't have to have the compiler and stdlib in the same zip... Definitely the cdt needs to be zipped with the stdlib, but perhaps we can split that group out of the mega-zip, at least once we've dropped cos6 this summer.

@beckermr
Copy link
Member

beckermr commented Mar 5, 2024

Possibly? I don't see the issue here really except that it makes matching things up somewhat error prone.

@h-vetinari
Copy link
Member

Possibly? I don't see the issue here really except that it makes matching things up somewhat error prone.

It's not a big issue per se, but it gets hard to change anything non-trivial about such a large zip in a local CBC without running into breakages. Also conceptually, compiler and stdlib shouldn't have to be coupled. But again, not an issue for now. Thanks for fixing it!

@jakirkham
Copy link
Member

If we do this, we need to update the CUDA migrators too as they touch this key. Currently migration is broken on any CUDA feedstock ( for example: conda-forge/ucx-split-feedstock#158 (comment) )

That said, this kind of smells like a bug in conda-smithy

cc @mbargull (in case you have any ideas 🙂)

@@ -13,8 +13,8 @@ c_stdlib:
- macosx_deployment_target # [osx]
- vs # [win]
c_stdlib_version: # [unix]
- 2.12 # [linux64]
- 2.17 # [aarch64 or ppc64le]
- 2.12 # [linux and x86_64]
Copy link
Member

Choose a reason for hiding this comment

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

Also for CUDA this needs to be 2.17 (even on x86_64)

@h-vetinari
Copy link
Member

If we do this, we need to update the CUDA migrators too as they touch this key.

Yeah, this is the kind of thing why I would have liked to avoid adding the stdlib into that mega-zip_keys... 😑

I think we'll have to duplicate much of the os.environ.get("CF_CUDA_ENABLED", "False") etc. stuff also for the stdlib version.

@beckermr
Copy link
Member

beckermr commented Mar 6, 2024

Oh no sorry! We have to zip it with cdt so we're stuck. How can I help fix things? I'm not sure what is needed.

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.

@conda-forge-admin rerender
5 participants