Skip to content
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

Start to Refactor Summary Output #1060

Merged
merged 3 commits into from
Apr 30, 2021

Conversation

ChristianTackeGSI
Copy link
Member

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:

Copy link
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.

cmake/private/FairRootSummary.cmake Outdated Show resolved Hide resolved
cmake/private/FairRootSummary.cmake Outdated Show resolved Hide resolved
cmake/private/FairRootSummary.cmake Outdated Show resolved Hide resolved
@ChristianTackeGSI
Copy link
Member Author

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

@dennisklein
Copy link
Member

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.

CMakeLists.txt Outdated Show resolved Hide resolved
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.
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.
* Show Package Name and Version
* Show install paths
* Little reformatting
@ChristianTackeGSI
Copy link
Member Author

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
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants