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

configury: update much configury to OMPI main #1130

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

davidozog
Copy link
Member

This PR aligns SOS's configury with OMPI's main branch ref cebad71e

It removes several configure-time warnings about obsolete autotools features and adds relevant updates for OFI (esp. for HMEM), UCX, and Portals4.

All changes from OMPI/config/* are verbatim, and SOS/configure.ac was run through autoupdate.

@davidozog davidozog self-assigned this May 20, 2024
@davidozog davidozog force-pushed the pr/configury_update branch from 20db94e to c0dd8d0 Compare May 20, 2024 13:52
aligns with ompi main branch ref:
cebad71e4f50d49e486ccd54668cf58698556a7c
@markbrown314
Copy link
Collaborator

markbrown314 commented Feb 6, 2025

I took a cursory look at the code generated and it looks fine to me. Generated code is notoriously difficult to audit for correctness. I will +1 it, but I think we should add some tests (perhaps in CI) to validate that the parameters provided by configure work as advertised.

@bcmIntc
Copy link
Collaborator

bcmIntc commented Feb 7, 2025

I took a cursory look at the code generated and it looks fine to me. Generated code is notoriously difficult to audit for correctness. I will +1 it, but I think we should add some tests (perhaps in CI) to validate that the parameters provided by configure work as advertised.

I agree, I feel we should strive to have test cases either through direct (code) testing or via CI coverage in general.

Copy link
Collaborator

@bcmIntc bcmIntc left a comment

Choose a reason for hiding this comment

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

Did a local build, all logs look cleaner and no errors. Thanks Dave.

@davidozog
Copy link
Member Author

Generated code is notoriously difficult to audit for correctness.

You might be surprised to learn that this code is not generated for the most part - I'm pretty sure many humans have contributed to OMPI's configury over the years. For example, this is the commit history on the UCX config file:
https://github.com/open-mpi/ompi/commits/main/config/ompi_check_ucx.m4

I think we should add some tests (perhaps in CI) to validate that the parameters provided by configure work as advertised.

The existing CI has pretty good coverage on the changes in this PR. Here are some pointers to where that happens in the SOS ci.yml file:

That said, it's not perfect coverage. We're not testing scenarios where the path is not provided explicitly, for example.

opal_case_sensitive_fs_setup.m4 I've checked on my Mac and it looks like a good improvement in my opinion.

I double checked attributes and visibility manually. These are not user-facing so a little tougher to integrate in CI...

Anyway, does that address the concerns or am I missing something?

One issue with this PR is that I integrated with a particularly random upstream commit... I believe at the time I did this work, the latest OMPI release had some configury issue, but I can't recall exactly... Maybe it's better to try to align with a newer release, but I don't see a strong need as long as we take note of where exactly we're cribbing this from.

The license coverage should be good unless we missed something in the past. Only the getdate.sh file is new, and it should be covered by our citation of the https://spdx.org/licenses/BSD-3-Clause-Open-MPI license.

@markbrown314
Copy link
Collaborator

@davidozog thanks for the change set and the insights. Please merge in when ready.

@davidozog davidozog merged commit 4dc8384 into Sandia-OpenSHMEM:main Feb 7, 2025
36 checks passed
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