-
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 #205
base: master
Are you sure you want to change the base?
new cmake harness #205
Conversation
I'm concerned about how eigen is handled in the library export. The local
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
Am I correct in finding this a concern, or am I missing something? |
… required CMake to 3.16
…massive ... let's stick with ERI rather than convert to twobody
@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 re prepending will have a look at the Eigen export concerns later today. |
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:
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.
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 libint/src/lib/libint/CMakeLists.txt.export Line 222 in e886334
|
@loriab the CI jobs currently fail due to some branches in libint/cmake/modules/int_am.cmake Line 129 in e886334
|
sounds good, will stick with ERI3 and
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
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 :-)
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.
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:
But the library building computer (e.g., Fedora package builder) and the consumer of
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?
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! |
6918ade
to
1ba2b2e
Compare
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?
I suggest we keep it at 3.16 ... that's the default in ubuntu 20.04 which
is the most common ubuntu out there.
The new component names are healed in #233
<#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.
So the install-tree's libint2-config has a build path in it?
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?
Yes, basically Libint's C++ interface will be only optionally header only.
The actual way I use Libint in MPQC is I instantiate the Engine code in a
single .cpp file to avoid the compile time impact of engine.impl.h ... if I
instantiate Engine in a libint target that target will depend on Eigen ABI
(not on Boost though ... Boost is only used for preprocessing) which means
we better make sure we "link" against same Eigen as we discovered in Libint
... so we need to cache locations of its config file and use it as a hint
in libint2-config file, etc. ... Eigen's configuration affects its ABI
(e.g. can force it to dispatch to BLAS/LAPACK, etc.) so we MUST make sure
the target we used to build Libint is the exact same one (not only version,
mind you, but the actual instance of that target). Right now the default is
to keep C++ interface as header only, but it's already an overly-expensive
(compile-time-wise) way to use it, so it probably needs to change, or at
least there should be an optional component that instantiates the Engine.
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.
Header-only libraries are fine for newbies/one-off uses. But too brittle
without an accompanying cmake/meson exported targets.
…--
web: valeyev.net
|
1ba2b2e
to
f89be3c
Compare
f89be3c
to
b13667b
Compare
fyi that the gha file in this branch is using commas and |
that's fine, the natural cmake separators are fine |
prefix=@CMAKE_INSTALL_PREFIX@ | ||
exec_prefix=@CMAKE_INSTALL_PREFIX@ | ||
libdir=@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@ | ||
includedir=@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_INCLUDEDIR@ |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
It looks like parts of library test 10 may be sensitive to SHGAUSS. All tests pass with
|
I was experimenting with the compiled cxx interface. Switching the library tests from
leads to build-time errors like
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. |
cmake build for compiler and library. supersedes #148 .
status board based on #148 (comment)
Major features
ExternalProject
; default) or as subdirectory (viaFetchContent
; only useful for testing)TODOs
sss
indicates all standard orderings andg4
to indicates that ERIs of at least deriv=1 and am=4 have been built, etc.Libint2Config.cmake
tolibint2-config.cmake
to maintain consistency with libint 2.6.pc
file for the libtool folks.BUILD_{SHARED,STATIC}
withBUILD_SHARED_LIBS
andLIBINT2_BUILD_SHARED_AND_STATIC_LIBS
(the latter is likely used by some downstream packages)BUILD_FPIC
withCMAKE_POSITION_INDEPENDENT_CODE
int2
as opposed tocxx
target)Won't fix in this PR