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

fix c_stdlib_version zip with cdt_name #5607

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

h-vetinari
Copy link
Member

Follow-up to #5592

This tries to match the setup of the linux selectors with cdtname:

cdt_name: # [linux]
- cos6 # [linux64 and os.environ.get("DEFAULT_LINUX_VERSION", "cos6") == "cos6"]
- cos7 # [linux64 and os.environ.get("DEFAULT_LINUX_VERSION", "cos6") == "cos7"]
- cos7 # [linux and aarch64]
- cos7 # [linux and ppc64le]
- cos7 # [linux and armv7l]
- cos7 # [linux and s390x]
- cos7 # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cos7 # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]

CC @beckermr @jakirkham @isuruf

@h-vetinari h-vetinari requested a review from a team as a code owner March 6, 2024 18:24
@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.

@jakirkham
Copy link
Member

Thanks Axel! 🙏

Do we need to do this with c_stdlib too as it is also zipped?

Think we may also need to add both of these to the CUDA 12 migrator as well

@h-vetinari
Copy link
Member Author

h-vetinari commented Mar 6, 2024

Do we need to do this with c_stdlib too as it is also zipped?

I don't believe so; for example, c_compiler, is part of the same zip as well but has no extra lines for CF_CUDA_ENABLED or in cuda120.yaml. That's because the zip itself changes based on CF_CUDA_ENABLED

Edit: Nevermind...

@h-vetinari
Copy link
Member Author

Do we need to do this with c_stdlib too as it is also zipped?

So I looked at this again in more detail, and I had wrongly remembered / imagined that c_compiler needed to be zipped with its version. That's why it doesn't have config for CF_CUDA_ENABLED, and why it's not updated as part of the CUDA migrator. I think the same situation applies to c_stdlib, so I've removed that from the zip_keys.

Think we may also need to add both of these to the CUDA 12 migrator as well

Consequently, I've added only c_stdlib_version to cuda120.yaml.

I was kinda expecting the CI here would check matching zip length also for CUDA builds? But in any case, I think this should be good now. I'd propose to merge it and see if it solves the ucx situation.

@beckermr
Copy link
Member

beckermr commented Mar 6, 2024

@h-vetinari @jakirkham can you all test these changes with relevant feedstocks before merging?

@jakirkham
Copy link
Member

Yes am planning to test. Just have been in meetings most of the day

@h-vetinari
Copy link
Member Author

AFAIU it's not possible to fully test this because the change in the zip_keys will only apply once the global pinning is updated.

@beckermr
Copy link
Member

beckermr commented Mar 6, 2024

You can run against the new copy of the config by download it locally and calling

conda-smithy rerender --no-check-uptodate -e conda_forge_pinning.yaml

@h-vetinari
Copy link
Member Author

h-vetinari commented Mar 6, 2024

OK, I can confirm that conda-forge/ucx-split-feedstock#158 can rerender again with the changes from this PR.

Edit: Also tested a non-cuda feedstock (numpy), no problems.

@h-vetinari h-vetinari changed the title fix c_stdlib_version zip with cdtname fix c_stdlib_version zip with cdt_name Mar 6, 2024
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.

Thank you!

@h-vetinari h-vetinari merged commit 53554ca into conda-forge:main Mar 7, 2024
4 checks passed
@h-vetinari h-vetinari deleted the stdlib branch March 7, 2024 00:02
@jakirkham
Copy link
Member

Thanks all! 🙏

This is working smoothly now 😄

@njzjz
Copy link
Member

njzjz commented Mar 17, 2024

I got the glibc error for cuda12 builds today. It installed sysroot 2.28 instead of 2.17, while the glibc in the image is still 2.17. Finally, I found this PR and realized cuda120.yaml should be updated manually. The update for cuda120.yaml here doesn't affect the downstream feedstocks.

@njzjz
Copy link
Member

njzjz commented Mar 17, 2024

I got the glibc error for cuda12 builds today. It installed sysroot 2.28 instead of 2.17, while the glibc in the image is still 2.17. Finally, I found this PR and realized cuda120.yaml should be updated manually. The update for cuda120.yaml here doesn't affect the downstream feedstocks.

Wait, it doesn't work...

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