-
Notifications
You must be signed in to change notification settings - Fork 14
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
exported symbol name pollution test #3
Conversation
@jjhursey Mark was asking if this will become part of Open-MPI mtt tests (or CI tests) once this gets merged. I'm not sure how to determine the answer to that, do you know? |
I don't think this will cause MTT or CI to automatically pick it up. For MTT we would need to add a new test suite for it since I don't think we have one setup for the ompi-tests-public yet. That's pretty easy to do though. Once this is merged in then I can show you how to do that. If you want it in CI then we would need to talk about that a bit. We can probably add it to the OMPI AWS CI setup. It would be fairly easy to run in a container as a separate CI task, or just tack it onto the end of one of the existing builds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a README.md at the package/
level that shows how to run this test? With some notes on expected output for both a successful run and a run where it fails?
That'll help when someone picks it up, and when we integrate it into MTT and/or CI.
fffc678
to
c5a0fc7
Compare
Added a README.md |
FWIW Geoffrey did point me at |
There is still discussion happening on open-mpi/ompi#8132 before it is ready to merge, but (last I heard) the general feeling is that it will make it into the v5 stream. I think a tool like this will become more important in that case since we want to keep the component's exported symbols in the proper namespace. So this test will at least be a warning sign for us. |
This runs from inside a rank and uses ldd on itself to figure out what libraries to examine, then nm on those MPI libraries to look for symbols without some accepted OMPI prefix. Signed-off-by: Mark Allen <[email protected]>
c5a0fc7
to
de8d737
Compare
Minore repush: added "libnbc_" and "tm_" as accepted prefixes. I don't necessarily like "tm_" as a prefix, but since its usage appears to spread over into the prrte repo I wasn't as inclined to add an "ompi_" in front of it to fix it, but rather at least for now accept "tm_" as one of our recognized prefixes. |
Back when I first made this there was some discussion about how best to fix symbol pollution, but I don't think that affects the testcase here. Regardless of how we fix it, I believe this is a correct testcase to identify when we've missed something. The most likely changes I could see needed is if someone objects to the "legal prefixes" list and wants it to be smaller. |
This runs from inside a rank and uses ldd on itself to figure out what
libraries to examine, then nm on those MPI libraries to look for symbols
without some accepted OMPI prefix.
Signed-off-by: Mark Allen [email protected]