Add badges and improve make output #879

MohammadAlTurany · 2019-03-09T19:07:01Z

Add license badge
Use the FairMQ style implemented by Dennis in FairRoot

dennisklein · 2019-03-11T16:22:58Z

cmake/modules/FairMacros.cmake

 EndMacro (SetBasicVariables)
+################################################################################
+macro(find_package2 qualifier pkgname)


Use this improved version of the find_package2 macro: https://github.com/FairRootGroup/FairMQ/blob/master/cmake/FairMQLib.cmake#L296-L341

It allows to write this (in top level CMakeLists.txt)

set(FairRoot_Boost_COMPONENTS thread system timer program_options random filesystem chrono exception regex serialization log log_setup atomic date_time signals) list(APPEND FairRoot_Boost_COMPONENTS ${FairMQ_Boost_COMPONENTS}) list(REMOVE_DUPLICATES FairRoot_Boost_COMPONENTS) find_package2(PUBLIC Boost VERSION 1.67 COMPONENTS ${FairRoot_Boost_COMPONENTS})

as just

find_package2(PUBLIC Boost VERSION 1.67 ${FairMQ_Boost_VERSION} COMPONENTS thread system timer program_options random filesystem chrono exception regex serialization log log_setup atomic date_time signals ${FairMQ_Boost_COMPONENTS} )

It will pick highest listed version and automatically make the components list unique. I was also thinking about a syntax like this:

find_package2(PUBLIC Boost VERSION 1.67 COMPONENTS thread system timer program_options random filesystem chrono exception regex serialization log log_setup atomic date_time signals ADD_REQUIREMENTS_OF FairMQ )

or similar. What do you think?

This reminds me, we should also add find_package2 to FairLogger, which now has an optional dependency to Boost as well. With generate_package_dependencies() one can generate a cmake string, that can be exported in the cmake package, which defines ${FairLogger_Boost_VERSION} and ${FairLogger_Boost_COMPONENTS} - basically for any find_package2 call, if PUBLIC and VERSION/COMPONENTS are specified. Made an issue FairRootGroup/FairLogger#14.

Then it would read

find_package2(PUBLIC Boost VERSION 1.67 ${FairMQ_Boost_VERSION} ${FairLogger_Boost_VERSION} COMPONENTS thread system timer program_options random filesystem chrono exception regex serialization log log_setup atomic date_time signals ${FairMQ_Boost_COMPONENTS} ${FairLogger_Boost_COMPONENTS} )

or

find_package2(PUBLIC Boost VERSION 1.67 COMPONENTS thread system timer program_options random filesystem chrono exception regex serialization log log_setup atomic date_time signals ADD_REQUIREMENTS_OF FairMQ FairLogger )

Actually, I like second one even better, will add it at some point.

This topic has another aspect: The reason for exporting the public package dependency versions/components, is obviously, that we do not have to hardcode any requirements of the dependent package. But what about the list of dependencies itself? Currently, we hardcode it. For Boost, it is somewhat hidden, because we depend on it directly in FairRoot, FairMQ, and FairLogger. E.g. FairMQ also exports a variable ${FairMQ_PACKAGE_DEPENDENCIES} that could be used to loop over all public FairMQ dependencies and "find_package" them, even if FairRoot does not directly depend on them.

So far, so good, but this is not enough. Imagine multiple dependencies exporting such a list of their dependencies and they overlap partly. Then one would first have to "fuse" those lists together preserving their topological properties (doable, if the order is the original call order !currently not the case! those lines would be a bug) and figure out a call order together with the direct dependencies FairRoot "finds" explicitely. All doable, but it is not yet done.

FairRootGroup/FairMQ#142 FairRootGroup/FairMQ#143 FairRootGroup/FairMQ#144

My understanding here is to clean and simplify the CMake output so that the users see in one shot what he has. The real issue of dependancies between packages is solved in AliBuild or FairSoft, OR?

Those tools will build, install and decide on the package versions. But the public package dependencies must be discovered by each project for the whole subtree.

E.g. if a dependency uses a symbol of yet another library in its header, that is included in a FairRoot source file. Then, we only know about the target name, that we need to link against. But that target is not defined unless we find_package the righr dependency.

In a way, it is a standard way of doing, what the config.sh does.

dennisklein

Regarding the package components, you may want to have a look at generate_package_components. It picks up the list of components in the variable ${PROJECT_PACKAGE_COMPONENTS} and generates component checking code that can be exported in a cmake package (code similar to this and that). This will enable that the user can find a FairRoot with specific components enabled (even without find_package2), e.g.

find_package(FairRoot COMPONENTS base eve)

In FairMQ, I chose to keep the component names in sync with the table output when running cmake. E.g.

find_package(FairMQ 1.3.8 COMPONENTS nanomsg_transport)

will explicitely require a FairMQ installation with enabled nanomsg transport.

MohammadAlTurany · 2019-03-11T20:45:55Z

@dennisklein Thanks, I will adapt to your suggestions.

Find out which package version was installed

-Find report only on errors, the out put of find is summarized at the end -Improve the Compiler check

fuhlig1

Looks good to me. I only add some comments at specific places. What I only saw in the end is that we use exec_program() at several places. Since this function is deprecated we should use execute_process() instead.

cmake/modules/CheckCompiler.cmake

cmake/modules/ROOTMacros.cmake

fuhlig1 · 2019-03-13T10:01:14Z

cmake/modules_old/FindCLHEP.cmake

@@ -1,8 +1,8 @@
 ################################################################################


Do we still need this Find module? We use the CLHEP which comes with Geant4.

fuhlig1 · 2019-03-13T10:05:09Z

cmake/modules_old/FindGENERATORS.cmake

@@ -9,7 +9,7 @@ if (GENERATORS_LIBRARY_DIR)
  SET (GENERATORS_LIBRARY_DIR GENERATORS_LIBRARY_DIR-NOTFOUND)


Do we still need this Find module?

fuhlig1 · 2019-03-13T10:07:32Z

CMakeLists.txt

 # Set project version
 SET(FAIRROOT_MAJOR_VERSION 18)
 SET(FAIRROOT_MINOR_VERSION 0)
-SET(FAIRROOT_PATCH_VERSION 1)
+SET(FAIRROOT_PATCH_VERSION 3)


We are already at patch version 6 for the v18.0 branch. So for the development branch we should choose a higher number or decide on a different numbering scheme.

Ok, I will change it to 7 now.

…PROGRAM

fuhlig1 · 2019-03-13T12:24:38Z

The problem I see is that we create a new patch we will have again a conflict. Somehow we have to distinguish patches from the development.

…

Am 13.03.2019 um 13:20 schrieb Mohammad Al-Turany ***@***.***>: @MohammadAlTurany commented on this pull request. In CMakeLists.txt: > # Set project version SET(FAIRROOT_MAJOR_VERSION 18) SET(FAIRROOT_MINOR_VERSION 0) -SET(FAIRROOT_PATCH_VERSION 1) +SET(FAIRROOT_PATCH_VERSION 3) Ok, I will change it to 7 now. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

MohammadAlTurany added the in progress label Mar 11, 2019

dennisklein reviewed Mar 11, 2019

View reviewed changes

MohammadAlTurany added 11 commits March 13, 2019 09:55

Add license badge

39c1247

Find out which package version was installed

print out package versions and prefix

757fcd0

minor fixes

c45a234

Improve findPythia8 and add accedently removed statment

f8915ca

Add FindVGM

ff2e40c

Add print out for components and improve find_pythia8

bd303d7

Improve CMake output

ebdd689

-Find report only on errors, the out put of find is summarized at the end -Improve the Compiler check

STRIP the string before comparing

5511bc7

correct compiler comparison on linux

09e9aac

reduce the cmake output and add print more options

2039c99

add build type table to cmake output

fe43822

MohammadAlTurany force-pushed the add_badges branch from 27fb66a to fe43822 Compare March 13, 2019 08:58

MohammadAlTurany requested review from fuhlig1, karabowi and kresan March 13, 2019 08:58

add accedently removed lines

701006b

fuhlig1 reviewed Mar 13, 2019

View reviewed changes

Correct patch number, use execute_process instead of deprecated EXEC_…

fab6d5c

…PROGRAM

MohammadAlTurany added 2 commits March 14, 2019 08:30

adapt the projects templates to the CMAKE changes

5b3ec96

adapt the projects templates to the CMAKE changes-typo in cmake

7fd2eb6

kresan approved these changes Mar 14, 2019

View reviewed changes

MohammadAlTurany removed the in progress label Mar 14, 2019

karabowi approved these changes Mar 14, 2019

View reviewed changes

MohammadAlTurany merged commit 3475c41 into FairRootGroup:dev Mar 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add badges and improve make output #879

Add badges and improve make output #879

MohammadAlTurany commented Mar 9, 2019

dennisklein Mar 11, 2019 •

edited

Loading

dennisklein Mar 11, 2019 •

edited

Loading

dennisklein Mar 11, 2019 •

edited

Loading

dennisklein Mar 11, 2019

MohammadAlTurany Mar 12, 2019

dennisklein Mar 12, 2019

MohammadAlTurany Mar 12, 2019

dennisklein Mar 12, 2019

dennisklein left a comment •

edited

Loading

MohammadAlTurany commented Mar 11, 2019

fuhlig1 left a comment

fuhlig1 Mar 13, 2019

fuhlig1 Mar 13, 2019

fuhlig1 Mar 13, 2019

MohammadAlTurany Mar 13, 2019

fuhlig1 commented Mar 13, 2019 via email

		@@ -1,8 +1,8 @@
		################################################################################

		@@ -9,7 +9,7 @@ if (GENERATORS_LIBRARY_DIR)
		SET (GENERATORS_LIBRARY_DIR GENERATORS_LIBRARY_DIR-NOTFOUND)

Add badges and improve make output #879

Add badges and improve make output #879

Conversation

MohammadAlTurany commented Mar 9, 2019

dennisklein Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

dennisklein Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

dennisklein Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisklein left a comment • edited Loading

Choose a reason for hiding this comment

MohammadAlTurany commented Mar 11, 2019

fuhlig1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuhlig1 commented Mar 13, 2019 via email

dennisklein Mar 11, 2019 •

edited

Loading

dennisklein Mar 11, 2019 •

edited

Loading

dennisklein Mar 11, 2019 •

edited

Loading

dennisklein left a comment •

edited

Loading