pnoltes commented on a change in pull request #228:
URL: https://github.com/apache/celix/pull/228#discussion_r422497443
##########
File path: CMakeLists.txt
##########
@@ -39,27 +39,35 @@ if (ENABLE_TESTING)
endif()
endif ()
-set(ENABLE_W_ERROR ON)
set(ENABLE_MORE_WARNINGS OFF)
-IF (ANDROID)
- set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -Wall ${CMAKE_C_FLAGS}")
-ELSE ()
- set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -fPIC ${CMAKE_C_FLAGS}")
- set(CMAKE_CXX_FLAGS "-std=c++11 -fno-rtti ${CMAKE_CXX_FLAGS}")
- set(CMAKE_C_FLAGS "-Wall -Werror ${CMAKE_C_FLAGS}")
- set(CMAKE_CXX_FLAGS "-Wall -Wextra -Weffc++ ${CMAKE_CXX_FLAGS}")
- set(CMAKE_C_FLAGS_DEBUG "-g -DDEBUG ${CMAKE_C_FLAGS}")
- set(CMAKE_CXX_FLAGS_DEBUG "-g -DDEBUG ${CMAKE_CXX_FLAGS}")
-ENDIF()
+# Set C specific flags
+set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -fPIC ${CMAKE_C_FLAGS}")
+set(CMAKE_C_FLAGS "-Wall -Werror ${CMAKE_C_FLAGS}")
+
+# Set C++ specific flags
+set(CMAKE_CXX_FLAGS "-std=c++11 -fno-rtti ${CMAKE_CXX_FLAGS}")
+set(CMAKE_CXX_FLAGS "-Wall -Werror -Wextra -Weffc++ ${CMAKE_CXX_FLAGS}")
-IF(APPLE)
+if(APPLE)
set(CMAKE_MACOSX_RPATH 1)
-ELSE ()
+endif()
+
+if(NOT APPLE)
set(CMAKE_C_FLAGS "-pthread ${CMAKE_C_FLAGS}")
set(CMAKE_CXX_FLAGS "-pthread ${CMAKE_CXX_FLAGS}")
set(CMAKE_EXE_LINKER_FLAGS "-pthread ${CMAKE_EXE_LINKER_FLAGS}")
-ENDIF()
+endif()
+
+# Set compiler specific options
+if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
Review comment:
Is a test on Clang enough?
For sanitizers the following was needed:
```CMake
if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR "${CMAKE_CXX_COMPILER_ID}"
STREQUAL "AppleClang")
```
##########
File path: CMakeLists.txt
##########
@@ -82,15 +90,25 @@ if (ENABLE_MORE_WARNINGS)
set(CMAKE_CXX_EXTRA_FLAGS "-Wold-style-cast -Wuseless-cast
${CMAKE_CXX_EXTRA_FLAGS}")
endif()
endif()
-
+
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_EXTRA_FLAGS} ${CMAKE_CXX_FLAGS}")
- set(CMAKE_CXX_FLAGS_DEBUG "-Werror ${CMAKE_CXX_EXTRA_FLAGS}
${CMAKE_CXX_FLAGS_DEBUG}")
endif()
-if(ENABLE_W_ERROR)
- set(CMAKE_CXX_FLAGS "-Werror ${CMAKE_CXX_FLAGS}")
- set(CMAKE_CXX_FLAGS_DEBUG "-Werror ${CMAKE_CXX_FLAGS_DEBUG}")
-endif()
+# Set build type specific flags
+# Debug
+set(CMAKE_C_FLAGS_DEBUG "-g -DDEBUG ${CMAKE_C_FLAGS}")
+set(CMAKE_CXX_FLAGS_DEBUG "-g -DDEBUG ${CMAKE_CXX_FLAGS}")
+set(CMAKE_DEBUG_POSTFIX "d")
+
+# Release with debug info
+# Optimization is disabled, enabled it will result in segfaults due to the
ffi_call function
+set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O0 -g -DNDEBUG ${CMAKE_C_FLAGS}")
Review comment:
Can this be solved by adding the -O0 option only for libdfi?
e.g:
```CMake
#libs/dfi/CMakeLists.txt
# Optimization is disabled, enabled it will result in segfaults due to the
ffi_call function
target_compile_options(dfi PRIVATE -O0)
```
##########
File path: cmake/cmake_celix/BundlePackaging.cmake
##########
@@ -189,10 +190,13 @@ function(add_celix_bundle)
if (NOT DEFINED BUNDLE_SYMBOLIC_NAME)
set(BUNDLE_SYMBOLIC_NAME ${BUNDLE_TARGET_NAME})
endif ()
+
if (NOT DEFINED BUNDLE_FILENAME)
- set(BUNDLE_FILENAME ${BUNDLE_TARGET_NAME}.zip)
+ set(BUNDLE_FILENAME ${BUNDLE_TARGET_NAME})
endif ()
+ set(BUNDLE_FILENAME ${BUNDLE_FILENAME}-${CMAKE_BUILD_TYPE}.zip)
Review comment:
If would prefer that an postfix can be excluded for a configurable build
type
I took the liberty to create a diff for that:
```diff
diff --git a/cmake/cmake_celix/BundlePackaging.cmake
b/cmake/cmake_celix/BundlePackaging.cmake
index b34fa138..3fb866c4 100644
--- a/cmake/cmake_celix/BundlePackaging.cmake
+++ b/cmake/cmake_celix/BundlePackaging.cmake
@@ -15,6 +15,9 @@
# specific language governing permissions and limitations
# under the License.
+
+set(CELIX_NO_POSTFIX_BUILD_TYPE "RelWithDebInfo" CACHE STRING "The build
type used for creating bundle without a build type postfix.")
+
find_program(JAR_COMMAND jar NO_CMAKE_FIND_ROOT_PATH)
if(JAR_COMMAND AND NOT CELIX_USE_ZIP_INSTEAD_OF_JAR)
@@ -194,7 +197,11 @@ function(add_celix_bundle)
set(BUNDLE_FILENAME ${BUNDLE_TARGET_NAME})
endif ()
- set(BUNDLE_FILENAME ${BUNDLE_FILENAME}-${CMAKE_BUILD_TYPE}.zip)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "${CELIX_NO_POSTFIX_BUILD_TYPE}")
+ set(BUNDLE_FILENAME ${BUNDLE_FILENAME}.zip)
+ else ()
+ set(BUNDLE_FILENAME ${BUNDLE_FILENAME}-${CMAKE_BUILD_TYPE}.zip)
+ endif ()
set(BUNDLE_FILE "${CMAKE_CURRENT_BINARY_DIR}/${BUNDLE_FILENAME}")
#set(BUNDLE_CONTENT_DIR
"${CMAKE_CURRENT_BINARY_DIR}/${BUNDLE_TARGET_NAME}_content")
diff --git a/libs/framework/gtest/CMakeLists.txt
b/libs/framework/gtest/CMakeLists.txt
index ab94fff6..7777db49 100644
--- a/libs/framework/gtest/CMakeLists.txt
+++ b/libs/framework/gtest/CMakeLists.txt
@@ -42,6 +42,16 @@ add_executable(test_framework
target_link_libraries(test_framework Celix::framework CURL::libcurl
GTest::gtest)
add_dependencies(test_framework simple_test_bundle1_bundle
simple_test_bundle2_bundle simple_test_bundle3_bundle
simple_test_bundle4_bundle simple_test_bundle5_bundle
bundle_with_exception_bundle unresolveable_bundle_bundle)
+target_compile_definitions(test_framework PRIVATE
+
-DSIMPLE_TEST_BUNDLE1_LOCATION=\"$<TARGET_PROPERTY:simple_test_bundle1,BUNDLE_FILE>\"
+
-DSIMPLE_TEST_BUNDLE2_LOCATION=\"$<TARGET_PROPERTY:simple_test_bundle2,BUNDLE_FILE>\"
+
-DSIMPLE_TEST_BUNDLE3_LOCATION=\"$<TARGET_PROPERTY:simple_test_bundle3,BUNDLE_FILE>\"
+
-DSIMPLE_TEST_BUNDLE4_LOCATION=\"$<TARGET_PROPERTY:simple_test_bundle4,BUNDLE_FILENAME>\"
+
-DSIMPLE_TEST_BUNDLE5_LOCATION=\"$<TARGET_PROPERTY:simple_test_bundle5,BUNDLE_FILENAME>\"
+
-DTEST_BUNDLE_WITH_EXCEPTION_LOCATION=\"$<TARGET_PROPERTY:bundle_with_exception,BUNDLE_FILE>\"
+
-DTEST_BUNDLE_UNRESOLVEABLE_LOCATION=\"$<TARGET_PROPERTY:unresolveable_bundle,BUNDLE_FILE>\"
+)
+
target_include_directories(test_framework PRIVATE ../src)
configure_file(config.properties.in config.properties @ONLY)
diff --git a/libs/framework/gtest/src/bundle_context_bundles_tests.cpp
b/libs/framework/gtest/src/bundle_context_bundles_tests.cpp
index d30d7eaa..dc16012c 100644
--- a/libs/framework/gtest/src/bundle_context_bundles_tests.cpp
+++ b/libs/framework/gtest/src/bundle_context_bundles_tests.cpp
@@ -35,13 +35,13 @@ public:
celix_bundle_context_t *ctx = nullptr;
celix_properties_t *properties = nullptr;
- const char * const TEST_BND1_LOC = "simple_test_bundle1-" BUILD_TYPE
".zip";
- const char * const TEST_BND2_LOC = "simple_test_bundle2-" BUILD_TYPE
".zip";
- const char * const TEST_BND3_LOC = "simple_test_bundle3-" BUILD_TYPE
".zip";
- const char * const TEST_BND4_LOC = "simple_test_bundle4-" BUILD_TYPE
".zip";
- const char * const TEST_BND5_LOC = "simple_test_bundle5-" BUILD_TYPE
".zip";
- const char * const TEST_BND_WITH_EXCEPTION_LOC =
"bundle_with_exception-" BUILD_TYPE ".zip";
- const char * const TEST_BND_UNRESOLVEABLE_LOC = "unresolveable_bundle-"
BUILD_TYPE ".zip";
+ const char * const TEST_BND1_LOC = "" SIMPLE_TEST_BUNDLE1_LOCATION "";
+ const char * const TEST_BND2_LOC = "" SIMPLE_TEST_BUNDLE2_LOCATION "";
+ const char * const TEST_BND3_LOC = "" SIMPLE_TEST_BUNDLE3_LOCATION "";
+ const char * const TEST_BND4_LOC = "" SIMPLE_TEST_BUNDLE4_LOCATION "";
+ const char * const TEST_BND5_LOC = "" SIMPLE_TEST_BUNDLE5_LOCATION "";
+ const char * const TEST_BND_WITH_EXCEPTION_LOC = ""
TEST_BUNDLE_WITH_EXCEPTION_LOCATION "";
+ const char * const TEST_BND_UNRESOLVEABLE_LOC = ""
TEST_BUNDLE_UNRESOLVEABLE_LOCATION "";
CelixBundleContextBundlesTests() {
properties = properties_create();
```
##########
File path: .github/workflows/macos-nightly.yml
##########
@@ -0,0 +1,40 @@
+name: Celix MacOS Nightly
+
+on:
+ schedule:
+ - cron: '0 0 * * *'
+
+jobs:
+ build:
+ runs-on: ${{ matrix.os }}
+ strategy:
+ fail-fast: false
+ matrix:
+ os: [macOS-latest]
+ compiler: [clang]
+ timeout-minutes: 120
+ steps:
+ - name: Checkout source code
+ uses: actions/checkout@master
+ - name: Install dependencies
+ run: |
+ brew update
+ brew install lcov zeromq czmq openssl cpputest
+ brew unlink openssl && brew link openssl --force
+ - name: Build
+ env:
+ CC: ${{ matrix.compiler }}
+ BUILD_OPTIONS: |
+ -DENABLE_TESTING=ON
+ -DENABLE_ADDRESS_SANITIZER=ON
+ run: |
+ mkdir build install
+ cd build
+ cmake -DCMAKE_BUILD_TYPE=Release ${BUILD_OPTIONS}
-DCMAKE_INSTALL_PREFIX=../install ..
Review comment:
I prefer the default build to be RelWithDebInfo. Especially if that is
going to be the build type without postfixes for bundles.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
