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

Avoid naming conflicts by installing shared objects to lib/nest instead of lib #1513

Merged
merged 8 commits into from
Apr 22, 2020

Conversation

clinssen
Copy link
Contributor

@clinssen clinssen commented Apr 14, 2020

Fixes #1222
Fixes #1195

Currently, shared objects (.so files) generated during the NEST Simulator build process are installed to ${CMAKE_INSTALL_PREFIX}/lib. This behaviour is dangerous because it can potentially overwrite existing system libraries. For example, in case the prefix is /usr or ~/.local, any file by the name librandom.so, libprecise.so, libmodels.so and so on, that is already present in /usr/lib or ~/.local/lib, will be overwritten. I heard that a naming conflict did indeed happen in the past with an Infiniband library named libtopology.so, which overlaps with the NEST shared object by the same name.

Generally, libraries are expected to use a unique prefix for their files (in our case, this would be "nest", so our files would be libnesttopology.so, libnestrandom.so, etc.) Other libraries (see ls /usr/lib) seem to prefer to create their own subdirectories. This means that we would have one subdirectory called nest, under which we would place all .so files generated during the build (libtopology.so, librandom.so, ...) In fact, this directory already exists, but is currently only used for user modules: the MyModule example, as well as NESTML generated modules are installed there.

This PR proposes to install all but one .so file in lib/nest to avoid potential naming collisions with existing files in the lib directory—the PyNEST .so file will be unaffected and stay in lib/python3.x/site-packages/nest.

Thanks to @Silmathoron for initially pointing this out! (See nest/nestml#480).

Notes for reviewers:

  • I chose to keep ${CMAKE_INSTALL_LIBDIR} equal to lib rather than set it to lib/nest, because of the PyNEST library that is installed in lib/python3.x/site-packages/nest, i.e. not under lib/nest.
  • Should we remove NEST_MODULE_PATH entirely, as it is now equal to the path for all other shared objects, and thus already in the rpath/LD_LIBRARY_PATH?

@sarakonradi
Copy link
Contributor

@clinssen Thanks a lot for your work. Following @jougs's comment in #1222, I have been working on the documentation of nest_vars.sh and somehow missed this your PR or yours. Sorry if #1515 caused any confusion!

I am happy pausing the work in #1515 until a decision is made here. We can then update the documentation accordingly in order to reflect any potential changes to NEST_MODULE_PATH.

@clinssen
Copy link
Contributor Author

No problem! I don't think there is any overlap in content of the PRs. We should indeed first decide whether or not to keep NEST_MODULE_PATH and then go from there.

@heplesser heplesser added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Apr 20, 2020
@heplesser heplesser added this to the NEST 3.0 milestone Apr 20, 2020
@heplesser heplesser requested review from gtrensch and jougs April 20, 2020 08:43
@clinssen clinssen mentioned this pull request Apr 20, 2020
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Many thanks! I think this looks good in general and I mostly have minor comments.

One slightly bigger thing I noticed is a lot of repetitive changes altering ${CMAKE_INSTALL_LIBDIR} to ${CMAKE_INSTALL_LIBDIR}/nest. Would it be possible to re-set the variable to the latter just once somewhere in the central CMakeLists.txt? I think that would be just as expressive but safer.

cmake/ConfigureSummary.cmake Outdated Show resolved Hide resolved
extras/nest-config.in Show resolved Hide resolved
extras/nest_vars.sh.in Outdated Show resolved Hide resolved
extras/nest_vars.sh.in Outdated Show resolved Hide resolved
extras/nest_vars.sh.in Outdated Show resolved Hide resolved
extras/travis_build.sh Show resolved Hide resolved
clinssen and others added 4 commits April 22, 2020 09:47
Co-Authored-By: Jochen Martin Eppler <[email protected]>
Co-Authored-By: Jochen Martin Eppler <[email protected]>
Co-Authored-By: Jochen Martin Eppler <[email protected]>
@clinssen
Copy link
Contributor Author

Thanks for the feedback! The reason that the value of ${CMAKE_INSTALL_LIBDIR} is not modified, but instead in several places the subdirectory nest is appended, is because of the installation location of pynestkernel.so:

${CMAKE_INSTALL_LIBDIR}/python3.5/site-packages/nest/pynestkernel.so
${CMAKE_INSTALL_LIBDIR}/nest/lib{models,topology,random,...}.so

Because pynestkernel.so does not go under the subdirectory nest, ${CMAKE_INSTALL_LIBDIR} refers to the highest level directory common to both.

This behaviour is consistent with the documentation (https://cmake.org/cmake/help/v3.0/module/GNUInstallDirs.html — see e.g. https://cmake.org/cmake/help/latest/command/install.html for example usage where they append a subdirectory to the CMake variable ${CMAKE_INSTALL_INCLUDEDIR}), so it should hold even if there were no pynestkernel.so and all dynamic libraries went into the nest subdirectory.

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnover! All questions resolved. Approving!

Copy link
Contributor

@gtrensch gtrensch left a comment

Choose a reason for hiding this comment

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

👍 Approved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Status: Done
5 participants