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

BLD: Check for glibc symbols in redist binaries #97

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

carterbox
Copy link
Member

@carterbox carterbox commented Nov 27, 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.

In this PR, I have added a script developed by @mtjrider which checks for glib symbols in the shipped binaries.

In theory this means we don't have to match the glibc version with the os_version anymore, but let's get explicit permission from the channel before doing that. Brought this up at conda-forge core meeting. Response was good.

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Nov 27, 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.

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

@carterbox
Copy link
Member Author

Haha! It works!

Comment on lines -6 to -9
# NOTE: For binary redists os_version should match oldest supported glibc
os_version:
linux_64: alma8
linux_aarch64: alma8
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that it's not necessary to set the os_version if we are checking the binaries for glibc symbols.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gonna revert this until we get approval from conda-forge/core at a future meeting. Or maybe just wait until that meeting before merging this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussing this tomorrow at meeting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feedback from core meeting what that checking symbols explicitly is better than testing if loading crashes the library.

@jakirkham jakirkham mentioned this pull request Dec 3, 2024
5 tasks
@carterbox carterbox closed this Dec 3, 2024
@carterbox carterbox deleted the glib-check branch December 3, 2024 17:59
@carterbox carterbox restored the glib-check branch December 3, 2024 21:15
@carterbox carterbox reopened this Dec 3, 2024
@jakirkham
Copy link
Member

jakirkham commented Dec 5, 2024

Thanks Daniel! 🙏

We also have some dlopen tests in a few packages (below). Is this something we would want to capture here?

@carterbox
Copy link
Member Author

For clarity, if we were to add this test to cudnn it would not check if cudnn uses dlopen. It would only use dlopen to load cudnn? How is that better than link checking?

@carterbox carterbox mentioned this pull request Dec 5, 2024
5 tasks
@carterbox carterbox marked this pull request as ready for review December 11, 2024 18:52
@carterbox
Copy link
Member Author

@conda-forge-admin, please rerender

@carterbox
Copy link
Member Author

Thanks Daniel! 🙏

We also have some dlopen tests in a few packages (below). Is this something we would want to capture here?

This test already exists in this feedstock.

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.

3 participants