Page MenuHomePhabricator

[CMAKE] Make RelWithDebInfo the default CMake configuration
ClosedPublic

Authored by Fabien on Mar 4 2019, 21:03.

Details

Summary

This set RelWithDebInfo as the default build type for CMake. This
makes the default optimization flags identical to those from
configure.

Depends on D2651

Test Plan
mkdir buildcmake && cd buildcmake
cmake -GNinja ..
ninja -v

Check that the -g -O2 flags are set in the compile commands for both
CC and CXX compile lines.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_default_RelDebWithInfo
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5134
Build 8331: Bitcoin ABC Buildbot (legacy)
Build 8330: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Mar 4 2019, 21:15

This generates a ton of warning, so there is likely something not correct about what RelWithDebInfo does. It's not -O2 or -g though.

This revision now requires changes to proceed.Mar 4 2019, 21:15

@deadalnix can you provide an example of what warning you are getting ?

deadalnix requested changes to this revision.Mar 5 2019, 20:51

Please do not change the config to something that's not working as expected. RelWithDebInfo is not working properly yet.

This revision now requires changes to proceed.Mar 5 2019, 20:51
Fabien requested review of this revision.Mar 13 2019, 14:56

Requesting review again since D2651 is landed.
This will add -g -O2 by default.

deadalnix requested changes to this revision.Apr 17 2019, 23:20
deadalnix added inline comments.
src/CMakeLists.txt
23 ↗(On Diff #7602)

Not if you change the configuration after the fact...

31 ↗(On Diff #7602)

This comes after flags have been modified the line just before. This is probably a bad idea.

This revision now requires changes to proceed.Apr 17 2019, 23:20

Change configuration before removing the NDEBUG flag

This revision is now accepted and ready to land.Apr 19 2019, 12:52
deadalnix requested changes to this revision.Apr 19 2019, 12:53
deadalnix added inline comments.
src/CMakeLists.txt
24 ↗(On Diff #8119)

Maybe it's better to put this in the root CMakeLists.txt

This revision now requires changes to proceed.Apr 19 2019, 12:53

Move to the root CMakeLists.txt

This revision is now accepted and ready to land.May 4 2019, 17:50
This revision was landed with ongoing or failed builds.May 6 2019, 08:05
This revision was automatically updated to reflect the committed changes.