OpenChemistry / avogadrolibs

Avogadro libraries provide 3D rendering, visualization, analysis and data processing useful in computational chemistry, molecular modeling, bioinformatics, materials science, and related areas.
https://two.avogadro.cc/
BSD 3-Clause "New" or "Revised" License
453 stars 172 forks source link

standalone tests file due to removal of Eigen configuration #1430

Open drew-parsons opened 1 year ago

drew-parsons commented 1 year ago

Avogadro version: (please complete the following information from the About box):

Desktop version: (please complete the following information):

Describe the bug

PR #1267 removed Eigen configuration from CMakeLists.txt files throughout avogadrolibs. This makes a problem when running the test standalone against an existing avogadrolibs installation (debian does that for CI testing)

Running cmake against tests/CMakeLists.txt only and then building tests gets an error "Eigen/Dense: No such file or directory" from /usr/include/avogadro/core/vector.h:10. This is because (on debian systems), Eigen is installed under /usr/include/eigen3. So EIGEN3_INCLUDE_DIR is needed.

To Reproduce Steps to reproduce the behavior:

  1. create test dir under tests
    mkdir tests/run_test 
    cd tests/run_test
  2. Configure tests, referring to installed avogadrolib headers
    cmake -DAvogadroLibs_BINARY_DIR=/usr/include/ -DUSE_QT=ON -DUSE_OPENGL=ON -Wno-dev ..
  3. make AvogadroTests
  4. See error

Expected behavior

Should be possible to build and run tests standalone for the purpose of CI testing of an existing avogadrolibs installation.

Screenshots Error is

[  9%] Building CXX object core/CMakeFiles/AvogadroTests.dir/atomtest.o
cd /tmp/autopkgtest.Uz9Zw5/tree/tests/run_test/core && /usr/bin/c++  -I/usr/include/avogadro/core -DGTEST_HAS_PTHREAD=1 -MD -MT core/CMakeFiles/AvogadroTests.dir/atomtest.o -MF CMakeFiles/AvogadroTests.dir/atomtest.o.d -o CMakeFiles/AvogadroTests.dir/atomtest.o -c /tmp/autopkgtest.Uz9Zw5/tree/tests/core/atomtest.cpp
In file included from /usr/include/avogadro/core/atom.h:10,
                 from /usr/include/avogadro/core/bond.h:11,
                 from /usr/include/avogadro/core/molecule.h:14,
                 from /tmp/autopkgtest.Uz9Zw5/tree/tests/core/atomtest.cpp:19:
/usr/include/avogadro/core/vector.h:10:10: fatal error: Eigen/Dense: No such file or directory
   10 | #include <Eigen/Dense>
      |          ^~~~~~~~~~~~~
compilation terminated.
make[3]: *** [core/CMakeFiles/AvogadroTests.dir/build.make:90: core/CMakeFiles/AvogadroTests.dir/atomtest.o] Error 1

Additional context

Can be resolve by reinstating

find_package(Eigen3 REQUIRED)
include_directories(SYSTEM
  ${GTEST_INCLUDE_DIRS}
  ${EIGEN3_INCLUDE_DIR})

in tests/CMakeLists.txt

ghutchis commented 1 year ago

I'd be happy to merge a PR on tests/CMakeLists.txt (I'd do it myself, but today has a lot of meetings and few gaps for code.)

drew-parsons commented 1 year ago

heh no specific rush :) I can patch around it in the meantime.

drew-parsons commented 1 year ago

When I said "can be resolved", that's only in regard to finding Eigen/Dense. There are a couple of other issues before I can run the tests standalone. The steps I gave above, after patching for eigen, give

[100%] Linking CXX executable AvogadroTests
cd /tmp/autopkgtest.Nb2RRo/tree/tests/run_test/core && /usr/bin/cmake -E cmake_link_script CMakeFiles/AvogadroTests.dir/link.txt --verbose=1
/usr/bin/c++ -rdynamic CMakeFiles/AvogadroTests.dir/arraytest.o CMakeFiles/AvogadroTests.dir/atomtest.o CMakeFiles/AvogadroTests.dir/atomtypertest.o CMakeFiles/AvogadroTests.dir/basissettest.o CMakeFiles/AvogadroTests.dir/bondtest.o CMakeFiles/AvogadroTests.dir/coordinateblockgeneratortest.o CMakeFiles/AvogadroTests.dir/coordinatesettest.o CMakeFiles/AvogadroTests.dir/cubetest.o CMakeFiles/AvogadroTests.dir/eigentest.o CMakeFiles/AvogadroTests.dir/elementtest.o CMakeFiles/AvogadroTests.dir/graphtest.o CMakeFiles/AvogadroTests.dir/meshtest.o CMakeFiles/AvogadroTests.dir/moleculetest.o CMakeFiles/AvogadroTests.dir/mutextest.o CMakeFiles/AvogadroTests.dir/neighborperceivertest.o CMakeFiles/AvogadroTests.dir/ringperceivertest.o CMakeFiles/AvogadroTests.dir/spacegrouptest.o CMakeFiles/AvogadroTests.dir/utilitiestest.o CMakeFiles/AvogadroTests.dir/unitcelltest.o CMakeFiles/AvogadroTests.dir/varianttest.o CMakeFiles/AvogadroTests.dir/variantmaptest.o -o AvogadroTests  -lAvogadro::Core /usr/lib/x86_64-linux-gnu/libgtest.a /usr/lib/x86_64-linux-gnu/libgtest_main.a /usr/lib/x86_64-linux-gnu/libgtest.a 
/usr/bin/ld: cannot find -lAvogadro::Core: No such file or directory

So some other patching seems to be needed to get from cmake's Avogadro::Core to -lAvogadroCore. If I add find_package(AvogadroLibs REQUIRED), it resolves Avogadro::Core but gets

[  4%] Linking CXX executable AvogadroTests
cd /projects/avogadrolibs/tests/run_test/core && /usr/bin/cmake -E cmake_link_script CMakeFiles/AvogadroTests.dir/link.txt --verbose=1
/usr/bin/c++ -rdynamic CMakeFiles/AvogadroTests.dir/arraytest.o CMakeFiles/AvogadroTests.dir/atomtest.o CMakeFiles/AvogadroTests.dir/atomtypertest.o CMakeFiles/AvogadroTests.dir/basissettest.o CMakeFiles/AvogadroTests.dir/bondtest.o CMakeFiles/AvogadroTests.dir/coordinateblockgeneratortest.o CMakeFiles/AvogadroTests.dir/coordinatesettest.o CMakeFiles/AvogadroTests.dir/cubetest.o CMakeFiles/AvogadroTests.dir/eigentest.o CMakeFiles/AvogadroTests.dir/elementtest.o CMakeFiles/AvogadroTests.dir/graphtest.o CMakeFiles/AvogadroTests.dir/meshtest.o CMakeFiles/AvogadroTests.dir/moleculetest.o CMakeFiles/AvogadroTests.dir/mutextest.o CMakeFiles/AvogadroTests.dir/neighborperceivertest.o CMakeFiles/AvogadroTests.dir/ringperceivertest.o CMakeFiles/AvogadroTests.dir/spacegrouptest.o CMakeFiles/AvogadroTests.dir/utilitiestest.o CMakeFiles/AvogadroTests.dir/unitcelltest.o CMakeFiles/AvogadroTests.dir/varianttest.o CMakeFiles/AvogadroTests.dir/variantmaptest.o -o AvogadroTests  /usr/lib/x86_64-linux-gnu/libAvogadroCore.so.1.98.0 /usr/lib/x86_64-linux-gnu/libgtest.a /usr/lib/x86_64-linux-gnu/libgtest_main.a -lEigen3::Eigen3 /usr/lib/x86_64-linux-gnu/libgtest.a 
/usr/bin/ld: cannot find -lEigen3::Eigen3: No such file or directory
ghutchis commented 1 year ago

I'm not expert enough in CMake to know how to handle that.

@cryos - do you have a moment to understand why find_package(AvogadroLibs REQUIRED) would add -lEigen3::Eigen3 to linking the tests?

cryos commented 1 year ago

The tests were never designed to be run standalone, I am shocked it works as well as you say. I don't really understand the desire to build the tests against an installed Avogadro, they are designed to be built in the same build tree. The testing subdirectory would really need a fair bit of work to find AvigadroLibs in order to reliably compile.

drew-parsons commented 1 year ago

CI testing. It's important to ensure the whole Linux distribution continues to keep functioning. For instance, when Qt libraries get upgraded.

cryos commented 1 year ago

You are not wrong, but the tests were never designed with this use case in mind and I don't think we should be obligated to change them to accommodate this use case. We could look at making the tests themselves installable, but I really think modernizing the CMake is more important than fixing something that only really worked through luck rather than design.