do_cmake.sh: remove -DCMAKE_BUILD_TYPE=Debug from cmake options #30250
Conversation
| @@ -57,7 +57,7 @@ if type cmake3 > /dev/null 2>&1 ; then | |||
| else | |||
| CMAKE=cmake | |||
| fi | |||
| ${CMAKE} -DCMAKE_BUILD_TYPE=Debug $ARGS "$@" .. || exit 1 | |||
| ${CMAKE} $ARGS "$@" .. || exit 1 | |||
There was a problem hiding this comment.
I'm afraid that outcome of this change may be surprising to many people. Over the time developers might got used to and expect presence of the debug stuff in default-configured builds.
Can we satisfy the requirement for crimson perf testing in another way? If not, then we should consider sending prior notification to ceph-devel or, at least, print message signalizing the change at the end of do_cmake.sh, I think.
There was a problem hiding this comment.
Can we satisfy the requirement for crimson perf testing in another way?
@rzarzynski yes, we can. but i'd keep the script simple and straightforward. i will send a mail to ceph-devel. and print out a warning message if a tty is attached.
There was a problem hiding this comment.
with #30799, i think this change is backward compatible
There was a problem hiding this comment.
There shouldn't be a difference for the perf test bot as we already pass -DCMAKE_BUILD_TYPE=Release there.
There was a problem hiding this comment.
i see, didn't realize that cmake allows us to pass multiple -D<FOO>=<BAR> with the same <FOO>, and the last wins.
|
@tchaikov: is this PR still relevant after merging ceph/ceph-build#1405? I think |
|
@rzarzynski i forgot this PR ! will follow it up in this week. thanks for the reminder! |
doc/install/build-ceph.rst
Outdated
| build a release version of the ceph executables instead. | ||
| .. note:: By default do_cmake.sh will build a release version of ceph with debug | ||
| symbols. Pass '-DCMAKE_BUILD_TYPE=Debug' to do_cmake.sh if you would like to | ||
| build a debug version of the ceph executables instead. |
There was a problem hiding this comment.
There;s a similar passage in README.md
There was a problem hiding this comment.
thanks, the document changes are dropped. as #30799 addresses the backward compatibility issue.
|
the documentation changes should be minimal (if any) if #30799 merges |
so we can use do_cmake.sh for building release builds, which are required for performance tests. Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov
force-pushed
the
wip-do-cmake.sh
branch
from
6045a24
to
4ea4ddc
Compare
ahh, sorry, i missed this comment! |
so we can use do_cmake.sh for building release builds, which are
required for performance tests.
Signed-off-by: Kefu Chai kchai@redhat.com
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard backendjenkins test docsjenkins render docs