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

orted-mpir: add version to shared library #8892

Merged
merged 1 commit into from May 3, 2021
Merged

Conversation

ahesford
Copy link

@ahesford ahesford commented Apr 29, 2021

Because orted-mpir is installed in the public library direrctory, it should have a version. Assign the library libopen_rte_so_version to be consistent with the rest of the RTE.

This is important, e.g., for Void Linux, which tracks shared library versions to avoid broken dependencies. Void splits the core libraries into a libopenmpi package so other packages can link against it and avoid pulling in the entire OpenMPI runtime should users not with to use the capability. The runtime modules in /usr/lib/opempi are shipped with the broader openmpi package, and the package system must track the libopen-orted-mpir.so dependency between these two packages at least.

This may need backporting to v4.0.x and earlier.

bot:notacherrypick

@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@awlauria
Copy link
Contributor

ok to test

@awlauria
Copy link
Contributor

awlauria commented Apr 29, 2021

@ahesford can you re-push after your amend your commit with -s.

git commit --amend -s should do it.

@ahesford
Copy link
Author

Commit amended

@bwbarrett
Copy link
Member

Why are we installing that library at all? Isn't it supposed to be a helper library to avoid CFLAGS?

@ahesford
Copy link
Author

I believe the only reason this library exists is to avoid CFLAGS.

The linker seems to pull it as a dependency in several runtime mca modules in /usr/lib/openmpi and the public libraries

/usr/lib/libmpi.so.40.30.1
/usr/lib/libmpi_usempif08.so.40.30.0
/usr/lib/libmpi_mpifh.so.40.30.0
/usr/lib/libopen-rte.so.40.30.1

@awlauria
Copy link
Contributor

Yes, it is only to avoid the global CFLAGS, which would optimize out certain MPIR functionality in optimized builds.

It was done to not blow away CFLAGS for other files that still want the CFLAGS optimizations.

@bwbarrett
Copy link
Member

Yes, it is only to avoid the global CFLAGS, which would optimize out certain MPIR functionality in optimized builds.

It was done to not blow away CFLAGS for other files that still want the CFLAGS optimizations.

Then this PR is the wrong fix. Rather than setting a version, we should make this library a noinst_ library and not worry about versioning.

@ahesford
Copy link
Author

I made the noinst change. If this is definitely the right solution, I can squash the commits for merge. Otherwise, if somebody has a compelling reason to install the library, we can drop the second commit and use the original solution.

@awlauria
Copy link
Contributor

LGTM.

The orted-mpir library was split to avoid global CFLAGS, which might
optimize out MPIR functionality. The library has been made a `noinst`
libtool convenience library to prevent its installation or appearance as
a dependency of other libraries.

Signed-off-by: Andrew J. Hesford <ajh@sideband.org>
@bwbarrett
Copy link
Member

I made the noinst change. If this is definitely the right solution, I can squash the commits for merge. Otherwise, if somebody has a compelling reason to install the library, we can drop the second commit and use the original solution.

The first patch is not necessary. noinst libraries should not be versioned in OMPI. And if we are going to version them, it shouldn't piggyback off of another library's version string, but have its own. So please don't squash; just drop the first patch.

@ahesford
Copy link
Author

The second patch reverted the first and made the library noinst, so squashing was equivalent to dropping the original patch. All done.

@awlauria
Copy link
Contributor

awlauria commented May 3, 2021

@ahesford did you want to cherry-pick this to the v4.0.x branch?

@ahesford
Copy link
Author

ahesford commented May 3, 2021

I think it makes sense to cherry-pick to v4.0.x. Is there something special I need to do to facilitate this?

@awlauria
Copy link
Contributor

awlauria commented May 3, 2021

You should just need to add -x to the cherry-pick.

@ahesford
Copy link
Author

ahesford commented May 3, 2021

OK, #8912

@bwbarrett bwbarrett merged commit fe63790 into open-mpi:v4.1.x May 3, 2021
@ahesford ahesford deleted the mpir branch May 3, 2021 19:11
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.

None yet

5 participants