[CMake] Remove force overwrite of compiler optimisation flags. by hageboeck · Pull Request #7932 · root-project/root

hageboeck · 2021-04-20T09:42:43Z

In CMake, variables like CMAKE_CXX_FLAGS_RELEASE etc should be cache
variables, so users can set them from outside. ROOT, however, FORCE
overwrote them, so users cannot change anything.
Now, the variables are not set by ROOT at all, we use the CMake
defaults.
It is fine to append or replace substrings, but the variables should NOT
be overwritten to give users some options.

In case ROOT wants to move away from CMake defaults, it can be done like
this:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index e40b84a920..ab41612006 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -29,6 +29,11 @@ endforeach()

 include(cmake/modules/CaptureCommandLine.cmake)

+set(CMAKE_CXX_FLAGS_DEBUG_INIT "-O1 -g")
+set(CMAKE_CXX_FLAGS_ASSERT_INIT "-O2 -g")
+
 project(ROOT)

Fix #6577

hageboeck · 2021-04-20T09:44:38Z

@phsft-bot build just on ROOT-performance-centos8-multicore/default with flags -DCMAKE_CXX_FLAGS_RELEASE=brokenFlag

phsft-bot · 2021-04-20T09:44:43Z

Starting build on ROOT-performance-centos8-multicore/default with flags -DCMAKE_CXX_FLAGS_RELEASE=brokenFlag
How to customize builds

hageboeck · 2021-04-20T09:45:02Z

Above is just a demo. Let's do the real build after we see it fail.

In CMake, variables like CMAKE_CXX_FLAGS_RELEASE etc should be cache variables, so users can set them from outside. ROOT, however, FORCE overwrote them, so users cannot change anything. Now, the variables are not set by ROOT at all, we use the CMake defaults. It is fine to append or replace substrings, but the variables should NOT be overwritten to give users some options. In case ROOT wants to move away from CMake defaults, it can be done like this: diff --git a/CMakeLists.txt b/CMakeLists.txt index e40b84a..ab41612006 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -29,6 +29,11 @@ endforeach() include(cmake/modules/CaptureCommandLine.cmake) +set(CMAKE_CXX_FLAGS_DEBUG_INIT "-O1 -g") +set(CMAKE_CXX_FLAGS_ASSERT_INIT "-O2 -g") + project(ROOT)

phsft-bot · 2021-04-20T09:54:14Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

stwunsch · 2021-04-20T10:40:32Z

Why should it break with the broken flag? We don't build Release in the PRs, I guess. Is it RelWithDebInfo?

hageboeck · 2021-04-20T13:35:55Z

I didn't think it is RelWithDeb. I pushed too fast, so the build hadn't even started, so we wouldn't know for sure.

hageboeck · 2021-04-20T13:36:44Z

@phsft-bot build just on ROOT-fedora30/cxx14 with flags -DCMAKE_CXX_FLAGS_RELEASE=brokenFlag

phsft-bot · 2021-04-20T13:37:01Z

Starting build on ROOT-fedora30/cxx14 with flags -DCMAKE_CXX_FLAGS_RELEASE=brokenFlag
How to customize builds

phsft-bot · 2021-04-20T13:41:45Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-04-20T13:41:37.174Z] FAILED: interpreter/llvm/src/lib/Demangle/CMakeFiles/LLVMDemangle.dir/Demangle.cpp.o
[2021-04-20T13:41:37.174Z] c++: error: brokenFlag: No such file or directory

hageboeck · 2021-04-20T14:11:52Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-04-20T13:41:37.174Z] FAILED: interpreter/llvm/src/lib/Demangle/CMakeFiles/LLVMDemangle.dir/Demangle.cpp.o

[2021-04-20T13:41:37.174Z] c++: error: brokenFlag: No such file or directory

I guess that settles it @stwunsch. 🙂

CI builds are RELEASE
Setting the flags from the outside works.
When you don't set anything, the CI passes.
The assert build that we have in the nightlies can be activated using either
- cmake -Dasserts=On ... or
- cmake -DCMAKE_CXX_FLAGS_RELEASE="-O3"

Axel-Naumann · 2021-04-20T14:45:47Z

CMakeLists.txt

+  string(REGEX REPLACE "-.NDEBUG" "" CMAKE_CXX_FLAGS_${BUILD_TYPE} "${CMAKE_CXX_FLAGS_${BUILD_TYPE}}")
+  string(REGEX REPLACE "-.NDEBUG" "" CMAKE_C_FLAGS_${BUILD_TYPE} "${CMAKE_C_FLAGS_${BUILD_TYPE}}")


Why can I not -INDEBUG? Why do you break my favorite CXX flag -DNDEBUG_EVERYTHING=42? ;-) What about -DNDEBUG=1?

Suggested change

string(REGEX REPLACE "-.NDEBUG" "" CMAKE_CXX_FLAGS_${BUILD_TYPE} "${CMAKE_CXX_FLAGS_${BUILD_TYPE}}")

string(REGEX REPLACE "-.NDEBUG" "" CMAKE_C_FLAGS_${BUILD_TYPE} "${CMAKE_C_FLAGS_${BUILD_TYPE}}")

string(REGEX REPLACE "-[DU]NDEBUG(=[^ ]+)? " "" CMAKE_CXX_FLAGS_${BUILD_TYPE} "${CMAKE_CXX_FLAGS_${BUILD_TYPE}}")

string(REGEX REPLACE "-[DU]NDEBUG(=[^ ]+)? " "" CMAKE_C_FLAGS_${BUILD_TYPE} "${CMAKE_C_FLAGS_${BUILD_TYPE}}")

Maybe better (since CMake 3.6):

Suggested change

string(REGEX REPLACE "-.NDEBUG" "" CMAKE_CXX_FLAGS_${BUILD_TYPE} "${CMAKE_CXX_FLAGS_${BUILD_TYPE}}")

string(REGEX REPLACE "-.NDEBUG" "" CMAKE_C_FLAGS_${BUILD_TYPE} "${CMAKE_C_FLAGS_${BUILD_TYPE}}")

list(FILTER CMAKE_CXX_FLAGS_${BUILD_TYPE} EXCLUDE REGEX "-[UD]NDEBUG(=.*)?")

list(FILTER CMAKE_C_FLAGS_${BUILD_TYPE} EXCLUDE REGEX "-[UD]NDEBUG(=.*)?")

Ah, true. I didn't know that people could have those as their favourite flags.

My preferred solution is to use
cmake -DCMAKE_CXX_FLAGS_RELEASE=-O3 <src> - less CMake code on our side.

If -Dasserts=On is desired, though, we can extend that regex.

These are not lists, so have to go for the other solution.

cmake/modules/SetUpLinux.cmake

phsft-bot · 2021-04-20T15:01:26Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

oshadura

I am super supporting the idea to roll back to CMake defaults !!! Let's cross our fingers that testing whole ROOT test suites will work...I just have a question, why we couldn't do this before (e.g. what was fa reason to use specific hardcodeded flags)?

hageboeck · 2021-04-20T15:13:48Z

I am super supporting the idea to roll back to CMake defaults !!! Let's cross our fingers that testing whole ROOT test suites will work...I just have a question, why we couldn't do this before (e.g. what was fa reason to use specific hardcodeded flags)?

To be honest:
I think we didn't know better. It looks to me like people thought that they had to to this manually. The only challenge is probably if you want to get other defaults than CMake, but the way to do this is also known - the CMAKE_CXX_*_INIT, which you have to set before project(ROOT).

oshadura · 2021-04-20T15:17:35Z

@hageboeck @stwunsch I will vote then to try it in the master and in case of something I can try to setup flags as you told: before project(ROOT) using CMAKE_CXX_*_INIT vars...

hageboeck · 2021-04-20T15:19:10Z

@hageboeck @stwunsch I will vote then to try it in the master and in case of something I can try to setup flags as you told: before project(ROOT) using CMAKE_CXX_*_INIT vars...

Just note that before project(ROOT) you don't know the compiler. You know whether it's linux or Windows, you can query Makefiles or ninja, etc, but you cannot ask if it's clang, for example.

phsft-bot · 2021-04-20T17:08:33Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-04-20T17:07:53.553Z] collect2: error: ld returned 1 exit status
[2021-04-20T17:08:32.510Z] collect2: error: ld returned 1 exit status

phsft-bot · 2021-04-20T17:25:48Z

Build failed on ROOT-fedora31/noimt.
Running on root-fedora-31-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-04-20T17:41:34Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-04-20T17:43:56Z

Build failed on mac1014/python3.
Running on macphsft17.dyndns.cern.ch:/build/jenkins/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-04-20T18:41:33Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-04-20T18:49:50Z

Build failed on ROOT-fedora30/cxx14.
Running on root-fedora30-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-04-20T20:47:22Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

oshadura · 2021-04-21T10:52:17Z

Looks like all these tests are failed because of timeout?

Axel-Naumann · 2021-04-21T10:55:45Z

That's likely significant i.e. this PR might introduce wrong flags! Please check the actual compiler invocation ( @hageboeck I suppose).

hageboeck · 2021-04-21T10:58:09Z

I had a look. The CI seems to set asserts from the outside, and the final flags for Release, which starts out at -O3 -DNDEBUG, seem to be "". Will check.

hageboeck · 2021-04-21T10:59:52Z

I suspect that it's in Axel's list filtering. Will check locally with some printouts.

hageboeck · 2021-04-21T11:02:55Z

... confirmed. Will fix.

hageboeck · 2021-04-21T11:07:05Z

Explanation:
list(FILTER REGEX ... applied to the list -O3 -DNDEBUG; (I added the list separator for clarity) deletes the list element -O3 -DNDEBUG from the list, so empty list after the modification.
I probably have to go back to string(... REGEX to do string manipulations.

Co-authored-by: Axel Naumann <[email protected]>

phsft-bot · 2021-04-21T11:21:15Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

hageboeck · 2021-04-21T11:27:33Z

@Axel-Naumann I reverted most of your suggestions, but the regex is still your updated one. I expect the CI to go as desired now. See what ROOT prints for ROOT-fedora30/cxx14:

13:22:25  System          Linux-5.0.9-301.fc30.x86_64
13:22:25  Processor       16 core Intel Core Processor (Broadwell, IBRS) (x86_64)
13:22:25  Build type      Release
13:22:25  Install path    /home/sftnight/build/workspace/root-pullrequests-build/install
13:22:25  Compiler        GNU 9.3.1
13:22:25  Compiler flags:
13:22:25  C                -fdiagnostics-color=always -Wno-implicit-fallthrough -pipe -Wall -W -pthread -O2 
13:22:25  C++              -fdiagnostics-color=always -std=c++14 -Wno-implicit-fallthrough -Wno-noexcept-type -pipe  -Wshadow -Wall -W -Woverloaded-virtual -fsigned-char -pthread -O2 
13:22:25  Linker flags:
13:22:25  Executable       -rdynamic
13:22:25  Module          
13:22:25  Shared           -Wl,--no-undefined -Wl,--hash-style="both"

Interesting side note:
Does CMake follow the defaults that are used for making packages? fedora and centos8 get -O2, I get -O3 on Mac.

oshadura · 2021-04-21T13:48:11Z

@Axel-Naumann about the note, interesting, but it looks like :)

oshadura

As I already told you, let's try: LGTM! and thanks a lot for finding such a nice solution!

hageboeck requested a review from oshadura as a code owner April 20, 2021 09:42

hageboeck requested review from Axel-Naumann and stwunsch April 20, 2021 09:43

hageboeck force-pushed the CMAKE_CXX_FLAGS branch from 4c7c19c to 9901376 Compare April 20, 2021 09:54

Axel-Naumann reviewed Apr 20, 2021

View reviewed changes

cmake/modules/SetUpLinux.cmake Outdated Show resolved Hide resolved

oshadura reviewed Apr 20, 2021

View reviewed changes

etejedor assigned oshadura Apr 21, 2021

Apply suggestions from code review

5166e33

Co-authored-by: Axel Naumann <[email protected]>

hageboeck force-pushed the CMAKE_CXX_FLAGS branch from 467ebed to 5166e33 Compare April 21, 2021 11:21

oshadura self-requested a review April 21, 2021 17:06

oshadura approved these changes Apr 21, 2021

View reviewed changes

hageboeck merged commit c77cf0d into root-project:master Apr 22, 2021

hageboeck deleted the CMAKE_CXX_FLAGS branch April 22, 2021 07:24

		string(REGEX REPLACE "-.NDEBUG" "" CMAKE_CXX_FLAGS_${BUILD_TYPE} "${CMAKE_CXX_FLAGS_${BUILD_TYPE}}")
		string(REGEX REPLACE "-.NDEBUG" "" CMAKE_C_FLAGS_${BUILD_TYPE} "${CMAKE_C_FLAGS_${BUILD_TYPE}}")

Conversation

hageboeck commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hageboeck commented Apr 20, 2021

Uh oh!

phsft-bot commented Apr 20, 2021

Uh oh!

hageboeck commented Apr 20, 2021

Uh oh!

phsft-bot commented Apr 20, 2021

Uh oh!

stwunsch commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hageboeck commented Apr 20, 2021

Uh oh!

hageboeck commented Apr 20, 2021

Uh oh!

phsft-bot commented Apr 20, 2021

Uh oh!

phsft-bot commented Apr 20, 2021

Errors:

Uh oh!

hageboeck commented Apr 20, 2021

Errors:

Uh oh!

Axel-Naumann Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hageboeck Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

hageboeck Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phsft-bot commented Apr 20, 2021

Uh oh!

oshadura left a comment

Choose a reason for hiding this comment

Uh oh!

hageboeck commented Apr 20, 2021

Uh oh!

oshadura commented Apr 20, 2021

Uh oh!

hageboeck commented Apr 20, 2021

Uh oh!

phsft-bot commented Apr 20, 2021

Errors:

Uh oh!

phsft-bot commented Apr 20, 2021

Failing tests:

Uh oh!

phsft-bot commented Apr 20, 2021

Failing tests:

Uh oh!

phsft-bot commented Apr 20, 2021

Failing tests:

Uh oh!

phsft-bot commented Apr 20, 2021

Failing tests:

Uh oh!

phsft-bot commented Apr 20, 2021

Failing tests:

Uh oh!

phsft-bot commented Apr 20, 2021

Failing tests:

Uh oh!

oshadura commented Apr 21, 2021

Uh oh!

Axel-Naumann commented Apr 21, 2021

Uh oh!

hageboeck commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hageboeck commented Apr 21, 2021

Uh oh!

hageboeck commented Apr 21, 2021

hageboeck commented Apr 20, 2021 •

edited

Loading

stwunsch commented Apr 20, 2021 •

edited

Loading

Axel-Naumann Apr 20, 2021 •

edited

Loading

hageboeck commented Apr 21, 2021 •

edited

Loading