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

Add to readme cmake FetchContent section #293

Merged

Conversation

cobyj33
Copy link
Contributor

@cobyj33 cobyj33 commented Oct 26, 2023

It's imporant to note that FetchContent will automatically build all of the argparse tests and samples when including it with FetchContent, since the root CMakeLists.txt file sets these options as ON by default.

Generally, users of the library wouldn't want tests and samples to be built alongside a library dependency, so we could include these two more lines in the README to show how to disable building the tests and samples by default.

@p-ranav p-ranav merged commit 31fb9e0 into p-ranav:master Oct 26, 2023
8 checks passed
@@ -1133,6 +1133,8 @@ PROJECT(myproject)

# fetch latest argparse
include(FetchContent)
set(ARGPARSE_BUILD_TESTS OFF CACHE INTERNAL "Turn off building argparse tests")
Copy link

Choose a reason for hiding this comment

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

These options should be OFF by default for subprojects (i.e. NOT PROJECT_IS_TOP_LEVEL)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly agree with you. The only thing is that PROJECT_IS_TOP_LEVEL is available in CMake versions 3.21+ and argparse uses CMake minimum version 3.12.4 as of now. There are ways around this of course like checking if PROJECT_NAME is defined (Catch2 uses this tactic) which I plan to use. The PR should be up in around the next hour if everything works out. Also, the install targets shouldn't be set when using as a subproject too, and as of now ARGPARSE_INSTALL defaults to ON.

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.

3 participants