WIP: Modernize CMake infrastructure #632

dennisklein · 2017-09-07T23:43:10Z

Most of this work has been merged already.

Missing are the following breaking changes:

Switch to root imported targets
Add CMake package
Modernize config.sh in concert with SPack

Major rework:

Provide CMake package config
Install exported targets
Update ROOT_GENERATE_DICTIONARY and ROOT_LINKER_LIBRARY based on upstream
Switch to target properties instead of manual path handling, patch
ROOT_GENERATE_DICTIONARY to fully support target properties
Modernize and cleanup Find* modules
Add support for native aliBuild search hints a la *_ROOT
Use CMake package configs of dependencies where possible
Rewrite old macros with new function interface if beneficial
Refactor/Deduplicate old "grown" spaghetti code
Clean and improve command line output
Remove dead code
Drop ROOT5 support
Set fairroot install prefix
Remove Geant3 dependency for EventDisplay (Review of eventdisplay CMake rules #550)
And more

resolves also #464

TODO:
*[x] ~~Get ROOT PR merged. This is a precondition for the new library/dictionary generation code to work.~~ We also need to provide a patch we apply locally for the transition phase, I guess.
[x] Discuss installation directories. Prefix dir or not (after lib and bin)? share/fairbase ?? -> decided in meeting on 8.9.17 to install to bin, lib/fairroot, include/fairroot, and share/fairroot.
[x] There have been other smaller non-critical PRs and bug reports for Geant3 (merged), Geant4 (open) and Geant4VMC (merged). IMHO the package configs of those projects could be extended to provide more info, so we have even less work to do in our Find modules. Do we want to invest this time? -> We can/should make contributions to CMake code of the above projects.
[x] resolve #513, the usage of environment variables should be reduced to Find modules only for aliBuild integration (_ROOT vars, mostly done) and the last places in cmake/Misc.cmake and cmake/modules/FairRootMacros.cmake need to be looked at.
*[x] resolve #550
*[x] resolve alisw/alibuild#508
*[x] Finish fixing examples directory
*[x] Generate package dependencies
*[x] Provide a feature summary output to get a concise overview, which components are built and which not and how to change that - basically document our build switches with some nice cmake output.
*[ ] Fix macro runtime -> Refactor variable set which is exported currently via config.sh, root_macro.sh and at least partly now in FAIRROOTConfig.cmake. IMHO we should avoid duplicating this logic in three places. I have foreseen the step file cmake/Runtime.cmake to be the authoritative place for this. What do you think? @fuhlig1 We also need to decide which variables to export in our package config.
*[ ] Convert ROOT_GENERATE_DICTIONARY to target_root_dictionary
*[ ] TEST TEST TEST (Mac, AliBuild etc)
*[ ] Update docs and project template!

in separate issue:
*[ ] Contribute to ROOT cmake
*[ ] make check_overlinking

dennisklein · 2017-09-11T21:48:39Z

@MohammadAlTurany @fuhlig1 I have decided to not use the upstream ROOT_GENERATE_DICTIONARY for now and closed the open PR. I found two more invasive changes needed, which I don't want to fight for right now to get accepted in ROOT. So, I have moved a patched copy of that macro into our own CMake module. In the future when the code is curated and stable and we have time, I guess, we can try to contribute it back.

dennisklein · 2017-12-08T17:18:18Z

It's almost rebased, looks like current dev changed again. In any case, this is not working yet.

I found another issue with aliBuild I am currently investigating. When you delete the defaults-release.sh recipe, aliBuild crashes. I will update this PR and alfadist simultaneously in a couple of days, once I got it to work.

dennisklein · 2017-12-13T18:00:01Z

$ pip install alibuild==1.5.1rc1
$ aliBuild --dist dennisklein/alfadist@master --defaults fairroot init FairRoot
$ cd FairRoot
FairRoot$ git remote add dklein https://github.com/dennisklein/FairRoot
FairRoot$ git fetch dklein
FairRoot$ git checkout -b cmake_refactor dklein/cmake_refactor
FairRoot$ git tag v-18.0.0
FairRoot$ cd ..
$ aliBuild --defaults fairroot build FairRoot

This builds now on my machine. There are still couple of open points, I will resolve soon. First to come is fixing the testsuite. It currently does not work yet for the macro based tests. Will post updates here.

ktf · 2018-01-18T09:23:40Z

Version.h.in

@@ -1,6 +1,6 @@
 #ifndef FAIRROOT_VERSION
 #define FAIRROOT_VERSION "@FAIRROOT_VERSION@"
-#define FAIRROOT_VERSION_INT (@FairRoot_VERSION_MAJOR@ * 1000000) + (@FairRoot_VERSION_MINOR@ * 1000) + @FairRoot_VERSION_PATCH@


Why changing this?

FAIRROOT_VERSION_INT was not existing before this PR anyways. The diff you see here is just WIP commits that I have not yet squashed.

ktf · 2018-01-18T09:24:24Z

cmake/Dependencies.cmake

+  thread
+  timer
 )
+if(Boost_FOUND)


Why defining an extra variable?

I have not traced this down yet. I did not invent this variable ;)

ktf · 2018-01-18T09:25:09Z

cmake/Runtime.cmake

+                  OUTPUT_STRIP_TRAILING_WHITESPACE
+                 )
+
+ElseIf(CMAKE_SYSTEM_NAME MATCHES Darwin)


I would try to be consistent with case.

yes, parts of the CMake are still legacy, I have not touched much yet.

ktf · 2018-01-18T10:35:27Z

cmake/Runtime.cmake

+list(APPEND LD_LIBRARY_PATH ${FAIRLIBDIR}) 
 list(REMOVE_DUPLICATES LD_LIBRARY_PATH)
-set(VMCWORKDIR ${CMAKE_SOURCE_DIR}/examples)
+set(MY_LD_LIBRARY_PATH ${LD_LIBRARY_PATH})


What about Mac and DYLD_LIBRARY_PATH?

The content of MY_LD_LIBRARY_PATH gets emitted in the FairRootConfig.sh script as LD_LIBRARY_PATH and DYLD_LIBRARY_PATH.

ktf · 2018-01-18T10:35:54Z

cmake/modules/FindPythia8.cmake

 )
+
+if(Pythia8_FOUND)
+  list(APPEND LD_LIBRARY_PATH ${PYTHIA8_LIB_DIR})


What about Mac?

It is just an internal variable. But this will change again. I don't like having to collect the lib dirs like this.

* Provide CMake package config * Install exported targets * Update ROOT_GENERATE_DICTIONARY and ROOT_LINKER_LIBRARY based on upstream * Switch to target properties instead of manual path handling, patch ROOT_GENERATE_DICTIONARY to fully support target properties * Modernize and cleanup Find* modules * Add support for native aliBuild search hints a la *_ROOT * Use CMake package configs of dependencies where possible * Rewrite old macros with new function interface if beneficial * Refactor/Deduplicate old "grown" spaghetti code * Clean and improve command line output * Remove dead code * Drop ROOT5 support * Set fairroot install prefix * Remove Geant3 dependency for EventDisplay (FairRootGroup#550) * Disable GoTutorial which puts temporary files into source dir * And more

dennisklein · 2020-02-28T00:44:46Z

Superseded by #933.

dennisklein added feature in progress labels Sep 7, 2017

dennisklein requested review from MohammadAlTurany, fuhlig1 and rbx September 7, 2017 23:43

dennisklein force-pushed the cmake_refactor branch from 02dc422 to ff7f7a9 Compare September 7, 2017 23:58

dennisklein self-assigned this Sep 8, 2017

dennisklein force-pushed the cmake_refactor branch 3 times, most recently from 9ddc4e6 to 46814ec Compare September 12, 2017 18:19

This was referenced Oct 10, 2017

update versions FairRootGroup/FairSoft#62

Merged

Improve protobuf detection #645

Closed

MohammadAlTurany mentioned this pull request Oct 11, 2017

use Findprotobuf from cmake instead of our own #646

Closed

dennisklein mentioned this pull request Oct 12, 2017

Look for protobuf on custom paths then system #652

Merged

MohammadAlTurany referenced this pull request Oct 23, 2017

require new boost and new cmake

62e5354

dennisklein force-pushed the cmake_refactor branch from 46814ec to 7cffba0 Compare December 8, 2017 17:13

dennisklein force-pushed the cmake_refactor branch from 7cffba0 to 38b0f79 Compare December 13, 2017 17:41

ktf reviewed Jan 18, 2018

View reviewed changes

MohammadAlTurany mentioned this pull request Mar 2, 2018

Make a special repository for FairMQ #736

Closed

dennisklein force-pushed the cmake_refactor branch from ec62395 to ef8cafc Compare May 9, 2018 16:42

Adapt to new version tag format

4973bac

MohammadAlTurany mentioned this pull request Nov 8, 2018

cmake Errors when building FairRoot v18.0.4 on Ubuntu #845

Closed

dennisklein added this to the 18.4 milestone Jun 28, 2019

dennisklein changed the title ~~[WIP] Modernize CMake infrastructure~~ WIP: Modernize CMake infrastructure Dec 16, 2019

dennisklein closed this Feb 28, 2020

WIP: Modernize CMake infrastructure #632

WIP: Modernize CMake infrastructure #632

Uh oh!

Conversation

dennisklein commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dennisklein commented Sep 11, 2017

Uh oh!

dennisklein commented Dec 8, 2017

Uh oh!

dennisklein commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisklein commented Feb 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dennisklein commented Sep 7, 2017 •

edited

Loading