PROTON-2331 Allow producing a RelWithDebInfo build with asserts enabled #297
Conversation
| @@ -132,6 +132,38 @@ if (CMAKE_BUILD_TYPE MATCHES "Coverage") | |||
| COMMAND ${CMAKE_SOURCE_DIR}/scripts/record-coverage.sh ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}) | |||
| endif() | |||
|
|
|||
| # Enable asserts for C/C++ code if requested | |||
| # from the LLVM project, https://opensource.apple.com/source/llvmCore/llvmCore-2358.3/CMakeLists.txt.auto.html | |||
There was a problem hiding this comment.
I'm considering moving all this into a .cmake file and then including it here.
There was a problem hiding this comment.
That might be better - I'm not sure.
There was a problem hiding this comment.
I wonder whether it would be better to create a new CMAKE_BUILD_TYPE for this rather than modify RelWithDebInfo perhaps call it SensibleRelWithDebInfo :) or more seriously maybe RelWithDebInfoAsserts. Then you can just use a different CMAKE_BUILD_TYPE to get this. I think this is more idiomatic maybe? We do this for coverage too so it's not a new idea for us.
| option(PN_ENABLE_ASSERTIONS "Enable assertions" ON) | ||
| else() | ||
| option(PN_ENABLE_ASSERTIONS "Enable assertions" OFF) | ||
| endif() | ||
| if(PN_ENABLE_ASSERTIONS) |
There was a problem hiding this comment.
Please call this ENABLE_ASSERTIONS - we don't use a PN_ prefix as everything in the build is Proton!
There was a problem hiding this comment.
Ack, will rename.
It has the same justification as the pn_ prefix in the C library. Somebody might include this CMake build in their own project (as a subproject: CMake supports fetching subprojects over HTTP, Git, ....) and then there would be value in being able to configure options for each project separately. I admit that here is a poor place where to start introducing the convention. Especially since I dont intend to apply it uniformly everywhere.
| if(NOT MSVC) | ||
| add_definitions(-D_DEBUG) | ||
| endif() |
There was a problem hiding this comment.
This is not necessary at all - all we need to do is not define NDEBUG no need to define _DEBUG which does nothing for our code or any compiler library except MSVC.
There was a problem hiding this comment.
Ack, it does look useless. I only included it because thats what LLVM does, and it did not look harmful in any way.
| @@ -117,7 +117,7 @@ jobs: | |||
| env: | |||
| - PATH="/usr/local/opt/python/libexec/bin:/usr/local/bin:$PATH" | |||
| - PKG_CONFIG_PATH='/usr/local/opt/openssl@1.1/lib/pkgconfig' | |||
| - QPID_PROTON_CMAKE_ARGS='-DCMAKE_OSX_DEPLOYMENT_TARGET=10.15 -DTHREADERCISER=ON' | |||
| - QPID_PROTON_CMAKE_ARGS='-DCMAKE_OSX_DEPLOYMENT_TARGET=10.15 -DPN_ENABLE_ASSERTIONS=ON -DTHREADERCISER=ON' | |||
| @@ -132,6 +132,38 @@ if (CMAKE_BUILD_TYPE MATCHES "Coverage") | |||
| COMMAND ${CMAKE_SOURCE_DIR}/scripts/record-coverage.sh ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}) | |||
| endif() | |||
|
|
|||
| # Enable asserts for C/C++ code if requested | |||
| # from the LLVM project, https://opensource.apple.com/source/llvmCore/llvmCore-2358.3/CMakeLists.txt.auto.html | |||
There was a problem hiding this comment.
That might be better - I'm not sure.
|
I want to contribute.. this project |
@Gauravmauryaios Know any C++? Try solving one of the coverage improvement tasks. This one looks good https://issues.apache.org/jira/browse/PROTON-2363. If you need help, read previous questions on the mailing list here https://lists.apache.org/thread.html/r061203f3269108ccc5e7fd853e4ad519fc311cb96d3d6bdd9360965e%40%3Cusers.qpid.apache.org%3E, or ask your own. |
No description provided.