-
Notifications
You must be signed in to change notification settings - Fork 256
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.
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
CMake and testability improvements #308
base: master
Are you sure you want to change the base?
Changes from all commits
65f5c8d
b627571
4dfa5cc
7c97651
c5d76b7
0851021
d29ee96
1c57e16
b9a08e1
869f054
b1aee23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,7 +1,7 @@ | ||||||||||||
|
||||||||||||
name: CI | ||||||||||||
|
||||||||||||
on: pull_request | ||||||||||||
on: [push, pull_request] | ||||||||||||
|
||||||||||||
jobs: | ||||||||||||
|
||||||||||||
|
@@ -12,6 +12,8 @@ jobs: | |||||||||||
|
||||||||||||
strategy: | ||||||||||||
|
||||||||||||
fail-fast: false | ||||||||||||
|
||||||||||||
matrix: | ||||||||||||
|
||||||||||||
toolchain: | ||||||||||||
|
@@ -63,25 +65,25 @@ jobs: | |||||||||||
steps: | ||||||||||||
|
||||||||||||
- name: Checkout Code | ||||||||||||
uses: actions/checkout@v2 | ||||||||||||
uses: actions/checkout@v3 | ||||||||||||
|
||||||||||||
- name: Create Build Environment | ||||||||||||
run: cmake -E make_directory ${{runner.workspace}}/build | ||||||||||||
|
||||||||||||
- name: Configure | ||||||||||||
working-directory: test | ||||||||||||
run: cmake -S . -B build ${{ matrix.cmake_opts }} | ||||||||||||
working-directory: ${{runner.workspace}}/build | ||||||||||||
shell: bash | ||||||||||||
run: cmake -DARGPARSE_BUILD_SAMPLES=ON -DCMAKE_BUILD_TYPE=Debug ${{ matrix.cmake_opts }} $GITHUB_WORKSPACE | ||||||||||||
env: | ||||||||||||
CC: ${{ matrix.c_compiler }} | ||||||||||||
CXX: ${{ matrix.cxx_compiler }} | ||||||||||||
|
||||||||||||
- name: Build for ${{ matrix.os }} with ${{ matrix.compiler }} | ||||||||||||
working-directory: test | ||||||||||||
run: cmake --build build | ||||||||||||
working-directory: ${{runner.workspace}}/build | ||||||||||||
run: cmake --build . --config Debug --verbose | ||||||||||||
Comment on lines
+82
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to set the
Suggested change
|
||||||||||||
|
||||||||||||
- name: Test | ||||||||||||
if: ${{ ! startsWith(matrix.os, 'windows') }} | ||||||||||||
working-directory: test/build | ||||||||||||
run: ./tests | ||||||||||||
|
||||||||||||
- name: Test (Windows) | ||||||||||||
if: ${{ startsWith(matrix.os, 'windows') }} | ||||||||||||
working-directory: test/build | ||||||||||||
run: ./Debug/tests.exe | ||||||||||||
working-directory: ${{runner.workspace}}/build | ||||||||||||
run: ctest -C Debug -V | ||||||||||||
env: | ||||||||||||
CTEST_OUTPUT_ON_FAILURE: True | ||||||||||||
Comment on lines
+86
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same here, we can also use
Suggested change
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,8 +13,8 @@ project(argparse | |||||||||||||
LANGUAGES CXX | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
option(ARGPARSE_INSTALL "Include an install target" ON) | ||||||||||||||
option(ARGPARSE_BUILD_TESTS "Build tests" ON) | ||||||||||||||
option(ARGPARSE_INSTALL "Include an install target" ${ARGPARSE_IS_TOP_LEVEL}) | ||||||||||||||
option(ARGPARSE_BUILD_TESTS "Build tests" ${ARGPARSE_IS_TOP_LEVEL}) | ||||||||||||||
option(ARGPARSE_BUILD_SAMPLES "Build samples" OFF) | ||||||||||||||
|
||||||||||||||
include(GNUInstallDirs) | ||||||||||||||
|
@@ -28,15 +28,41 @@ target_include_directories(argparse INTERFACE | |||||||||||||
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> | ||||||||||||||
$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
# Pedantic compiler options | ||||||||||||||
# Update if necessary | ||||||||||||||
if(MSVC) | ||||||||||||||
# Force to always compile with W4 | ||||||||||||||
if(CMAKE_CXX_FLAGS MATCHES "/W[0-4]") | ||||||||||||||
string(REGEX REPLACE "/W[0-4]" "/W4" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}") | ||||||||||||||
else() | ||||||||||||||
set(ARGPARSE_PEDANTIC_COMPILE_FLAGS "/W4") | ||||||||||||||
endif() | ||||||||||||||
Comment on lines
+35
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't just putting |
||||||||||||||
list(APPEND ARGPARSE_PEDANTIC_COMPILE_FLAGS "/WX") | ||||||||||||||
elseif(CMAKE_CXX_COMPILER_ID MATCHES "GNU") | ||||||||||||||
set(ARGPARSE_PEDANTIC_COMPILE_FLAGS -Wall -Wno-long-long -pedantic -Wsign-conversion -Wshadow -Wconversion -Werror -Wextra) | ||||||||||||||
elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") | ||||||||||||||
set(ARGPARSE_PEDANTIC_COMPILE_FLAGS -Wall -Wno-long-long -pedantic -Wsign-conversion -Wshadow -Wconversion -Werror -Wextra) | ||||||||||||||
Comment on lines
+42
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any differences in here, why don't we use this instead:
Suggested change
|
||||||||||||||
endif() | ||||||||||||||
|
||||||||||||||
if(NOT CMAKE_BUILD_TYPE) | ||||||||||||||
set(CMAKE_BUILD_TYPE Release) | ||||||||||||||
endif() | ||||||||||||||
|
||||||||||||||
if(NOT CMAKE_CXX_STANDARD) | ||||||||||||||
set(CMAKE_CXX_STANDARD 17) | ||||||||||||||
endif() | ||||||||||||||
|
||||||||||||||
if(ARGPARSE_BUILD_SAMPLES) | ||||||||||||||
add_subdirectory(samples) | ||||||||||||||
endif() | ||||||||||||||
|
||||||||||||||
if(ARGPARSE_BUILD_TESTS AND ARGPARSE_IS_TOP_LEVEL) | ||||||||||||||
|
||||||||||||||
if(ARGPARSE_BUILD_TESTS) | ||||||||||||||
enable_testing() | ||||||||||||||
add_subdirectory(test) | ||||||||||||||
endif() | ||||||||||||||
if(ARGPARSE_INSTALL AND ARGPARSE_IS_TOP_LEVEL) | ||||||||||||||
|
||||||||||||||
if(ARGPARSE_INSTALL) | ||||||||||||||
install(TARGETS argparse EXPORT argparseConfig) | ||||||||||||||
install(EXPORT argparseConfig | ||||||||||||||
NAMESPACE argparse:: | ||||||||||||||
|
@@ -95,6 +121,6 @@ if(ARGPARSE_INSTALL AND ARGPARSE_IS_TOP_LEVEL) | |||||||||||||
install(FILES "${PKG_CONFIG_FILE_NAME}" | ||||||||||||||
DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig" | ||||||||||||||
) | ||||||||||||||
endif() | ||||||||||||||
|
||||||||||||||
include(CPack) | ||||||||||||||
include(CPack) | ||||||||||||||
endif() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
#include <doctest.hpp> | ||
#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN | ||
#include "doctest.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to leave it like the previous implementation, no need to add another step for creating a directory:
Also setting
shell: bash
is not needed here.