Fix cmake install #3228

junghans · 2019-10-02T16:34:58Z

Supersedes #3226

Description of changes:

fix the CMake build system to include libraries in the list of installed files and test it in a dedicated CI job
fix the versioning of Cython .so shared objects
simplify the CMake tests

codecov · 2019-10-02T16:50:58Z

Codecov Report

Merging #3228 into 4.1 will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##             4.1   #3228   +/-   ##
=====================================
  Coverage     85%     85%           
=====================================
  Files        530     530           
  Lines      26025   26025           
=====================================
  Hits       22157   22157           
  Misses      3868    3868

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ffa392...4035463. Read the comment docs.

mkuron · 2019-10-02T18:09:36Z

Needs cherry pick into master branch too.

junghans · 2019-10-02T19:08:33Z

Needs cherry pick into master branch too.

The merge seems to work as well.

junghans · 2019-10-11T19:27:35Z

Ping

jngrad

Please edit the original post to explain what this PR is achieving.

jngrad · 2019-10-12T16:51:49Z

CMakeLists.txt

-               "${PROJECT_VERSION}")
-if(NOT ${SOVERSION} MATCHES "^[1-9]+$")
-  message(FATAL_ERROR "Could not determine SOVERSION from ${PROJECT_VERSION}")
-endif(NOT ${SOVERSION} MATCHES "^[1-9]+$")


Why is SOVERSION not needed anymore?

All libraries get to installed in the python site-packages directory and in there so versioning isn’t required anymore. Plus half of the new library miss it anyhow.

jngrad · 2019-10-12T16:52:09Z

src/core/io/mpiio/CMakeLists.txt

@@ -1,3 +1,4 @@
 add_library(mpiio SHARED mpiio.cpp)
 target_include_directories(mpiio PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
 target_link_libraries(mpiio PRIVATE EspressoConfig EspressoCore MPI::MPI_CXX)
+install(TARGETS mpiio LIBRARY DESTINATION ${PYTHON_INSTDIR}/espressomd)


What is the purpose of the new install() line here and in the other submodules?

These libraries were missing from the installation and hence made the espressomd module unusable.

jngrad · 2019-10-13T14:21:31Z

@junghans Thanks for reporting this issue. I will add a test to detect this type of regression.

@mkuron While trying to reproduce the issue on Ubuntu, I found another flaw in the build system: the CMAKE_INSTALL_RPATH is not updated when the user provides a custom DESTDIR during installation. This should have been caught by our CMake tests, unfortunately I did not configure the destdir test properly. I've investigated the issue further and came to the conclusion that we cannot actually use make install DESTDIR="path/to/install" and should warn the user explicitly against using that command, and only use cmake .. -DCMAKE_INSTALL_PREFIX="path/to/install". Here is a MWE:

# with DESTDIR and no CMAKE_INSTALL_PREFIX, PATH is right but RPATH is wrong
cmake ..
make -j$(nproc)
make install DESTDIR="/tmp/espresso-1"

-- Installing: /tmp/espresso-1/usr/local/lib/python3.6/site-packages/espressomd/EspressoScriptInterface.so
-- Set runtime path of "/tmp/espresso-1/usr/local/lib/python3.6/site-packages/espressomd/EspressoScriptInterface.so" to "/usr/local/lib/python3.6/site-packages/espressomd"

# with no DESTDIR and CMAKE_INSTALL_PREFIX, PATH is right and RPATH is right
cmake .. -DCMAKE_INSTALL_PREFIX="/tmp/espresso-2"
make -j$(nproc)
make install

-- Installing: /tmp/espresso-2/lib/python3.6/site-packages/espressomd/EspressoScriptInterface.so
-- Set runtime path of "/tmp/espresso-2/lib/python3.6/site-packages/espressomd/EspressoScriptInterface.so" to "/tmp/espresso-2/lib/python3.6/site-packages/espressomd"

# with DESTDIR and CMAKE_INSTALL_PREFIX, PATH is wrong and RPATH is wrong
cmake .. -DCMAKE_INSTALL_PREFIX="/tmp/espresso-3"
make -j$(nproc)
make install DESTDIR="/tmp/espresso-4"

-- Installing: /tmp/espresso-4/tmp/espresso-3/lib/python3.6/site-packages/espressomd/EspressoScriptInterface.so
-- Set runtime path of "/tmp/espresso-4/tmp/espresso-3/lib/python3.6/site-packages/espressomd/EspressoScriptInterface.so" to "/tmp/espresso-3/lib/python3.6/site-packages/espressomd"

When RPATH is wrong, we get the following error message:

$> ls /tmp/espresso-1/usr/local/lib/python3.6/site-packages/espressomd/EspressoCore.so 
/tmp/espresso-1/usr/local/lib/python3.6/site-packages/espressomd/EspressoCore.so
$> /tmp/espresso-1/usr/local/bin/pypresso 
Python 3.6.8 (default, Oct  7 2019, 12:59:55) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import espressomd
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/tmp/espresso-1/usr/local/lib/python3.6/site-packages/espressomd/__init__.py", line 23, in <module>
    import espressomd._init
ImportError: EspressoCore.so: cannot open shared object file: No such file or directory

jngrad · 2019-10-13T15:15:32Z

I have written most of the logic for the new test, however I can't install 4.1.1rc with or without this PR:

cmake .. -DCMAKE_INSTALL_PREFIX=/tmp/espresso-unit-tests
make -j$(nproc)
make install
./pypresso -c 'from espressomd.cluster_analysis import ClusterStructure;print("ok1")'
/tmp/espresso-unit-tests/bin/pypresso -c 'import espressomd;from espressomd.cluster_analysis import ClusterStructure;print("ok2")'

ok1
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: dynamic module does not define module export function (PyInit_cluster_analysis)

The cluster_analysis.so file is a C++ shared object, which is not meant to be imported from Python. To access its classes, we use the ScriptInterfaceHelper in cluster_analysis.py. In the build folder, the .so and .py files are in different folders so there is no name collision. However, when we install espresso, the .so and .py end up in the same folder and Python thinks the .so file is a Cython shared object, which is expected to have a PyInit_* function. When I rename cluster_analysis.py to e.g. clusters_analysis.py, there is no import error.

@KaiSzuttor could this be a regression of our efforts to restructure the core folders?

mkuron · 2019-10-13T16:46:32Z

@mkuron While trying to reproduce the issue on Ubuntu, I found another flaw in the build system: the CMAKE_INSTALL_RPATH is not updated when the user provides a custom DESTDIR during installation.

Just get rid of DESTDIR. That's non-standard anyway and everyone should use CMAKE_INSTALL_PREFIX instead.

Installing ESPResSo with `make install DESTDIR="/path/to/es"` will install files in the wrong folder and set an incorrect runtime path in Cython shared objects, causing import errors. See full report: espressomd#3228 (comment)

When installing ESPResSo, the C++ shared objects and Cython shared objects are stored in the same folder. Since they have the same .so extension, when importing espressomd.cluster_analysis, the file cluster_analysis.py is loaded and the file cluster_analysis.so is automatically considered a Cython shared object, and Python fails to find the Cython-specific symbol PyInit_cluster_analysis. This is remediated by giving a different name to the C++ shared object.

junghans · 2019-10-14T01:08:14Z

DESTDIR is really only for packaging, i.e. to make a tarball with a certain RPATH encoded without installing it there, so don't use it here.

I would replace make install by cmake --install to allow for testing with ninja as well.

jngrad · 2019-10-14T07:58:48Z

I would replace make install by cmake --install to allow for testing with ninja as well.

Nice feature! But this seems to be CMake 3.15-specific, which has been released very recently. We still support 3.4, and consider bumping to 3.10 in the next release.

Install the files in a different directory and check the python and tutorials/samples tests can still run with the installed files. Please note that pypresso will experience a major slowdown in CI when located outside the build directory, hence the 3h timeout. This job is meant to be run only during releases and in PRs that add new core modules.

junghans · 2019-10-14T14:07:52Z

I would replace make install by cmake --install to allow for testing with ninja as well.

Nice feature! But this seems to be CMake 3.15-specific, which has been released very recently. We still support 3.4, and consider bumping to 3.10 in the next release.

Then if clause it depending on the CMale version, in CMake-3.4 you could at least use cmake --build instead of make all.

Takes more than 1 hour to run in CI when pypresso is located outside the build directory.

Installing ESPResSo with `make install DESTDIR="/path/to/es"` will install files in the wrong folder and set an incorrect runtime path in Cython shared objects, causing import errors. See full report: espressomd#3228 (comment)

3248: Fix cmake install on dev branch r=KaiSzuttor a=jngrad Porting #3228 to the dev branch Description of changes: - fix the CMake build system to include libraries in the list of installed files and test it in a dedicated CI job - fix the versioning of Cython .so shared objects - simplify the CMake tests 3251: Particle data r=jngrad a=fweik Fixes #3157. Description of changes: - Split definition of `struct Particle` from rest of particle data. Co-authored-by: Christoph Junghans <[email protected]> Co-authored-by: Jean-Noël Grad <[email protected]> Co-authored-by: Florian Weik <[email protected]>

The installed files only exist if the Python bindings are enabled.

The clang:6.0 job fails due to undefined ASAN symbols. The logic to fix that error is in the pypresso wrapper.

3228: Fix cmake install Description of changes: - fix the CMake build system to include libraries in the list of installed files and test it in a dedicated CI job - fix the versioning of Cython .so shared objects - simplify the CMake tests

jngrad · 2019-10-15T16:50:05Z

This PR was merged into the open PR for the 4.1.1 release candidate.

junghans added 3 commits October 2, 2019 10:14

cmake: install missing targets

1f222ce

cmake: remove SOVERSION

0ea5308

cmake: fix permission of python module install

b7b7d3e

junghans mentioned this pull request Oct 2, 2019

cmake: add soversion to libH5mdCore #3226

Closed

mkuron approved these changes Oct 2, 2019

View reviewed changes

jngrad added this to the Espresso 4.1.1 milestone Oct 11, 2019

jngrad reviewed Oct 12, 2019

View reviewed changes

jngrad added 2 commits October 12, 2019 19:44

Document CMake logic

0a1ecba

Fix version nomenclature in Sphinx docs

9e8aa4b

jngrad added BugFix CMake labels Oct 12, 2019

jngrad added 2 commits October 13, 2019 19:34

Deprecate DESTDIR install method

e0ad9ed

Installing ESPResSo with `make install DESTDIR="/path/to/es"` will install files in the wrong folder and set an incorrect runtime path in Cython shared objects, causing import errors. See full report: espressomd#3228 (comment)

jngrad force-pushed the 4.1 branch 2 times, most recently from 6d09834 to 475fa1a Compare October 13, 2019 19:47

jngrad force-pushed the 4.1 branch 2 times, most recently from 01ca3c8 to 357eee7 Compare October 14, 2019 08:51

jngrad force-pushed the 4.1 branch from 3fe644e to 759b80f Compare October 14, 2019 13:24

Skip tutorial ferrofluid part 3 (timeout)

511584f

Takes more than 1 hour to run in CI when pypresso is located outside the build directory.

jngrad force-pushed the 4.1 branch from 9d2ef64 to 511584f Compare October 15, 2019 08:02

jngrad approved these changes Oct 15, 2019

View reviewed changes

jngrad mentioned this pull request Oct 15, 2019

Fix cmake install on dev branch #3248

Merged

jngrad added 4 commits October 15, 2019 17:52

Remove redundant PYTHONPATH

6568c0f

Simplify CMake test logic

e11198b

The installed files only exist if the Python bindings are enabled.

Remove unused DESTDIR variable

13b350e

Cannot test python3 in CI

4035463

The clang:6.0 job fails due to undefined ASAN symbols. The logic to fix that error is in the pypresso wrapper.

jngrad closed this Oct 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cmake install #3228

Fix cmake install #3228

junghans commented Oct 2, 2019 •

edited by jngrad

Loading

codecov bot commented Oct 2, 2019 •

edited

Loading

mkuron commented Oct 2, 2019

junghans commented Oct 2, 2019

junghans commented Oct 11, 2019

jngrad left a comment

jngrad Oct 12, 2019

junghans Oct 12, 2019

jngrad Oct 12, 2019

junghans Oct 12, 2019

jngrad commented Oct 13, 2019

jngrad commented Oct 13, 2019

mkuron commented Oct 13, 2019

junghans commented Oct 14, 2019

jngrad commented Oct 14, 2019

junghans commented Oct 14, 2019

jngrad commented Oct 15, 2019

Fix cmake install #3228

Fix cmake install #3228

Conversation

junghans commented Oct 2, 2019 • edited by jngrad Loading

codecov bot commented Oct 2, 2019 • edited Loading

Codecov Report

mkuron commented Oct 2, 2019

junghans commented Oct 2, 2019

junghans commented Oct 11, 2019

jngrad left a comment

Choose a reason for hiding this comment

jngrad Oct 12, 2019

Choose a reason for hiding this comment

junghans Oct 12, 2019

Choose a reason for hiding this comment

jngrad Oct 12, 2019

Choose a reason for hiding this comment

junghans Oct 12, 2019

Choose a reason for hiding this comment

jngrad commented Oct 13, 2019

jngrad commented Oct 13, 2019

mkuron commented Oct 13, 2019

junghans commented Oct 14, 2019

jngrad commented Oct 14, 2019

junghans commented Oct 14, 2019

jngrad commented Oct 15, 2019

junghans commented Oct 2, 2019 •

edited by jngrad

Loading

codecov bot commented Oct 2, 2019 •

edited

Loading