Page MenuHomePhabricator

Specify log level for CMake messages (less stderr)
ClosedPublic

Authored by siedentop on Jun 14 2019, 5:00 PM.

Details

Summary

Specify message levels in CMake. Prefer STATUS (stdout).

As the default message mode (i.e. level) is NOTICE in CMake, more then necessary messages get printed to stderr. Some tools, noticably ccmake treat this as an error and require additional confirmation and re-running CMake's configuration step.

This commit specifies a mode (either STATUS or WARNING or FATAL_ERROR) instead of the default.

  • I used csearch -f 'llvm-project/.+(CMakeLists\.txt|cmake)' -l 'message\("' to find all locations.
  • Reviewers were chosen by the most common authors of specific files. If there are more suitable reviewers for these CMake changes, please let me know.

Diff Detail

Event Timeline

siedentop created this revision.Jun 14 2019, 5:00 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, lldb-commits, Restricted Project and 2 others. · View Herald Transcript

Seems alright to me. I'd like to see 1 message changed from a STATUS to a WARNING though.

lldb/cmake/modules/LLDBConfig.cmake
131

Please make this a warning.

siedentop updated this revision to Diff 204886.Jun 14 2019, 5:20 PM

Modified STATUS to WARNING as requested by @xiaobai

siedentop marked an inline comment as done.Jun 14 2019, 5:21 PM

Thank you for the review, @xiaobai. I've made the requested change.

The llvm/utils/benchmark/ changes - can you please either submit them upstream yourself, or explicitly state that it is okay to steal them from you and submit?

The llvm/utils/benchmark/ changes - can you please either submit them upstream yourself, or explicitly state that it is okay to steal them from you and submit?

Thanks, @lebedev.ri, I was not aware that llvm/utils/benchmark contains Google Benchmark.

I just checked: all three modified files in llvm/utils/benchmark/ already have the same modifications on upstream (HEAD == 090faecb454fb).

I propose that I remove my modifications from this review. I'll go ahead with this -- but please let me know if you disagree.

The llvm/utils/benchmark/ changes - can you please either submit them upstream yourself, or explicitly state that it is okay to steal them from you and submit?

Thanks, @lebedev.ri, I was not aware that llvm/utils/benchmark contains Google Benchmark.

I just checked: all three modified files in llvm/utils/benchmark/ already have the same modifications on upstream (HEAD == 090faecb454fb).

Ah yes, apparently so, thanks for checking!
I have no further comments then.

I propose that I remove my modifications from this review. I'll go ahead with this -- but please let me know if you disagree.

Removed changes to llvm/utils/benchmark.

Thanks for the initiative! Three inline comments for cosmetics.

lldb/cmake/modules/LLDBConfig.cmake
143

STATUS messages are automatically prefixed with -- so it can be removed from the text here.

187

Same as above. Please remove -- .

lldb/cmake/modules/LLDBStandalone.cmake
98

Same as above. Please remove -- .

siedentop updated this revision to Diff 205242.Jun 17 2019, 8:41 PM

@sgraenitz , good find. I've removed the leading dashes.

sgraenitz accepted this revision.Jun 18 2019, 4:30 AM
This revision is now accepted and ready to land.Jun 18 2019, 4:30 AM

@sgraenitz I cannot merge the changes myself, could you please commit them for me?

Closed by commit rL363821: Specify log level for CMake messages (less stderr) (authored by stefan.graenitz, committed by ). · Explain WhyJun 19 2019, 8:22 AM
This revision was automatically updated to reflect the committed changes.