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

Show colored messages in CMake output #2194

Merged
merged 14 commits into from
May 17, 2022

Conversation

med-ayssar
Copy link
Contributor

@med-ayssar med-ayssar commented Oct 20, 2021

Changes:

  • Replace each occurrence of message([<mode>]) with the corresponding print function, depending on the mode keyword.
  • FATAL_ERROR is mapped to printError function, outputting a red colored message.
  • WARNING is mapped to printWarning function, outputting a yellow colored message.
  • STATUS is mapped to printInfo function, outputting a green colored message.
  • A message function without a mode is replaced by the printInfo function.
  • The summary phase of CMake is assigned a BoldCyan color.
  • The print function in ColorMessges can be used to extend new print functions with different colors.
  • Important: If the output will be written to a file, it will have escape sequences in it because they are being forced to be emitted always.

Before:

  • First output lines:
    cmake_output_orig_teil_1
  • Summary:
    camke_output_orig_teil_2

After:

  • First output lines:
    camke_output_teil_1

  • Summary:
    cmake_output_teil_2

  • On Error: Setting CMAKE_INSTALL_PREFIX to an empty string!
    cmake_abort

@terhorstd terhorstd added this to To do in Build system and CI via automation Oct 28, 2021
@terhorstd
Copy link
Contributor

@med-ayssar , could you provide a bit more description? E.g. is it sensitive to non-terminal output?

@med-ayssar
Copy link
Contributor Author

med-ayssar commented Oct 28, 2021

@med-ayssar , could you provide a bit more description? E.g. is it sensitive to non-terminal output?

Can you elaborate more, what do you mean by non-terminal output.
Basically, we are assigning "FATAL_ERROR" a red color, "WARNING" a yellow color and "STATUS" or default message a green color. For The summary, I have assigned different color to distinguish it from the above mentioned colors

The functions calls responsible for outputing suchs messages are now called:

  • printInfo -> green
  • printWarning -> yellow
  • printError -> red

All function calls to message are then replaced by the corresponding message type.

@terhorstd terhorstd added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Oct 28, 2021
@jougs
Copy link
Contributor

jougs commented Oct 28, 2021

@med-ayssar: The description should always contain a summary of what this PR is about and why we would want it in NEST (similar to what your comment does).

Both the description and the PR name should definitely mention that this relates to the cmake stage (in contrast to the runtime phase, PyNEST, NEST, SLI, or any such thing).

The description might also benefit from an example screenshot contrasting the situation before/after the change. Also please use GitHub Flavored Markdown to highlight code like things as typewriter and make things easier to read.

@jougs
Copy link
Contributor

jougs commented Nov 12, 2021

@med-ayssar: ping!

@med-ayssar med-ayssar changed the title Replace messages with colorful messages Show colored messages in cmake output Nov 12, 2021
@stinebuu stinebuu requested review from terhorstd and removed request for gtrensch December 6, 2021 09:53
@med-ayssar med-ayssar marked this pull request as draft December 6, 2021 12:27
@med-ayssar med-ayssar force-pushed the replace-message-calls-withColor branch from b63adfc to 2cbd6bb Compare December 6, 2021 12:48
@med-ayssar med-ayssar marked this pull request as ready for review December 6, 2021 13:48
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I have some minor suggestions and a less minor comment.

cmake/ColorMessages.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/ColorMessages.cmake Outdated Show resolved Hide resolved
@github-actions
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Feb 14, 2022
@jougs
Copy link
Contributor

jougs commented Feb 14, 2022

@med-ayssar: could you please have a look at my comments and finalize this? I really think it is a big improvement and should go in instead of sitting here stale and half-ready :-)

@jougs jougs removed the stale Automatic marker for inactivity, please have another look here label Feb 14, 2022
@jougs
Copy link
Contributor

jougs commented Mar 7, 2022

I think I have replied to all your questions. Please have another look.
And please also merge master, so the conflicts go away. Thanks!

cmake/ColorMessages.cmake Outdated Show resolved Hide resolved
cmake/ColorMessages.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

@med-ayssar: I just took the liberty to merge my suggestions into your branch 😁

@terhorstd: could you please especially look at differentiating between output to the shell and when redirected to files?

@terhorstd
Copy link
Contributor

All common tools strip color codes when the output is not going to a terminal. I'm not sure how the CI handles the output, but all common highlighting seems to be still there so there is either additional highlighting, or trickery with pseudo-ttys. So I'd guess that the isatty() / test -t type of calls can be used to split to non-highlighting versions when used in a pipe or redirected to a file (see for example this StackExchange answer).

@heplesser
Copy link
Contributor

@med-ayssar Could you fix the conflicts that have come up?

@med-ayssar med-ayssar marked this pull request as draft April 24, 2022 09:48
@jougs
Copy link
Contributor

jougs commented May 4, 2022

@terhorstd!

@jougs
Copy link
Contributor

jougs commented May 17, 2022

Due to the vacation of @terhorstd and that a) this is lying around for quite some time now, b) #2379 depends on it, and c) this is minor change that only affects the CMake output, I'm merging without the second review. Any (possible) problems with the rendering of output on the CI can be fixed in follow-up PRs.

@jougs jougs merged commit 584e3de into nest:master May 17, 2022
Build system and CI automation moved this from To do to Done May 17, 2022
@gtrensch gtrensch changed the title Show colored messages in cmake output Show colored messages in CMake output Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants