Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PROTON-2331 Allow producing a RelWithDebInfo build with asserts enabled #297

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

PROTON-2331 Allow producing a RelWithDebInfo build with asserts enabled #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
162f926
PROTON-2331 Allow producing a RelWithDebInfo build with asserts enabled
jiridanek Feb 9, 2021
372eb95
Update .travis.yml
jiridanek Feb 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,12 @@ jobs:
brew install libuv swig pkgconfig jsoncpp
- name: cmake configure
working-directory: ${{env.BuildDir}}
run: cmake "${{github.workspace}}" "-DCMAKE_BUILD_TYPE=${BuildType}" "-DCMAKE_INSTALL_PREFIX=${InstallPrefix}" ${{matrix.cmake_extra}}
run: >
cmake "${{github.workspace}}"
"-DCMAKE_BUILD_TYPE=${BuildType}"
"-DCMAKE_INSTALL_PREFIX=${InstallPrefix}"
"-DPN_ENABLE_ASSERTIONS=ON"
${{matrix.cmake_extra}}
shell: bash
- name: cmake build/install
run: cmake --build "${BuildDir}" --config ${BuildType} -t install
Expand Down
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ jobs:
env:
- PATH="/usr/local/opt/python/libexec/bin:/usr/local/bin:$PATH"
- PKG_CONFIG_PATH='/usr/local/opt/[email protected]/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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below

# TODO PROTON-2225: c-threaderciser sometimes fails with assertion error
# python-tox-test segfaults and ruby tests do not start due to dynamic library issues
- QPID_PROTON_CTEST_ARGS="--exclude-regex 'c-threaderciser|python-tox-test|ruby.*'"
Expand Down
32 changes: 32 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm considering moving all this into a .cmake file and then including it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be better - I'm not sure.

if(CMAKE_BUILD_TYPE STREQUAL "Debug" )
option(PN_ENABLE_ASSERTIONS "Enable assertions" ON)
else()
option(PN_ENABLE_ASSERTIONS "Enable assertions" OFF)
endif()
if(PN_ENABLE_ASSERTIONS)
Comment on lines +138 to +142
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please call this ENABLE_ASSERTIONS - we don't use a PN_ prefix as everything in the build is Proton!

Copy link
Contributor Author

@jiridanek jiridanek Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# MSVC doesn't like _DEBUG on release builds.
if(NOT MSVC)
add_definitions(-D_DEBUG)
endif()
Comment on lines +144 to +146
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, it does look useless. I only included it because thats what LLVM does, and it did not look harmful in any way.

# On non-Debug builds cmake automatically defines NDEBUG, so we explicitly undefine it:
if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
# NOTE: use `add_compile_options` rather than `add_definitions` since
# `add_definitions` does not support generator expressions.
add_compile_options($<$<OR:$<COMPILE_LANGUAGE:C>,$<COMPILE_LANGUAGE:CXX>>:-UNDEBUG>)

# Also remove /D NDEBUG to avoid MSVC warnings about conflicting defines.
foreach (flags_var_to_scrub
CMAKE_CXX_FLAGS_RELEASE
CMAKE_CXX_FLAGS_RELWITHDEBINFO
CMAKE_CXX_FLAGS_MINSIZEREL
CMAKE_C_FLAGS_RELEASE
CMAKE_C_FLAGS_RELWITHDEBINFO
CMAKE_C_FLAGS_MINSIZEREL)
string (REGEX REPLACE "(^| )[/-]D *NDEBUG($| )" " "
"${flags_var_to_scrub}" "${${flags_var_to_scrub}}")
endforeach()
endif()
endif()

# Try to keep any platform specific overrides together here:

# MacOS has a bunch of differences in build tools and process and so we have to turn some things
Expand Down