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

monitoring lib: rename to ompi_monitoring_prof.so #1390

Merged

Conversation

jsquyres
Copy link
Member

The library that is installed is specific to Open MPI, so put an "ompi_" prefix on it.

Also do some minor line wrappings and cleanups of text.

Signed-off-by: Jeff Squyres jsquyres@cisco.com

@bosilca I added an "ompi_" prefix to the library name so that it is clear that the installed library is specific to Open MPI, and did some other minor text cleanups. Please review. Did you have a milestone in mind for bringing this monitoring stuff over to the v2.x branch?

@bosilca
Copy link
Member

bosilca commented Feb 20, 2016

The library was never intended to be used by a wider community. It was intended for our internal testing purposes (which explains why we put it in the test directory).

I would like to bring this in the 2.x asap.

@jsquyres
Copy link
Member Author

Should it be a noinst ? Right now, it's installed.

Sent from my phone. No type good.

@bosilca
Copy link
Member

bosilca commented Feb 21, 2016

The test itself is installed and will not work without the library.

@jsquyres
Copy link
Member Author

@bosilca If it's a test, why is the test installed?

I.e., you argued both ways: 1) it's a test, and shouldn't be used by anyone, 2) but we install it anyway.

My $0.02 -- it either:

  • Is installed by default and is intended for a wide audience
  • Is not installed by default and is intended for a narrow (developer-only) audience

@jsquyres jsquyres force-pushed the pr/minor-monitoring-library-cleanups branch from a066568 to 795e204 Compare February 22, 2016 19:52
@bosilca
Copy link
Member

bosilca commented Feb 22, 2016

I should have been more clear. The test itself is not installed, but the library is. This library provides the minimal support for using the monitoring PML (via the PMPI interface), it trigger the pvars to generate and save the communication matrix that can be then used for other things (weights for the topology as an example). So, while it is a convenience library, we need it installed in order to be able to play with the topologies.

@jsquyres
Copy link
Member Author

Let's discuss in person this week.

@hjelmn
Copy link
Member

hjelmn commented Feb 23, 2016

:bot:retest:

@jsquyres jsquyres force-pushed the pr/minor-monitoring-library-cleanups branch from 795e204 to 2044ade Compare February 24, 2016 00:17
@ibm-ompi
Copy link

Test passed.

@jsquyres
Copy link
Member Author

Please add a Signed-off-by line to this PR's commit.

Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

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

Let merge this into master.

@jsquyres jsquyres force-pushed the pr/minor-monitoring-library-cleanups branch from 2044ade to 0110260 Compare May 10, 2017 16:03
jsquyres added 2 commits May 10, 2017 09:03
The library that is installed is specific to Open MPI, so put an
"ompi_" prefix on it.

Also do some minor line wrappings and cleanups of text.

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
* Use the proper lib prefix name
* Use the proper extra LDFLAGS

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
@jsquyres jsquyres force-pushed the pr/minor-monitoring-library-cleanups branch from 0110260 to c34ba88 Compare May 10, 2017 16:04
@jjhursey
Copy link
Member

PGI CI failure is an internal problem - should be fixed soon.

@jjhursey
Copy link
Member

bot:ibm:pgi:retest

@jsquyres jsquyres merged commit 9f317f0 into open-mpi:master May 11, 2017
@jsquyres jsquyres deleted the pr/minor-monitoring-library-cleanups branch May 11, 2017 15:11
@jsquyres
Copy link
Member Author

@bosilca I rebased yesterday and merged.

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.

5 participants