CMAKE_CXX_FLAGS are overwritten

[jacobmerson]

The CMAKE_CXX_FLAGS variable is overwritten by bob_begin_cxx_flags. I see that the variables SCOREC_CXX_FLAGS and SCOREC_EXTRA_CXX_FLAGS can be used instead.

However, this behavior is not "standard" and I had to go into the bob.cmake file to figure out how to change the flags.

I think it makes more sense to have the default behavior for CMAKE_CXX_FLAGS do what SCOREC_EXTRA_CXX_FLAGS currently does.

If we don't want to go that route, then there needs to be some sort of documentation on how to set the cxx flags and a comment in the CMakeLists.txt

[jacobmerson]

As evidence of this problem see issue #338. Symbols were not exporting due to us overwriting flags in bob.cmake and this made the debug process much harder.

[KennethEJansen]

Obviously, I agree with Jacob. I might have below average CMAKE skills but, if the intent is to control/protect the flags being set to somehow reduce mistakes that objective is not being met. A few more "seasoned" CMAKE gurus also had eyes on my CMAKE configuration and did not see my "foul" of adding a flag and clobbering my request for symbols. It was not until the third person plainly told me that my binary had no symbols even though the file command reported that it was not stripped that it occurred to me that adding -fpermissive to kill the warnings that were promoted to errors regarding casting MIGHT have been the culprit.

[cwsmith]

The documentation here:

https://github.com/SCOREC/core/wiki/General-Build-instructions#configure

describes that SCOREC_CXX_FLAGS overrides while SCOREC_CXX_EXTRA_FLAGS adds.

Clearly, this is not documented enough and there is not enough of a warning. Towards Jacob's point, this wrapper layer for flags maybe against modern CMake best practices.

As Ken said, the main intent of SCOREC_CXX_FLAGS and SCOREC_CXX_EXTRA_FLAGS is to prevent the user of pumi from unintentionally overriding default flags that pumi developers have deemed important. I suggest we do the following in the short term:

• add a highly visible warning message to the CMake output when the user sets either SCOREC_CXX_FLAGS or CMAKE_CXX_FLAGS that describes what is happening (override vs ignored), and
• add comments to CMakeLists.txt describing how compiler flags are controlled (similar to what is in the wiki page).

and towards longer term improvements, email the CMake mailing list to get opinions on the approach.

edit: changed todo

[jacobmerson]

The problem isn't so much that it's not documented, but that overriding CMAKE_CXX_FLAGS (or any other CMAKE_ variable ith a set() command is not standard practice. Most C++ devs who have used CMake will just expect whatever you set there gets passed through.

looking through bob.cmake

I see almost no flags that we should be explicitly setting...

• We should not be explicitly setting -std=c++11 explicitly, see Update CMake CXX11 flags and disable openmpi warning #305, Require C++11 for builds #306.
• We should not be setting -Werror this creates all sorts of problems for people that are building on different compilers. For example see: https://alexreinking.com/blog/how-to-use-cmake-without-the-agonizing-pain-part-2.html
• We should not set the optimization levels explicitly (-O0, -O1, etc.). Not good info in the CMake documentation, but the defaults are here. If -O3 should not be used, bob.cmake needs a comment explaining that. If we want to set different defaults we can accomplish this with cache variables so the user can override.
• Turning off the following warnings are probably ok -Wno-c++98-compat-pedantic -Wno-c++98-compat,-Wno-strict-overflow as long as we do so in a way that doesn't override user flags, and as long as we don't start turning off all the warnings...

If we want to provide default flags for our developers like -Werror we should use provide either a toolchain file, or CMakePresets.json file (although this isn't available until cmake 3.19).

I can take responsibility for updating the CMake stuff, although I have a lot on my plate with trying to finish up my thesis, so it won't happen immediately. I'm also not familiar with what the XSDK folks require.

[cwsmith]

Given that cmake has moved on since bob.cmake was written I think we are due for a more comprehensive and cohesive overhaul. The current approach basically defines a custom interface to control some key cmake variables. If users understand it, it works, but there is a lack of documentation and 'strange' things happen when users make changes by directly changing cmake variables. I'm fine with overhauling it to make it more intuitive.

The other big feature bob.cmake provides is an export capability for creating .cmake files placed with the install to support find_package(...). IIRC, cmake cannot completely populate these files by itself yet with all the things we currently have (transitive dependencies, compiler flags, versions, etc..).

I don't think there is a big rush in the short term on making these changes/improvements as most users have either kludged their way through problems or know what to do. If you can help after doing thesis work that would be much appreciated.

Responses to the bob.cmake review items are below.

• --std=c++11 is not on by default -

  core/CMakeLists.txt

  Line 14 in dd107ee

  option(SCOREC_ENABLE_CXX11 "enable compilation with c++11 support" NO)

  . There is probably a more modern cmake way to control this
• I agree, general 'users' (those just calling our APIs or using the binaries) should be have to worry -Werror etc. Developer builds should have them enabled though to catch problems before they become bugs and keep the build output clean. The cmake 'preset' json file discussed in the blog you posted is interesting way to support a dev build mode. Another, possibly simpler, alternative is to define a cmake build type ( named developer or some variation of it) that has the warnings and errors as defaults. I haven't tried either, or read up on them in depth, so I don't know the pros and cons. From what I understand, toolchain files are meant to support alternative sets of build tools (e.g., for cross compiling) and the build type of json would be a better approach.
• The optimization levels mean different things for different compilers. Most users just want code that is performs reasonably well; -O2 is likely a safe choice. If we make the default build type for non-developers release then letting cmake populate this flag seems OK.
• The C98 warnings look like are another 'developer' flag.