Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update glibc pinning to 2.28 for aarch64 #24

Merged
merged 7 commits into from
Dec 3, 2024

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Nov 28, 2024

We've seen in conda-forge/pytorch-cpu-feedstock#293 that libcufile.so requires glibc 2.28 on aarch (not just that, it looks like it also requires a constraint on the sysroot). First add a test in c234982 that should fail for the current state of the feedstock.

Fixes #23

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Nov 28, 2024

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.

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

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). Your recipe may not receive automatic updates and/or may not be compatible with conda-forge's infrastructure. Please check the logs for more information and ensure your recipe can be parsed.
  • ℹ️ The recipe is not parsable by parser conda-recipe-manager. Your recipe may not receive automatic updates and/or may not be compatible with conda-forge's infrastructure. Please check the logs for more information and ensure your recipe can be parsed.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12149779418. Examine the logs at this URL for more detail.

@h-vetinari h-vetinari force-pushed the glibc branch 7 times, most recently from db4f871 to 9bafb14 Compare November 28, 2024 06:39
@h-vetinari h-vetinari marked this pull request as ready for review November 28, 2024 07:03
@h-vetinari h-vetinari requested a review from a team as a code owner November 28, 2024 07:03
@h-vetinari
Copy link
Member Author

@conda-forge/libcufile, this is ready, PTAL. The test for right glibc version in a library could probably be reused elsewhere where binary repackaging happens, to ensure the metadata for c_stdlib_version is actually correct.

Comment on lines 3 to 4
c_stdlib_version: # [linux and aarch64]
- 2.28 # [linux and aarch64]
Copy link
Member

@carterbox carterbox Dec 2, 2024

Choose a reason for hiding this comment

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

Suggested change
c_stdlib_version: # [linux and aarch64]
- 2.28 # [linux and aarch64]
c_stdlib_version: # [linux]
- 2.28 # [linux and aarch64]
- 2.17 # [linux and x86_64]

We probably want to make this explicit for x86 too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, you can if you prefer, but it's really not necessary

recipe/meta.yaml Outdated
Comment on lines 69 to 72
# if there's another, older, sysroot in the same environment;
# libcufile might see the glibc there and fail to load, see
# https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/6745#issuecomment-2503604312
- sysroot_{{ target_platform }} >={{ c_stdlib_version }}
Copy link
Member

Choose a reason for hiding this comment

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

libcufile already depends on __glibc at runtime. My understanding is that sysroot packages are only installed into the build environment. When would libcufile be installed into a build environment or need to be loaded in order to compile a library?

Suggested change
# if there's another, older, sysroot in the same environment;
# libcufile might see the glibc there and fail to load, see
# https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/6745#issuecomment-2503604312
- sysroot_{{ target_platform }} >={{ c_stdlib_version }}

Copy link
Member

Choose a reason for hiding this comment

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

I've tried to read through the discussion in the linked issue. What I'm still confused about it where the sysroot dependency in the test environment is coming from!? Is it added by conda-build or is it some other dependency in that specific environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm assuming it's coming in as a transitive dependency. It's not a common case, but for example, the C/C++ compilers run-depend on the sysroot.

Copy link
Member

@carterbox carterbox Dec 3, 2024

Choose a reason for hiding this comment

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

Sorry for all the pushback, but I don't understand why this would/should be a cufile-specific fix. It seems that any package could have this issue when a trying to install both compilers/sysroot package along with packages that are built for a newer glibc version.

If a downstream package needs to link __glibc and sysroot_linux-64 they should do that themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's libcufile that fails to load in the presence of an older sysroot, so it's libcufile that needs the constraint. Please don't unilaterally change my PR and then merge it without giving me a chance to respond

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I wanted to merge the updated build with new c_stdlib_version pinning. I should have created my own PR instead.

@jakirkham
Copy link
Member

Thanks for raising this Axel! 🙏

There is some broader work to test GLIBC compatibility and fix-up metadata across CUDA packages

Think it make sense to roll this into that broader effort so we can ensure consistency across these packages and make sure no gaps are left

xref: conda-forge/cuda-feedstock#28

@h-vetinari
Copy link
Member Author

I'd like to decouple the extended testing effort from fixing the currently incorrect metadata on aarch here, which forces us to add hacks in other places (e.g. pytorch).

Think it make sense to roll this into that broader effort so we can ensure consistency across these packages and make sure no gaps are left

I'd say you can still track this feedstock (and bring it up to whatever standard you decide on) even if this PR were to be merged?

Mainly I'd like to get rid of the hacks in pytorch. If your overall effort is not too far off (~1-2 weeks) then that's okay. Otherwise this should IMO be merged.

@jakirkham
Copy link
Member

Updating this metadata is high priority for us. It is holding us back in a few places. So we are eager to resolve it. We just discussed next steps earlier today

This work had been blocked until we could use newer images in conda-forge. Now that newer images are fully integrated in conda-forge, we are able to move forward

@h-vetinari
Copy link
Member Author

Great to hear!

@carterbox carterbox changed the title Fix glibc metadata Update glibc pinning to 2.28 for aarch64 Dec 3, 2024
@carterbox carterbox merged commit e67635d into conda-forge:main Dec 3, 2024
5 checks passed
@h-vetinari h-vetinari deleted the glibc branch December 4, 2024 00:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: libcufile has wrong __glibc constraint on aarch
4 participants