-
Notifications
You must be signed in to change notification settings - Fork 98
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, round 3 #233
base: features/new-cmake-harness
Are you sure you want to change the base?
new cmake harness, round 3 #233
Conversation
00cb557
to
61e284b
Compare
f89be3c
to
b13667b
Compare
…re validation. start INSTALL.md option documentation.
cb89c05
to
ed49e12
Compare
…. this should help with random deviations observed in eri validation
…like tests/eri/test.cc
@loriab CI passes! how painful would be to revert to cmake 3.16? |
Thanks!
It won't be too bad. I figured I'd keep it at 3.18 for int_am readability for now (2 lines to compute each max instead of ~7) then revert to 3.16 at the end. I have updated some Python and Boost things that could be used only >=3.15. |
Sorry to leave CI broken after you so kindly fixed it. This shows a strategy for separating machine-path-dependent targets (that you described the need for but are unsuitable for distribution) from the usual redistributable targets. This also fixes the full paths in the installed target tree for the latter. I'm still working on shifting eigen3 detection, so CI has no chance at the moment. |
@loriab no worries BTW I am not sure I understand why we need to have the "nonlocal" (path-independent) targets. If you allow the user to discover Eigen3 on their own or provide their own Eigen3 target there is no way to make sure that it is configured correctly (e.g. the one used by Libint to pre-instantiate Engine used Eigen3 configured to need BLAS/LAPACK ... there is no way to test Eigen3's configuration without much This is a common scenario for C++ and even for C code. E.g. CMake's standard MPI targets are ABI-specific, so it is not possible to say Is this a requirement motivated by package maintenance?? |
Yes, I'm thinking of packaging and accommodating the up to three computers that build library source, build library, and build library consumer. All I really want in the end is:
I think I'm reading code right that the Libint2 cxx interface at present is header only. So the detection at library-build time in src/lib/libint/CMakeLists.txt.export is perfunctory so that the eigen target can be recorded in libint2-config.cmake as a dependency of Libint2::cxx interface library target. The eigen code only gets compiled within the libint repo for the tests that use the cxx interface. If I have all that right, I feel on pretty firm cmake footing that there shouldn't be full paths in the config file and one doesn't generally install external dependency targets (https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-relocatable-packages). The latter is unusual enough that there's a new v3.21 capability to allow it (https://cmake.org/cmake/help/latest/command/install.html#installing-runtime-dependencies). I'd expect the eigen detection to happen in a Your plan of non-header-only Libint2 cxx interface surprised me. I gather that the int-cxx-shared target becomes a tangible library with compiled code, not just an interface library. I accept that the same eigen code and offloading defines ought to be used for both libint-building and libint-consumer-building. This is fairly easy when packagers control both the libint2-building and the e.g., psi4-building steps because they'll use common eigen conditions. I'm not sure how it should work for the more general cmake case, though. Just vendorizing the eigen code into include/libint2/eigen/ doesn't work because it's not just the eigen code you want shared between libint2 and the qc program but also the offloading defines and attendant library linking. Since you seemed to assume that libint2 and QC consumer were build on the same computer anyways by the full paths to eigen, I figured a workaround to preserve normal cmake relocatability for the header-only cxx interface and accommodate the common-computer non-header-only cxx interface you describe would be to separate the targets so packagers can decline to install the latter while users can request the latter. I hope I'm understanding your points and plans better in these posts. Mostly I've been focusing on the existing header-only state. Thanks for you patience. Pinging @susilehtola in case there's a packaging aspect/constraint that I'm missing, being mostly conda-focused. |
@loriab due to the header-only inefficiencies the plan to make C++ API a proper (compiled) library has been around for awhile (for as long as engine.impl.h existed). There will be no impact on current API users (like Psi), and switching to the compiled version will be trivial: I agree that for careful users the ABI issues will not appear. But I'm just giving you a heads-up for what's likely to happen (and has happened to me multiple times before ... so experts are not immune). Since ABI mismatches are impossible to detect at build-time and in general extremely painful to diagnose the ABI mismatches in my book are more evil than hardwiring to the particular target instance ... perhaps the hardwiring should be the default with an opt-out for experts? OR the other way around? (the latter makes less sense to me: the only reason to hardwire search paths is for an average user, not an expert). P.S. Eigen is just an fn pain. I'd love to be able to just restrict to config-equipped Eigen but the amount of pushback I get along the lines of "but Eigen is a header-only library!!!! I should not need to install it" is tiresome. |
@loriab btw I made the |
# Conflicts: # src/lib/libint/CMakeLists.txt.export
@evaleev, I think this is ready for review. I'm still working on the INSTALL.md docs, but I'm satisfied with the technical situation. If you approve of the general structure and solutions, I'd like to advise packagers that it's ready to test soon, as the problems they find are probably in my court rather than yours. Let me know if you'd like to Zoom to talk over reasoning and concerns in real time for efficiency. |
At present, this PR is so EFV can oversee proposed changes to #205. It configures, but I haven't tried so much as building the compiler.Added 19 Jan
Boost::boost
to h-oBoost::headers
allowed in CMake 3.15add_library(libint-libcompiler STATIC
after build failed withBUILD_SHARED_LIBS=ON
find_project(Python
and result vars since CMake Python detection improved much around 3.15$<INSTALL_INTERFACE:DATADIR="\$\{_IMPORT_PREFIX\}/${CMAKE_INSTALL_DATADIR}">
${PN}
and${NS}
abbreviation variables to${L2}
as the former generic names can get overwritten by a call (e.g.,find_package()
where another project assigns them)gss
for gaussian-standard-standard) since I think the 2-charss
were in anticipation of runtime-selectable solid harmonics. (less a priority for psi now)WIP c. 19 Jan
bumped CMake to 3.18 for list sorting inint_am.cmake
. will revert to 3.16 at the end unless new requirements surface.start upgrade guide and options description inINSTALL.md
detect Eigen if needed in Config fileFindTargetEigen3int_am finalization (need to show where new cmake harness #205 not behaving. need to handle G12 properly)organize exports to targets files betterhandle library targets as components in Config betterAdded 20 Jan
archived_libool/
dir so they can't participate unbeknownst to usproject()
(top-level and library export) so that usual _MAJOR, PROJECT_TWEAK, etc. that ppl may expect are definedshare/libint/version/basis
, notlibint2
?Added 21 Jan
Libint2::
aliases so that available internally. use these for tests and drop theint-library
, etc. series.Added 24 Jan
Libint2_Runtime
orLibint2_Development
Added 28 Jan
FindMPFR.cmake
toFindMultiprecision.cmake
to handle gmp, gmpxx, and mpfr targets. Based on offical cmake module FindGSL.cmake to hopefully handle Windows, too. Seek correct targets in build_libint and library.Libint2_LIBRARIES
from config. added backLibint2_EXT_VERSION
. Rearrange config so no targets created before "found" assured.CMAKE_Fortran_MODULE_DIRECTORY
and propertyFortran_MODULE_DIRECTORY
seemed redundant. Switched to only the latter and added the path to include directories on the target. Looking on the internet, where to install module files looks like it has no guidelines (esp. since compiler-version-dependent) so added an install knob. Added fpic when necessary so tests work with shared lib.Added 12 Feb
build_repo
, as before, builds and runs the compiler, builds the library, tests the library, tests a library consumer for Lin/Mac Debug/Release, all static libs. To this was added artifacting the built tarball and building on Windows (Release only). The Windows build is an expected fail -- the generated source is faulty. The second and new jobbuild_export
takes the linux tarball artifact and builds it on Lin/Mac/Win, shared libs where possible, with some additional dependency quirks testing. Changed build matrix from include format to cfg format, as I think it's easier to read. Switched Debug/Release EP/FetchContent pairs as Windows only works with 1/4 combinations.REQUIRE_CXX_API_COMPILED
to turn on/off the compiled C++11 interface apart from the header-only library. It's on by default, so no change.ENABLE_T1G12_SUPPORT
missing before.int-cxx-{static,shared}
toint-cxx-compiled-{static,shared}
for clarityLIBINT_LOCAL_Eigen3_FIND
is set, the FindEigen instead looks to import a local L2-defined target (see below) to ensure ABI consistency. This is off by default. The FindEigen3 detection is shared by the library build and libint2-config.cmakeLIBINT_LOCAL_Eigen3_INSTALL
is set, the detected eigen3 is installed as a target in a separate file. This is off by default, but I don't care if it's on by default, so long as it's turn-off-able and the _FIND option is off by default.WIP
ENABLE_XHOST
to autoselect optimization for current instruction set? It's gotten more complicated (2 -> 20 lines)int2-cxx
as provisional? I find the name confusing, and the library isn't testable.