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 NFD_INSTALL option + disable install target for subproject usage #77

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Add NFD_INSTALL option + disable install target for subproject usage #77

merged 1 commit into from
Sep 5, 2022

Conversation

encounter
Copy link
Contributor

This checks whether the project is being included as a subproject, and sets NFD_INSTALL and NFD_BUILD_TESTS accordingly.

Note: this does change the default for NFD_BUILD_TESTS to ON when being built as the root project, this can be reverted if not desired.

@encounter encounter changed the title Add NFD_INSTALL option + disable install target by default Add NFD_INSTALL option + disable install target for subproject usage Aug 29, 2022
@btzy
Copy link
Owner

btzy commented Aug 30, 2022

Thanks for the PR! This seems reasonable. Tests were not built by default since #17, which seems to be using NFD in another CMake project.

Also, I'm wondering if this is the "correct" solution to this problem in CMake - it seems like a fairly common use case. The only resource on the Internet that I could find describing this solution is here. Is this what large CMake projects usually do?

@encounter
Copy link
Contributor Author

encounter commented Sep 5, 2022

As far as I’m aware, this is the proper CMake solution. It’s in all the large CMake projects I’ve seen. (Though I agree it appears slightly janky 🙂)

SDL2 example: https://github.com/libsdl-org/SDL/blob/main/CMakeLists.txt#L8

@btzy
Copy link
Owner

btzy commented Sep 5, 2022

I see. I did a check on the list of popular projects known to use NFDe, and it seems that none of them will be affected by the NFD_BUILD_TESTS change.

@btzy btzy merged commit dee61e5 into btzy:master Sep 5, 2022
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

2 participants