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

new cmake harness #205

Open
wants to merge 123 commits into
base: master
Choose a base branch
from
Open

new cmake harness #205

wants to merge 123 commits into from

Conversation

evaleev
Copy link
Owner

@evaleev evaleev commented Jan 26, 2021

cmake build for compiler and library. supersedes #148 .

status board based on #148 (comment)

Major features

  • CMake harness builds compiler, generates and exports library, and builds the exported library. Unlike all-stage CMake #148 does not use superbuild.
    • Building exported lib can be done in superbuild manner (via ExternalProject; default) or as subdirectory (via FetchContent; only useful for testing)
  • removes autotools harness
  • ported to Windows

TODOs

  • FIXED Windows version builds and links to psi4 and produces a clean test suite.
  • Use CMake package components to indicate library features (e.g. orderings, etc.). E.g. component sss indicates all standard orderings and g4 to indicates that ERIs of at least deriv=1 and am=4 have been built, etc.
  • rename Libint2Config.cmake to libint2-config.cmake to maintain consistency with libint 2.6
  • export .pc file for the libtool folks.
  • replace/extend obsolete CMake vars:
  • update documentation
    • update INSTALL (and move to INSTALL.md)
    • sync other docx (wiki)
  • support for non-C++11 API library (int2 as opposed to cxx target)
    • need eigen3 discovery
  • support for Fortran interface

Won't fix in this PR

  • Portable libint #190 solid-harmonic order as runtime option rather than build-time. Beyond this PR, but it will require a slight revamp of the cmake component interface when ready.

@loriab loriab mentioned this pull request Jan 13, 2022
45 tasks
@loriab
Copy link
Collaborator

loriab commented Jan 14, 2022

I'm concerned about how eigen is handled in the library export. The local libint2::libint-eigen3 target is exported at library build time https://github.com/evaleev/libint/pull/205/files#diff-8e69ff48dd421c27038dafc327ded859e9be46b2e1aecec070d8b263a7b03d02R142 so the build-computer path shows up in the libint2-targets-*cmake files (along with other full paths to the libint2::cxx target itself)

# Create imported target Libint2::libint-Eigen3
add_library(Libint2::libint-Eigen3 INTERFACE IMPORTED)

set_target_properties(Libint2::libint-Eigen3 PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "/psi/toolchainconda/envs/py39d/include/eigen3"
)

# Create imported target Libint2::int2
add_library(Libint2::int2 STATIC IMPORTED)

set_target_properties(Libint2::int2 PROPERTIES
  INTERFACE_COMPILE_FEATURES "cxx_std_11"
  INTERFACE_INCLUDE_DIRECTORIES "/psi/gits/libint2-efv/build01/library-install-stage/include"
)

But that library build computer isn't necessarily the computer on which the library is used, and the eigen might be a different distribution. Below are some snippets from the libint2-targets-*cmake from the original #148 without full paths and with generic eigen target. (I guess the eigen target should be detected by find_dependency(Eigen3) in libint2-config.cmake? I admittedly didn't have that working. One could make something like https://github.com/SebWouters/CheMPS2/blob/master/CMake/FindTargetHDF5.cmake to detect a home-grown eigen target since it looks like you don't want to rely upon eigen installed with config files alongside.)

# Create imported target Libint2::cxx
add_library(Libint2::cxx SHARED IMPORTED)

set_target_properties(Libint2::cxx PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "Eigen3::Eigen"
)

set_target_properties(Libint2::cxx PROPERTIES
  IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib/libint2.so.2"
  IMPORTED_SONAME_RELEASE "libint2.so.2"

Am I correct in finding this a concern, or am I missing something?

@evaleev
Copy link
Owner Author

evaleev commented Jan 14, 2022

@loriab I am trying to fix the PR so that CI passes ... note that I partially reverted 76b6605 because the amount of changes needed to replace eri with twobody is fairly massive ... I actually implemented them, CLion ate them, and I am not going to waste the hours again :(

re prepending LIBINT_ to all variables: the goal was to "namespace" all Libint-specific variables (some already are, but the legacy stuff like ERI_MAX_AM etc at not namespaces) just to make subproject builds a bit saner. But again much code depends on the current naming convention for the variables, so perhaps postponing this until it's actually needed is the way to go.

will have a look at the Eigen export concerns later today.

@evaleev
Copy link
Owner Author

evaleev commented Jan 14, 2022

I'm concerned about how eigen is handled in the library export.

so am I ... this is the least fragile I could come up with. For a "header-only" library it's sure difficult to consume it in cmake. Issues:

  • The users want to be able to just unpack it (have had multiple users tell me this) and tell cmake where it is ... re-discovering it during find_package(libint) requires remembering how it was discovered, caching the path, etc. Also, most custom FindEigen3 modules out there (there is no guarantee that there is not one in CMAKE_MODULE_PATH ... :( do not create an imported target, so you would have to make one anyway ...
  • Eigen exported via cmake registers its build tree with per-user cmake registry ... it's just broken: Incorrect installation path saved in install/lib/cmake/libint2/libint2-targets.cmake on Summit #158
  • There is no guarantee that the C++ interface with be header-only in the future, so it may depend on the particular Eigen ABI.

I am not saying that the current approach cannot/should not be improved, but basically we need to support non-cmake-configured eigen3, handle the corner case of the per-user registry with its build tree, etc.

set_target_properties(Libint2::int2 PROPERTIES
INTERFACE_COMPILE_FEATURES "cxx_std_11"
INTERFACE_INCLUDE_DIRECTORIES "/psi/gits/libint2-efv/build01/library-install-stage/include"
)

is this the build-tree targets file? If yes then it's correct. The install-tree targets file should not contain refs to the build tree (see

$<INSTALL_INTERFACE:${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_INCLUDEDIR}>)
)

@evaleev
Copy link
Owner Author

evaleev commented Jan 14, 2022

@loriab the CI jobs currently fail due to some branches in int_am.cmake not having been updated to the new component names:

set(_candidate_${class}_E ${_candidate0_${class}_E})

@loriab
Copy link
Collaborator

loriab commented Jan 14, 2022

@loriab I am trying to fix the PR so that CI passes ... note that I partially reverted 76b6605 because the amount of changes needed to replace eri with twobody is fairly massive ... I actually implemented them, CLion ate them, and I am not going to waste the hours again :(

sounds good, will stick with ERI3 and eri_c3_d1_l4, etc.


re prepending LIBINT_ to all variables: the goal was to "namespace" all Libint-specific variables (some already are, but the legacy stuff like ERI_MAX_AM etc at not namespaces) just to make subproject builds a bit saner. But again much code depends on the current naming convention for the variables, so perhaps postponing this until it's actually needed is the way to go.

thanks, saving for future pass sounds good to me.


I see you bumped CMake min to 3.16, hooray! Any objections to me bumping to 3.18 (released July 2020) to simplify int_am.cmake?


@loriab the CI jobs currently fail due to some branches in int_am.cmake not having been updated to the new component names

The new component names are healed in #233 . I'll get that rebased and moved back to ERI3 and see where the next CI error happens :-)


is this the build-tree targets file? If yes then it's correct. The install-tree targets file should not contain refs to the build tree (see

Right, that was the build-tree targets file, as I had some paths mix-up. But the install-tree file looks the same with full paths.


I am not saying that the current approach cannot/should not be improved, but basically we need to support non-cmake-configured eigen3, handle the corner case of the per-user registry with its build tree, etc.

Ok, I think I have a cmake pattern for a dependency that's not necessarily detected as target so can fulfill those requirements.

The parts that are confusing me are:

  • "requires remembering how it was discovered, caching the path, etc."

But the library building computer (e.g., Fedora package builder) and the consumer of libint2-config.cmake (grad student building mpqc) won't be using the same eigen location, so I'm not seeing why remembering/caching is needed. Is there an eigen version constraint that needs transmitting to the consumer?

  • "There is no guarantee that the C++ interface with be header-only in the future, so it may depend on the particular Eigen ABI."

To clarify, this means that the Libint C++ interface mayn't be purely header-only in future, so the libint2-library-generation eigen version might need to match the eigen version present when, say, psi4 links against a pre-built libint2? This sounds do-able. And actually sounds rather like the Boost usage?


For a "header-only" library it's sure difficult to consume it in cmake.

I've found this, too, via pybind11 and HelPME (an Andy Simmonett project). All the header-only distribution and inclusion flexibility to the user makes the CMake handling a pain.


Thanks for all the comments and commits!

@evaleev
Copy link
Owner Author

evaleev commented Jan 14, 2022 via email

@loriab
Copy link
Collaborator

loriab commented Jan 14, 2022

fyi that the gha file in this branch is using commas and _LIST options that I don't think will process as expected with cmake or int_am as originally designed. The changes below configure as expected with my revised int_am.

https://github.com/evaleev/libint/pull/233/files#diff-cdd48abbd3eb8d1c54077449fc74a8de1f29805d2be5d8e5232b7aab76ea7a6f

@evaleev
Copy link
Owner Author

evaleev commented Jan 14, 2022

fyi that the gha file in this branch is using commas and _LIST options that I don't think will process as expected with cmake or int_am as originally designed. The changes below configure as expected with my revised int_am.

https://github.com/evaleev/libint/pull/233/files#diff-cdd48abbd3eb8d1c54077449fc74a8de1f29805d2be5d8e5232b7aab76ea7a6f

that's fine, the natural cmake separators are fine

Comment on lines +1 to +4
prefix=@CMAKE_INSTALL_PREFIX@
exec_prefix=@CMAKE_INSTALL_PREFIX@
libdir=@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@
includedir=@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_INCLUDEDIR@

Choose a reason for hiding this comment

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

For the record, cmake is really bad at this but it can be worked around: https://github.com/jtojnar/cmake-snips

(The existing autotools build handles this properly, because @libdir@ @includedir@ etc. work properly and support pkg-config's own runtime ${prefix} expansion. CMake developers hate pkg-config because of NIH though.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I think I've fixed this up at 1574ad2 . Below is the diff for generated libint2.pc (with -DCMAKE_INSTALL_LIBDIR=mylib -DCMAKE_INSTALL_INCLUDEDIR=myinclude). Do you need a variable to shift the installed .pc location or is ${CMAKE_INSTALL_LIBDIR}/pkgconfig/ fine?

< libdir=/psi/gits/libint2-efv/build-22/library-install-stage/mylib
< includedir=/psi/gits/libint2-efv/build-22/library-install-stage/myinclude
---
> libdir=${exec_prefix}/mylib
> includedir=${prefix}/myinclude

Copy link

@eli-schwartz eli-schwartz Jan 23, 2022

Choose a reason for hiding this comment

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

${CMAKE_INSTALL_LIBDIR}/pkgconfig is correct (especially since this is only calculated at install time, while pkg-config files are processed as runtime data).

That is the standard location where pkg-config files are expected to be installed.

...

Well, mostly correct. On FreeBSD, it should be libdata/pkgconfig (cmake will install that relative to ${CMAKE_INSTALL_PREFIX} automatically), but that still doesn't take an option. FreeBSD porters can generally move the file after the fact...

…currently includes pre-instantiated Engine code, so should speed up compilation of consuming code tremendously
@loriab
Copy link
Collaborator

loriab commented Jan 19, 2022

It looks like parts of library test 10 may be sensitive to SHGAUSS. All tests pass with standard, but -DLIBINT2_SHGAUSS_ORDERING=gaussian leads to the below:

10:  16     -76.003354058439   1.184157242911e-15   1.418467295368e-12    0.03415
10: compute_2body_fock:precision = 2.22044604925031e-16
10: will set Engine::precision as low as = 1.63035233462786e-07
10: time for integrals = 0.0272398820000002
10: # of integrals = 411041
10:  17     -76.003354058439   1.522487883743e-15   9.885192866120e-13    0.03339
10: HF energy check: passed
10: electric dipole moment check: passed
10: electric quadrupole moment check: passed
10: spherical electric dipole moment check: failed
10: reference: [0.04560184159274555, -0.105312942076475, 0.1316411775955935] 
10: actual: ** sph edipole = -0.105312942035006 0.131641177543759 0.0456018415747894 
10: 
10: spherical electric quadrupole moment check: failed
10: reference: [0.0061560863769835, -0.3191024539954775, 0.5904181715318579, 0.2881006495120795, 0.12874628868708882] 
10: actual: ** sph equadrupole = 0.590418171501538 0.288100649497834 -0.319102453978188 0.12874628867967 0.00615608637620421 
10: 
10: 1-body force check: passed
10: Pulay force check: passed
10: compute_2body_fock:precision = 2.22044604925031e-16
10: will set Engine::precision as low as = 2.21669529173275e-16
10: time for integrals = 0.127778161
10: # of integrals = 4932492
10: 2-body force check: passed
10: nuclear repulsion force check: passed
10: HF force check: passed

@loriab
Copy link
Collaborator

loriab commented Jan 31, 2022

I was experimenting with the compiled cxx interface. Switching the library tests from Libint2::cxx (ho) to Libint2::int2-cxx (compiled) leads to them being linked (via ldd) to int2 but not int2-cxx. Trying to force the compiled interface via

+    target_compile_definitions(
+      hf++-libint2
+      PUBLIC
+        LIBINT2_DOES_NOT_INLINE_ENGINE=1
+        __COMPILING_LIBINT2=1
+      )

leads to build-time errors like

test 9
      Start  9: libint2/hf++/build

9: Test command: /psi/toolchainconda/envs/basenol2/bin/cmake "--build" "/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library-build" "--target" "hf++-libint2"
9: Test timeout computed to be: 1500
9: [1/2] Building CXX object tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o
9: FAILED: tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o 
9: /psi/toolchainconda/envs/basenol2/bin/x86_64-conda-linux-gnu-c++ -DBOOST_ALL_NO_LIB -DLIBINT2_DOES_NOT_INLINE_ENGINE=1 -DSRCDATADIR=\"/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/lib/basis\" -D__COMPILING_LIBINT2=1 -I/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include -I/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/src -I/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library-build/include -I/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library-build/include/libint2 -isystem /psi/toolchainconda/envs/singleeigen/include/eigen3 -isystem /psi/toolchainconda/envs/singleboost/include -fvisibility-inlines-hidden -std=c++17 -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /psi/toolchainconda/envs/basenol2/include -march=native -O3 -DNDEBUG -MD -MT tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o -MF tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o.d -o tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o -c /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/tests/hartree-fock/hartree-fock++.cc
9: In file included from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/boys_cheb7.h:24,
9:                  from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/statics_definition.h:26,
9:                  from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/tests/hartree-fock/hartree-fock++.cc:53:
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/boys_cheb7_v2.h:34:66: error: 'cheb_table_nintervals' was not declared in this scope
9:    34 | template<> double libint2::FmEval_Chebyshev7<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+1)*(interpolation_order+1)]=
9:       |                                                                  ^~~~~~~~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/boys_cheb7_v2.h:34:90: error: 'cheb_table_mmax' was not declared in this scope
9:    34 | template<> double libint2::FmEval_Chebyshev7<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+1)*(interpolation_order+1)]=
9:       |                                                                                          ^~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/boys_cheb7_v2.h:34:110: error: 'interpolation_order' was not declared in this scope
9:    34 | template<> double libint2::FmEval_Chebyshev7<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+1)*(interpolation_order+1)]=
9:       |                                                                                                              ^~~~~~~~~~~~~~~~~~~
9: In file included from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb.h:24,
9:                  from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/statics_definition.h:27,
9:                  from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/tests/hartree-fock/hartree-fock++.cc:53:
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb15.h:15:60: error: 'cheb_table_nintervals' was not declared in this scope
9:    15 | template<> double libint2::TennoGmEval<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+2)*(interpolation_order+1)*(interpolation_order+1)]=
9:       |                                                            ^~~~~~~~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb15.h:15:84: error: 'cheb_table_mmax' was not declared in this scope
9:    15 | template<> double libint2::TennoGmEval<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+2)*(interpolation_order+1)*(interpolation_order+1)]=
9:       |                                                                                    ^~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb15.h:15:104: error: 'interpolation_order' was not declared in this scope
9:    15 | template<> double libint2::TennoGmEval<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+2)*(interpolation_order+1)*(interpolation_order+1)]=
9:       |                                                                                                        ^~~~~~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb15.h:15:128: error: 'interpolation_order' was not declared in this scope
9:    15 | template<> double libint2::TennoGmEval<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+2)*(interpolation_order+1)*(interpolation_order+1)]=
9:       |                                                                                                                                ^~~~~~~~~~~~~~~~~~~
9: ninja: build stopped: subcommand failed.
 9/14 Test  #9: libint2/hf++/build ...............***Failed   12.86 sec
test 10
      Start 10: libint2/hf++/run
Failed test dependencies: libint2/hf++/build
10/14 Test #10: libint2/hf++/run .................***Not Run   0.00 sec

Most likely, I'm just using the compiled interface wrong. But in case this rings a bell about more header manipulations needed, I thought I'd post it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make distclean blows away include/libint2, resulting in build failures Portable libint
5 participants