Closed krasznaa closed 2 years ago
Weird that the tests didn't trigger automatically. I guess I'll need @niermann999 to sign off on starting the tests.
The "local ones" in my fork did run.
https://github.com/krasznaa/algebra-plugins/actions
That's how I know that there are problem... :frowning:
Also pinging @paulgessinger, so he would share in this fun as well. :stuck_out_tongue:
I now see that I'll also need to update the project's README.md
as part of this PR. But I'll wait with that until it would seem that people are at large friendly towards these changes.
Also pinging @asalzburger. :wink:
On the CI trigger not firing: that's really odd. I don't immediately have an idea why it wouldn't trigger.
That's what happened to detray. I think it is CI is trying to use master in build.yml. it will run normally once we change it to main.
And the point goes to Beomki! :smile: Indeed, the actions were set up to run on pull-requests into master
and not into main
...
I am always happy about help with cmake!
Let me try to further debug the issue with the new hints from https://gitlab.kitware.com/cmake/cmake/-/issues/22766. I may be able to make this PR fully functional after all... So let's wait with it just a little longer... :wink:
The macOS tests still have to start, but I believe the builds will succeed this time around.
As scary as this sounds, the problem was with the order in which the externals were set up. :frowning: When building GoogleTest at first, that is the one that first "pulls in" some of the files that use try_compile to determine what the build system is capable of doing.
And the very latest version of googletest still only requires a minimum of CMake 3.5.
https://github.com/google/googletest/blob/master/CMakeLists.txt#L4
Which, to be fair, is very understandable from their side.
So when check_cxx_source_compiles is used for the first time during the configuration, because of the CMake 3.5 requirement, CMP0067 is "frozen into" the OLD
value. :frowning: Even though vecmem is very clear that it needs at least CMake 3.10...
So... for now I just reversed the include-order of googletest and vecmem. Which is a super fragile thing to have to do to make the build work, I absolutely agree. But I think this is the best that we can do for the moment... :thinking:
We will also need to look out for this with all the other projects. But I'll take a look at those as well myself as well.
Okay, the PR is ready to be merged from my side. :wink:
Unfortunately I could just not get rid of:
CMake Deprecation Warning at /home/krasznaa/ATLAS/projects/algebra-plugins/build/_deps/eigen3-src/CMakeLists.txt:3 (cmake_minimum_required):
Compatibility with CMake < 2.8.12 will be removed from a future version of
CMake.
Update the VERSION argument <min> value or use a ...<max> suffix to tell
CMake that the project does not need compatibility with older versions.
:frowning: This one can only be disabled from the command line, with -Wno-dev
. It can not be influenced by any (cache) variable. :frowning:
Still, the Eigen build looks a bit better with at least the
CMake Warning (dev) at /atlas/software/cmake/3.21.3/x86_64-ubuntu1804-gcc7-opt/share/cmake-3.21/Modules/FindPackageHandleStandardArgs.cmake:438 (message):
The package name passed to `find_package_handle_standard_args` (CHOLMOD)
does not match the name of the calling package (Cholmod). This can lead to
problems in calling code that expects `find_package` result variables
(e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
/home/krasznaa/ATLAS/projects/algebra-plugins/build/_deps/eigen3-src/cmake/FindCholmod.cmake:86 (find_package_handle_standard_args)
/home/krasznaa/ATLAS/projects/algebra-plugins/build/_deps/eigen3-src/bench/spbench/CMakeLists.txt:16 (find_package)
This warning is for project developers. Use -Wno-dev to suppress it.
type of warnings gone. Which you can still turn on by calling CMake explicitly with -Wdev
when configuring this project.
I don't have modification rights for this repository, so if you're happy, you should press the merge button Joana. :wink:
Unfortunately the code is not working correctly at the moment (more on that further down), but I thought I would still open a PR with all of these, just give time for people to familiarize themselves with what I've done.
What I started doing was to update the CMake configuration in traccc, to make sure that it could build SYCL code consistently with how vecmem builds SYCL code. (For the work that @konradkusiak97 will be doing.) But I had to realise that to do this, I would need to do some cleanup in every project. So I decided to start with this one...
Let's start with the main features of the "new" CMake configuration:
algebra_add_library(...)
andalgebra_add_test(...)
functions to harmonise how libraries and tests would be set up in the project.Smatrix
however would always have to come from some existing place.I have to say, I really don't like how the source code is laid out in the repository at the moment. So this PR re-organises that quite a bit as well. All (
INTERFACE
) libraries are now put into "top level directories" in the project. I also renamed thevecmem
andvc
directories tovecmem_array
andvc_array
, to be in sync with the names of the CMake libraries. Though I guess the synchronisation could've been done the other way around as well, by renaming the CMake libraries.The libraries are now set up as follows:
algebra::common
is always set up from the "common headers" that were previously pretty haphazardly included by the other libraries. It also definesALGEBRA_PLUGIN_CUSTOM_SCALARTYPE
itself, so that it would only be done in one place.algebra::array
is set up on top ofalgebra::common
, without any noteworthy settings.algebra::vecmem_array
is set up on top ofalgebra::common
andvecmem::core
, and setsALGEBRA_PLUGIN_USE_VECMEM
toTRUE
.algebra::vc_array
is set up on top ofalgebra::common
andVc
, and setsALGEBRA_PLUGIN_INCLUDE_VC
toTRUE
.algebra::eigen
is set up on top ofalgebra::common
andEigen3::Eigen
by default. And ifalgebra::vecmem_array
and/oralgebra::vc_array
are "built", it also links against those. So that users ofalgebra::eigen
would inherit the pre-processor macros configured on all those libraries.algebra::smatrix
is set up on top ofalgebra::common
andROOT::Smatrix
by default. It links againstalgebra::vecmem_array
and/oralgebra::vc_array
in the same way asalgebra::eigen
does.I hope I understood the intent of the code correctly. Since I found it very confusing how
ALGEBRA_PLUGIN_USE_VECMEM
andALGEBRA_PLUGIN_INCLUDE_VC
are used in the configuration at the moment...I also re-shuffled the unit test source files a bit, to reduce the "depth" of the project's directory structure a bit.
The packaging of the project works such, that when I build it with:
, I would get an installation like the following:
And what's more, the project can even be set up successfully in a child project at that point.
And finally, the elephant in the room... The CI doesn't actually work. Because of: https://gitlab.kitware.com/cmake/cmake/-/issues/22766 Or at least that's the best that I could identify the problem as. :frowning:
Still, that problem affects the current configuration as well. This configuration just identified the problem by attempting to use VecMem on macOS in the CI...
So... I'm curious about any feedback. :wink: