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

Parameterize and move Fortran MPI bindings modulefiles install location #12649

Merged

Conversation

jsquyres
Copy link
Member

@jsquyres jsquyres commented Jul 1, 2024

See individual commit messages for details.

This PR is split into 2 commits so that we can cherry-pick one of them to the v5.0.x branch and still leave the default install location as $libdir.

Refs #12600

FYI @minrk

@ggouaillardet
Copy link
Contributor

The first commit (e.g. add --with-mpi-moduledir option) looks good to me.

About the second one, well, you know what they say "if it ain't broke, don't fix it".
It won't change anything for those who are already doing the right thing (e.g. have correctly configured MPI wrappers, and either use them or get the flags from mpifort --showme). But it might break stuff for those who did it the wrong way but somehow figured out how to get the job done.
In all fairness, those who do not understand what they are doing, or use borked wrappers, or assume the Fortran module files are always in $PREFIX/include and sing kumbaya might get the illusion things are now working out of the box.
In my not so humble opinion, that is not an upside for me (quite the opposite indeed), so regarding the second commit, I invite you to try reaching a consensus with other folks, look for a tie-breaker or simply disregard my opinion.

@minrk
Copy link
Contributor

minrk commented Jul 2, 2024

Thanks, both changes look great to me and I think are clearly the right choice by default.

Parameterize the install location of Fortran MPI bindings modulefiles
via the configure --with-mpi-moduledir CLI option (default to $libdir,
per all prior versions of Open MPI).

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres jsquyres force-pushed the pr/move-fortran-modulefiles-install branch from 8e149fd to 9fb2160 Compare July 2, 2024 11:47
@jsquyres
Copy link
Member Author

jsquyres commented Jul 2, 2024

I had the docs not quite correctly split across the 2 commits; I just fixed that and re-pushed.

@ggouaillardet makes some fair points:

But it might break stuff for those who did it the wrong way but somehow figured out how to get the job done.

I don't have too much heartbreak for breaking these use cases. It may even cause such cases to fix what they're doing wrong (i.e., switch to using a supported method).

In all fairness, those who do not understand what they are doing, or use borked wrappers, or assume the Fortran module files are always in $PREFIX/include and sing kumbaya might get the illusion things are now working out of the box.

I hear what you're saying. I think your 2 examples slightly contradict each other, though: the first one worries that we're going to break people who are doing the wrong thing (but who currently still manage to work); the second one worries that people doing the wrong thing are now going to work.

I guess my main thought is: if the Fortran world's conventions have changed to put modulefiles in $includedir, shouldn't we also change to do that? We're not trying to be Fortran trendsetters here -- we should be trying to do what the rest of the Fortran community does.

That being said, I don't have too strong of opinions here -- I think it would be ok to make this change at an Open MPI major release (i.e., 6.0), but if there's strong opposition to it, then I'll be happy enough with just adding --with-mpi-moduledir and not changing the default.

So we have @jsquyres and @ggouaillardet's opinions here -- anyone else want to chime in?

@rhc54
Copy link
Contributor

rhc54 commented Jul 2, 2024

Just a suggestion: have you considered asking the other packagers on your mailing list? Probably a concern about breaking them without warning - I haven't seen anyone else from that community raising or commenting on this issue.

@jsquyres
Copy link
Member Author

jsquyres commented Jul 6, 2024

Fair point. I posted to the packagers list several days ago with no response yet: https://www.mail-archive.com/[email protected]//msg00098.html

@jsquyres
Copy link
Member Author

jsquyres commented Jul 8, 2024

We talked on the RM call today:

  • It seems like everyone feels the "let's not change the default" cautious attitude. So let's do that.
  • I'll remove the 2nd commit (which changes the default), and just keep the first commit (that adds the --with-mpi-moduledir configure CLI option).

@jsquyres jsquyres force-pushed the pr/move-fortran-modulefiles-install branch from 9fb2160 to 7338594 Compare July 8, 2024 19:40
@jsquyres
Copy link
Member Author

jsquyres commented Jul 9, 2024

@ggouaillardet Since we ended up not changing the default, could you approve this PR?

Thanks!

@jsquyres jsquyres merged commit b4bf0f8 into open-mpi:main Jul 9, 2024
13 checks passed
@jsquyres jsquyres deleted the pr/move-fortran-modulefiles-install branch July 9, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants