Skip to content

Start to Refactor Summary Output#1060

Merged
dennisklein merged 3 commits intoFairRootGroup:devfrom
ChristianTackeGSI:pr/cmake_summary
Apr 30, 2021
Merged

Start to Refactor Summary Output#1060
dennisklein merged 3 commits intoFairRootGroup:devfrom
ChristianTackeGSI:pr/cmake_summary

Conversation

@ChristianTackeGSI
Copy link
Copy Markdown
Member

@ChristianTackeGSI ChristianTackeGSI commented Apr 27, 2021

Refactor the summary output into some functions.
This really only moves the functionality over into functions.

There is one commit with some slight improvments.

Making these things more generic/etc is left for another PR.

Checklist:

dennisklein
dennisklein requested changes Apr 28, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dennisklein dennisklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, pls remove the heading. The other two remarks are optional, we can delay them to later.

Comment thread cmake/private/FairRootSummary.cmake Outdated
Comment thread cmake/private/FairRootSummary.cmake Outdated
Comment thread cmake/private/FairRootSummary.cmake Outdated
@ChristianTackeGSI
Copy link
Copy Markdown
Member Author

ChristianTackeGSI commented Apr 29, 2021

Also added CXX standard checking:
Check that a given CMAKE_CXX_STANDARD maches up with the one from ROOT.

@dennisklein
Copy link
Copy Markdown
Member

dennisklein commented Apr 29, 2021

fairroot_summary

Hmm, looking at the added lines again and thinking about the added value, I am not sure, if I like it. Main goal was to put the typical info which is relevant for the user and in a way that it is easy to learn what are the options one would most likely change from time to time. A secondary goal was to put information which is useful in logs and not shown by CMake by default or not in a compact way easy for the eye.

We are now adding 7 questionable lines regarding the goals from above:

  • PROJECT NAME will basically never change and is of no importance in the summary at all. For logging just the directory itself and such already make clear which project it is. IMHO, remove it or see the example with FairMQ later.
  • VERSION is somewhat interesting, definitely for logging, also see FairMQ example later.
  • INSTALL PREFIX is absolutely fine, probably the most changed option.
  • the top-level install tree dirs are questionable IMHO. What do you think about not show them if they are the default ones, and only show the ones that were changed (for logging)?

This shows how one could save a lot of space for project name and version:

fmq_summary

Looking at version now, it shows 18.6.0 (18.4.1-94-g8711c788a) - I realize both are not of any use (partially because of the way I managed the branches). But here we are neither at version 18.6.0, nor are we at 18.4.1-94-g8711c788a (technically yes, but not for the average viewer). So, this needs more logic now. I will have a look into this.

dennisklein
dennisklein reviewed Apr 29, 2021
View reviewed changes
Comment thread CMakeLists.txt Outdated
@ChristianTackeGSI
cmake: Check CMAKE_CXX_STANDARD against ROOT
d012e3c 
ROOT and the package that depend on it should be compiled
with the CXX STANDARD.

So let's check that CMAKE_CXX_STANDARD matches up with
ROOT's setting.

Emits warnings for now.
@ChristianTackeGSI
Refactor Summary Output
89fb0d7 
Move Summary Output Generation into functions in a new
cmake/private/FairRootSummary.cmake. This is not yet
reusable. Goal is to make the main CMakeLists.txt more
readable.
@ChristianTackeGSI
Improve Summary Output
0e96a4a 
* Show Package Name and Version
* Show install paths
* Little reformatting
@ChristianTackeGSI
Copy link
Copy Markdown
Member Author

ChristianTackeGSI commented Apr 30, 2021

Okay, added a lot of STATUS -> VERBOSE in there.
So now you need to do

$ cmake … --log-level=VERBOSE

to see this stuff.

Maybe that's better now?

I am currently highly considering to add such a flag in our spack recipe "If the user looks at the logs, they want logs".

@dennisklein
Copy link
Copy Markdown
Member

dennisklein commented Apr 30, 2021

Okay, added a lot of STATUS -> VERBOSE in there.
So now you need to do

$ cmake … --log-level=VERBOSE
to see this stuff.

Maybe that's better now?

Yes, awesome.

I am currently highly considering to add such a flag in our spack recipe

Agreed. It is a good idea.

@dennisklein dennisklein merged commit d34c1ac into FairRootGroup:dev Apr 30, 2021
@dennisklein dennisklein added this to the v19.0 milestone Apr 30, 2021
@ChristianTackeGSI ChristianTackeGSI deleted the pr/cmake_summary branch April 30, 2021 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dennisklein dennisklein dennisklein requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v19.0

Development

Successfully merging this pull request may close these issues.

2 participants

@ChristianTackeGSI @dennisklein