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

Set argparse to not run tests or set up install if not top level #298

Merged

Conversation

cobyj33
Copy link
Contributor

@cobyj33 cobyj33 commented Nov 1, 2023

This pull request comes as a way to make sure users including argparse through FetchContent or through a direct add_subdirectory() call won't have argparse tests built or the argparse install commands configured by default.

Concerning not using the cmake provided PROJECT_IS_TOP_LEVEL variable, PROJECT_IS_TOP_LEVEL is not used because it has a required minimum cmake version of 3.21 while argparse uses a minimum cmake version of 3.12.4.

This approach is largely based on the way Catch2 handles this same condition (Lines 5-9) in their CMakeLists.txt, as well as this issue which references to glitchy installs when setting install targets in a subdirectory CMake structure.

While the user will have to use FetchContent after their project call, this is also how the official PROJECT_IS_TOP_LEVEL handles detecting subdirectory projects, so I don't see this as much of a concern.

Also, I removed the manual setting of ARGPARSE_BUILD_TESTS and ARGPARSE_BUILD_SAMPLES from the FetchContent section of the README, as they are not necessary as ARGPARSE_BUILD_SAMPLES defaults to OFF and test building will now not be run if argparse is not the top level project.

ARGPARSE_BUILD_SAMPLES was also officially declared as an option, as well as initialized to OFF to maintain previous behavior.

@p-ranav p-ranav merged commit ac4c578 into p-ranav:master Nov 1, 2023
8 checks passed
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.

2 participants