Beautify CMake messages #411

Shatur · 2021-07-29T12:09:34Z

At the moment, stp can be conveniently used together with FetchContent in other projects.
But CMake output from stp a bit noisy. And can be confusing for other developers, because they can see warning messages like "Testing is disabled" that comes from stp and unrelated their projects.

In this PR I changed messages type from STATUS to DEBUG because the information like "Boost -- adding '${Boost_LIBRARY_DIRS}' to link directories" is for the stp developers. The documentation says that the purpose of DEBUG is "Detailed informational messages intended for developers working on the project itself as opposed to users who just want to build it.". So looks like a good fit to me because usually the official modules (which are provided by CMake itself) don't print that much information.

To provide the information about what things are included, I used FeatureSummary.

aytey · 2021-07-29T12:37:59Z

Should FeatureSummary list all of the options? For example, including the "SUPPRESS_WARNINGS" you recently added?

In fact, should add_feature_info be called for all options?

Shatur · 2021-07-29T12:40:40Z

Should FeatureSummary list all of the options?

As you wish. I see FeatureSummary as a list of options that affect features in an application. But if you want, I can add descriptions for all options as well.

aytey · 2021-07-29T12:40:58Z

In this PR I changed messages type from STATUS to DEBUG because the information like "Boost -- adding '${Boost_LIBRARY_DIRS}' to link directories" is for the stp developers.

I suggest doing something like (not tested, written in the Github editor):

option(DEBUG_MESSAGES "enable debug messages from CMake" ON)

if (DEBUG_MESSAGES)
   set(DEBUG_STREAM STATUS)
else()
   set(DEBUG_STREAM DEBUG)
endif()

message(${DEBUG_STREAM} "You can choose the type of build, options are: ${build_types}")

So this leaves it "as-is" today but allows a "knowledgeable user" to silence this to the debug stream if necessary.

aytey · 2021-07-29T12:45:27Z

I see FeatureSummary as a list of options that affect features in an application.

Sure, I can agree with that.

I guess then we need:

COVERAGE
ENABLE_ASSERTIONS
NOCRYPTOMINISAT
ONLY_SIMPLE
SANITIZE
STATICCOMPILE
USE_RISS
USE_THREAD_LOCAL

Does that seem like a reasonable enough list to you?

Shatur · 2021-07-29T12:49:26Z

I suggest doing something like (not tested, written in the Github editor):

Okay, will do!

Does that seem like a reasonable enough list to you?

Hm... As to me COVERAGE, ENABLE_ASSERTIONS and SANITIZE not directly related to the features of the library. But won't hurt, will add :)

aytey · 2021-07-29T12:51:54Z

As to me COVERAGE, ENABLE_ASSERTIONS and SANITIZE not directly related to the features of the library.

Hmm, why do you think that? The behaviour of a build that enables ENABLE_ASSERTIONS could be "observably different" from a build that doesn't enable. Same for SANITIZE or COVERAGE -- linking a library with these could be different to linking against a library without them.

Shatur · 2021-07-29T12:54:07Z

Hmm, why do you think that?

Just because this features mostly for developing, not like a regular feature for user.

aytey · 2021-07-29T12:55:58Z

Just because this features mostly for developing, not like a regular feature for user.

Nah, I don't agree with that -- a build without ENABLE_ASSERTIONS might be more tolerant of runtime issues caused by a user.

If you're happy, I think the above list a reasonable starting point.

Shatur · 2021-07-29T12:59:51Z

If you're happy, I think the above list a reasonable starting point.

OK, I do not mind :)

option(DEBUG_MESSAGES "enable debug messages from CMake" ON)

How about to use CMAKE_MESSAGE_LOG_LEVEL instead? Personally, it seems to me that it is better to set this parameter from the command line / IDE settings, but we can change the default log level using this variable if you want.

aytey · 2021-07-29T13:03:59Z

How about to use CMAKE_MESSAGE_LOG_LEVEL instead? Personally, it seems to me that it is better to set this parameter from the command line / IDE settings, but we can change the default log level if you want.

https://cmake.org/cmake/help/latest/variable/CMAKE_MESSAGE_LOG_LEVEL.html#variable:CMAKE_MESSAGE_LOG_LEVEL

New in version 3.17.

Could be too new 😞

I think @msoos's comment on 3.11 being fine is reasonable; I am not sure about 3.17 (I certainly don't want to bump to 3.17 just for messages 😂)

This raises a good point ... do we need to bump the CMake version for FeatureSummary?

Shatur · 2021-07-29T13:09:30Z

I think @msoos's comment on 3.11 being fine is reasonable; I am not sure about 3.17 (I certainly don't want to bump to 3.17 just for messages joy)

😄
But everything should work on the old version, just not all messages will be shown until the user specifies --log-level.
But I just noticed that DEBUG was added in 3.15. So you should decide if you want to bump it to 3.15. BTW, for the current Debian / Ubuntu CMake 3.15+ is available.

This raises a good point ... do we need to bump the CMake version for FeatureSummary?

Bumping is not necessary, It's available in 3.3.

aytey · 2021-07-29T13:13:35Z

How much do you care about changing these messages? I don't want to break some of the upstream projects that use STP (our main user is KLEE, which is currently at 3.5 🙀 )

Break is a strong term there: I want STP to work everywhere that KLEE works; if there are places where KLEE works and STP doesn't (because of CMake) that's a really dumb choice by us (== the STP project).

Shatur · 2021-07-29T13:25:46Z

How much do you care about changing these messages? I don't want to break some of the upstream projects that use STP (our main user is KLEE, which is currently at 3.5 scream_cat )

Oh, got it. So even 3.11 is unavailable?
Hm... Okay, probably we could left only FeatureSummary. I just found myself a lot of messages from STP annoying. Libraries usually don't print a lot of debug information, such as:

MESSAGE(STATUS "PROJECT_VERSION: ${PROJECT_VERSION}")
MESSAGE(STATUS "PROJECT_VERSION_MAJOR: ${PROJECT_VERSION_MAJOR}")
MESSAGE(STATUS "PROJECT_VERSION_MINOR: ${PROJECT_VERSION_MINOR}")
MESSAGE(STATUS "PROJECT_VERSION_PATCH: ${PROJECT_VERSION_PATCH}")

Or

message(DEBUG "You can choose the type of build, options are: ${build_types}")

Where build_types is a hardcoded list of default CMake options (in your CMakeLists.txt!). Also this options do not have any effect for multi-configuration generators (MSVC or Ninja in multi-configuration mode) because built type is determined at the build stage (via --config <mode>).

I can leave with this. But can I suggest to use STATUS if cryptominisat is disabled instead of WARNING (for example, FeatureSummary also use regular for disabled optional components)? Also it would be nice to not print anything or at least use STATUS instead of WARNING if testing is disabled.

aytey · 2021-07-29T13:43:16Z

How about:

if (SUPPRESS_WARNINGS)
   set(WARNING_STREAM STATUS)
else()
   set(WARNING_STREAM WARNING)
endif()

message(${WARNING_STREAM} "<cms message>")

?

Shatur · 2021-07-29T13:47:57Z

How about:

Are you suggesting to use the same SUPPRESS_WARNINGS variable as for compiler warnings?
The solution looks hacky to me. Is anyone really need the current behavior? I would just use regular messages as in FeatureSummary and remove some useless debug information like mentioned above. It quite uncommon to print such stuff. Anyone can just look at CMakeCahe, CMake GUI or just add required message to debug something.

aytey · 2021-07-29T13:54:48Z

The solution looks hacky to me. Is anyone really need the current behavior?

Yes, the developers of STP (e.g., me).

I would actually argue that your approach of looking in the cache directly is significantly more advanced.

Shatur · 2021-07-29T14:06:03Z

Yes, the developers of STP (e.g., me).

Okay, I get it.
Overriding message type with another option will be too hacky. Let's leave the current behavior... I just tried to suggest how it usually done in other libraries. Yes, I understand why you can't bump the required CMake version, but right now I'm talking about not using WARNING for the library features. Then FeatureSummary will not fit well too.

I would actually argue that your approach of looking in the cache directly is significantly more advanced.

Sure, but two other suggested approaches should be fine. And you almost never need this information. Especially printing stuff like PROJECT_VERSION or standard build types.

aytey · 2021-07-29T14:14:57Z

Does swapping WARNING for STATUS help you though? Because I guess I'm fine with that.

Just so you know: I'm not trying to be difficult; I'm trying to find the middle-ground between sticking with an older CMake, not changing the interface the current STP development team use and making life better for you -- not always easy :)

Shatur · 2021-07-29T14:19:05Z

Does swapping WARNING for STATUS help you though? Because I guess I'm fine with that.

Yes! This is almost exactly what I suggesting, sorry If I confuse you. Will do it and update FeatureSummary list.

not always easy

Oh, totally understand 😄

aytey · 2021-07-29T14:20:07Z

I think we got distracted.

Okay, so:

WARNING -> STATUS
Add FeatureSummary

🚀

aytey · 2021-07-29T14:23:58Z

It is probably "abusive", but how about those warnings going into FeatureSummary (and then the messages can go away all together)?

Shatur · 2021-07-29T14:30:39Z

how about those warnings going into FeatureSummary

Do I understand correctly that you suggest to use feature summary instead of "found / not found" messages?

For example, use only this:

add_feature_info(CryptoMiniSat USE_CRYPTOMINISAT "Enables CryptoMiniSat solver, allows --cryptominisat5 option")

and remove the following "found" messages:

message(STATUS "Found CryptoMiniSat 5.x and C++11, allowing --cryptominisat5 option")
message(STATUS "CryptoMiniSat5 (5.0 or above) or C++11 support not found, not allowing --cryptominisat option")

while keep other information if the library found, like

message(STATUS "CryptoMiniSat5 dynamic lib: ${CRYPTOMINISAT5_LIBRARIES}")
message(STATUS "CryptoMiniSat5 static lib:  ${CRYPTOMINISAT5_STATIC_LIBRARIES}")
message(STATUS "CryptoMiniSat5 static lib deps: ${CRYPTOMINISAT5_STATIC_LIBRARIES_DEPS}")
message(STATUS "CryptoMiniSat5 include dirs: ${CRYPTOMINISAT5_INCLUDE_DIRS}")

aytey · 2021-07-29T14:32:35Z

To me, that seems like it has some elegance and "improves" things (and hopefully makes life better for you). What do you think?

I don't think we can solve the STATUS ones that you listed at the bottom, but small incremental steps are always good ...

Shatur · 2021-07-29T14:34:54Z

To me, that seems like it has some elegance and "improves" things (and hopefully makes life better for you). What do you think?

Agree, will look much nicer, will do!

aytey · 2021-07-29T14:36:00Z

And so we'll do it for stuff that isn't "just" features? Like SANTIZE and TESTING? This was the "abusive" part.

Shatur · 2021-07-29T14:45:09Z

And so we'll do it for stuff that isn't "just" features? Like SANTIZE and TESTING? This was the "abusive" part.

Sure, going to add a summary for every item in the following list:

COVERAGE
ENABLE_ASSERTIONS
NOCRYPTOMINISAT
ONLY_SIMPLE
SANITIZE
STATICCOMPILE
USE_RISS
USE_THREAD_LOCAL

aytey · 2021-07-29T14:50:20Z

Won't things like PYTHON_EXECUTABLE cause you problems though?

message(WARNING "Cannot find python interpreter, cannot build python interface")

Same for ENABLE_TESTING:

message(WARNING "Testing is disabled")

Shatur · 2021-07-29T14:53:52Z

Won't things like PYTHON_EXECUTABLE cause you problems though?

Going to replace the following:

if ((NOT DEFINED ENABLE_PYTHON_INTERFACE) OR ENABLE_PYTHON_INTERFACE)
    find_package (PythonInterp)
    if (PYTHON_EXECUTABLE)
        set(PYTHON_OK ON)
        message(STATUS "OK, found python interpreter")
    else()
        message(WARNING "Cannot find python interpreter, cannot build python interface")
    endif()

    if (PYTHON_OK AND BUILD_SHARED_LIBS)
        message(STATUS "Python found, enabling python interface")
        set(ENABLE_PYTHON_INTERFACE ON)
    endif()
endif()

with

if((NOT DEFINED ENABLE_PYTHON_INTERFACE) OR ENABLE_PYTHON_INTERFACE)
    find_package(PythonInterp)
    if(PYTHON_EXECUTABLE AND BUILD_SHARED_LIBS)
        set(ENABLE_PYTHON_INTERFACE ON)
    endif()
endif()
add_feature_info(PythonInterface ENABLE_PYTHON_INTERFACE "Enables Python interface")

What do you think? PYTHON_OK was used in this block. A message about found Python interpreter will be displayed by CMake itself.

Same for ENABLE_TESTING:

Similar here, going to represent it as a build "feature". Is it okay?

aytey · 2021-07-29T15:00:21Z

if((NOT DEFINED ENABLE_PYTHON_INTERFACE) OR ENABLE_PYTHON_INTERFACE)
    find_package(PythonInterp)
    if(PYTHON_EXECUTABLE AND BUILD_SHARED_LIBS)
        set(ENABLE_PYTHON_INTERFACE ON)
    endif()
endif()
add_feature_info(PythonInterface ENABLE_PYTHON_INTERFACE "Enables Python interface")

Don't we need an OFF if PYTHON_EXECUTABLE isn't found?

Shatur · 2021-07-29T15:02:19Z

Don't we need an OFF if PYTHON_EXECUTABLE isn't found?

Yes. Good catch!

aytey · 2021-07-29T15:03:32Z

Anyway, what you've shared is correct ... as long as the logic is correct :p

aytey · 2021-07-29T16:18:32Z

I need to play with this, but it seems like the changes look good.

Do you they achieve what you hoped to achieve? Do they solve the "why are these messages WARNINGS?" for you?

aytey

Tried it; works great!

One minor comment (well, a minor comment that applies to all of add_feature_info calls).

CMakeLists.txt

Shatur · 2021-07-29T19:20:08Z

Do you they achieve what you hoped to achieve? Do they solve the "why are these messages WARNINGS?" for you?

Yes, warnings were the main reason if this PR!
I also like how the code looks a little more :)

aytey · 2021-07-29T20:09:30Z

@Shatur are we good? Happy for me to merge?

Shatur · 2021-07-29T20:15:13Z

@andrewvaughanj yes, totally!

aytey · 2021-07-29T20:19:24Z

🚀

aytey · 2021-07-29T20:19:33Z

Thanks, @Shatur!

Shatur · 2021-07-29T20:20:44Z

Thank you too!

Shatur · 2021-07-30T13:29:51Z

@andrewvaughanj, I realized that to use stp as a library in FetchContent we need another option that disables executable compilation. Sorry for bothering so often. Opened another PR: #412.

Shatur mentioned this pull request Jul 29, 2021

Fix various warning issues #410

Merged

Shatur mentioned this pull request Jul 29, 2021

Evaluate moving to recent ABC #375

Open

Shatur changed the title ~~Silence messages~~ Beautify CMake messages Jul 29, 2021

Beautify CMake messages

710a2f1

aytey requested changes Jul 29, 2021

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

aytey approved these changes Jul 29, 2021

View reviewed changes

aytey merged commit 71ba6ba into stp:master Jul 29, 2021

Shatur deleted the silence-messages branch July 29, 2021 20:20

Beautify CMake messages #411

Beautify CMake messages #411

Conversation

Shatur commented Jul 29, 2021

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021

aytey commented Jul 29, 2021

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021

aytey commented Jul 29, 2021

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

aytey commented Jul 29, 2021

aytey left a comment

Choose a reason for hiding this comment

Shatur commented Jul 29, 2021 • edited Loading

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021

aytey commented Jul 29, 2021

aytey commented Jul 29, 2021

Shatur commented Jul 29, 2021

Shatur commented Jul 30, 2021

Shatur commented Jul 29, 2021 •

edited

Loading

Shatur commented Jul 29, 2021 •

edited

Loading

Shatur commented Jul 29, 2021 •

edited

Loading

Shatur commented Jul 29, 2021 •

edited

Loading

Shatur commented Jul 29, 2021 •

edited

Loading

Shatur commented Jul 29, 2021 •

edited

Loading

Shatur commented Jul 29, 2021 •

edited

Loading

Shatur commented Jul 29, 2021 •

edited

Loading

Shatur commented Jul 29, 2021 •

edited

Loading