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.

Sign up for GitHub

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

Jump to bottom

Require GLIBC 2.17+ in MPI compilers #147

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Require GLIBC 2.17+ in MPI compilers #147

merged 3 commits into from
Apr 9, 2024

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Apr 5, 2024 •
edited

Fixes #143

Adds sysroot_{{ target_platform }} 2.17 to requirements/build of MPI compiler packages. This will make GLIBC 2.17 a minimum requirement at install time.

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.

@jakirkham
MNT: Re-rendered with conda-build 24.3.0, conda-smithy 3.34.1, and co…
6fd5724 
…nda-forge-pinning 2024.04.05.13.18.18
@jakirkham
Require GLIBC 2.17+ in MPI compiler packages
f6dac20 
Explicitly add the `sysroot` to the compiler packages
@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 jakirkham mentioned this pull request Apr 5, 2024
Open
1 task
@jakirkham
Copy link
Member Author

How do we handle build number bumps for these packages?

beckermr
beckermr previously requested changes Apr 5, 2024
View reviewed changes
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 should use the new stdlib stuff, right?

@jakirkham
Copy link
Member Author

We are already using sysroot above. This is just trying to fix a user problem

If someone wants to migrate this feedstock great, but please handle that separately

@beckermr
Copy link
Member

beckermr commented Apr 5, 2024

Totally make sense @jakirkham. Indeed a separate PR is better! LGTM and thank you!

@jakirkham jakirkham marked this pull request as draft April 5, 2024 21:06
@jakirkham
Copy link
Member Author

Marking as draft until we figure out how to handle the build/number here. Just want to make sure we handle that correctly

@beckermr
Copy link
Member

beckermr commented Apr 5, 2024 •
edited

What is the question about the build number specifically?

I'd argue we bump it to get a working build and also apply patches to the problematic older builds.

jakirkham
jakirkham commented Apr 5, 2024
View reviewed changes
recipe/meta.yaml
@@ -1,7 +1,7 @@
{% set version = "5.0.2" %}
{% set major = version.rpartition('.')[0] %}
{% set cuda_major = (cuda_compiler_version|default("11.8")).rpartition('.')[0] %}
{% set build = 0 %}
{% set build = 1 %}
Copy link
Member Author

Choose a reason for hiding this comment

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

Can see there are some things happening with the build/number in this recipe

So not sure if just bumping here is sufficient. Or if something more needs to be done

Copy link
Member

Choose a reason for hiding this comment

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

Ah got it. I hope this is all that is needed, otherwise the bot PRs have been wrong!

Copy link
Member Author

Choose a reason for hiding this comment

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

Think you are most likely right

Just want to double check in case I'm missing something. I may just be a worrywart 😅

@leofang
Copy link
Member

leofang commented Apr 8, 2024

Thanks, @jakirkham, this looks good to me. Is there anything else needed before merging?

@leofang leofang mentioned this pull request Apr 8, 2024
Closed
3 tasks
@jakirkham
Copy link
Member Author

If the build number bump looks good, then this should be ready to go

@jakirkham jakirkham marked this pull request as ready for review April 9, 2024 19:31
@leofang
Copy link
Member

leofang commented Apr 9, 2024

Thanks, John!

@leofang leofang merged commit eaeade0 into conda-forge:main Apr 9, 2024
10 checks passed
@jakirkham jakirkham deleted the req_glibc_217_mpi_compilers branch April 9, 2024 20:04
@jakirkham
Copy link
Member Author

Thanks Leo and Matt! 🙏

@jakirkham jakirkham mentioned this pull request Apr 9, 2024
Merged
5 tasks
@jakirkham
Copy link
Member Author

We should use the new stdlib stuff, right?

Handling the {{ stdlib('c') }} pieces in PR: #149

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beckermr beckermr beckermr left review comments

@leofang leofang leofang approved these changes

@astrofrog astrofrog Awaiting requested review from astrofrog astrofrog is a code owner

@bekozi bekozi Awaiting requested review from bekozi bekozi is a code owner

@dalcinl dalcinl Awaiting requested review from dalcinl dalcinl is a code owner

@minrk minrk Awaiting requested review from minrk minrk is a code owner

@msarahan msarahan Awaiting requested review from msarahan msarahan is a code owner

@ocefpaf ocefpaf Awaiting requested review from ocefpaf ocefpaf is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mpifort 5.0.1 fails with undefined refs to memcpy@GLIBC_2.14 and clock_gettime@GLIBC_2.17
3 participants
@jakirkham @beckermr @leofang