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.

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

phprus
Copy link

@phprus phprus commented Nov 10, 2023

CMake and testability improvements.
I apologize for the large number of changes in single PR.

Purpose of changes:

  1. Support for compiling and running tests if argparse is included via add_subdirectory.
  2. Reusing code inside CMakeLists.txt project files.
  3. Enable compilation of all components in GitHub Actions CI.

@p-ranav, Please review my PR.

Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
@topazus
Copy link

topazus commented Dec 17, 2023

@phprus Maybe it will be good to add and install CMake target file argparseTargets.cmake? I am not familiar about CMake. I see this from other projects, like cxxopts. https://github.com/jarro2783/cxxopts/blob/554396be3b19bff45384a3c6307a1eb9a3497940/cmake/cxxopts.cmake#L118-L129

Comment on lines +35 to +40
# 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()
Copy link

Choose a reason for hiding this comment

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

Isn't just putting /W4 will guarantee it will always replace other lower level warning level?

Comment on lines +42 to +45
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)
Copy link

Choose a reason for hiding this comment

The 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
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)
elseif(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
set(ARGPARSE_PEDANTIC_COMPILE_FLAGS -Wall -Wno-long-long -pedantic -Wsign-conversion -Wshadow -Wconversion -Werror -Wextra)

Comment on lines +74 to +76
working-directory: ${{runner.workspace}}/build
shell: bash
run: cmake -DARGPARSE_BUILD_SAMPLES=ON -DCMAKE_BUILD_TYPE=Debug ${{ matrix.cmake_opts }} $GITHUB_WORKSPACE
Copy link

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:

Suggested change
working-directory: ${{runner.workspace}}/build
shell: bash
run: cmake -DARGPARSE_BUILD_SAMPLES=ON -DCMAKE_BUILD_TYPE=Debug ${{ matrix.cmake_opts }} $GITHUB_WORKSPACE
run: cmake . -B build -DARGPARSE_BUILD_SAMPLES=ON -DCMAKE_BUILD_TYPE=Debug ${{ matrix.cmake_opts }}

Also setting shell: bash is not needed here.

Comment on lines +82 to +83
working-directory: ${{runner.workspace}}/build
run: cmake --build . --config Debug --verbose
Copy link

Choose a reason for hiding this comment

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

No need to set the working-directory, we can just use this:

Suggested change
working-directory: ${{runner.workspace}}/build
run: cmake --build . --config Debug --verbose
run: cmake --build build --config Debug --verbose

Comment on lines +86 to +89
working-directory: ${{runner.workspace}}/build
run: ctest -C Debug -V
env:
CTEST_OUTPUT_ON_FAILURE: True
Copy link

Choose a reason for hiding this comment

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

The same here, we can also use --test-dir instead of setting the working-directory and use --output-on-failure instead of CTEST_OUTPUT_ON_FAILURE:

Suggested change
working-directory: ${{runner.workspace}}/build
run: ctest -C Debug -V
env:
CTEST_OUTPUT_ON_FAILURE: True
run: ctest --test-dir build -C Debug -V --output-on-failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants