celix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [celix] pnoltes commented on a change in pull request #228: Feature/support multiple build types
Date Sat, 09 May 2020 14:36:44 GMT

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:
users@infra.apache.org



Mime
View raw message