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

Fix CMakes #250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix CMakes #250

wants to merge 1 commit into from

Conversation

aljen
Copy link

@aljen aljen commented Sep 18, 2019

Prefix options with RTTR_ and if it's added as a subdirectory, turn everything off (examples, docs, etc.)

Prefix options with RTTR_ and if it's added as a subdirectory, turn everything off (examples, docs, etc.)
option(CUSTOM_DOXYGEN_STYLE "Enable this option to use a custom doxygen style for HTML documentation; Otherwise the default will be used" ON)
option(BUILD_WEBSITE_DOCU "Enable this option to create the special docu for the website" OFF)

if (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

For what is this if and the code below?

Copy link
Author

Choose a reason for hiding this comment

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

This if will get fired only if you open this project CMakeLists.txt as a top, standalone project, but it will not if it's parsed for example from add_subdirectory(vendor/rttr)

NAMESPACE RTTR::
FILE rttr-config.cmake)
if (RTTR_BUILD_INSTALLER)
install(EXPORT rttr_targets
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should go with 4 spaces here for indention

@acki-m
Copy link
Contributor

acki-m commented Sep 26, 2019

RTTR_ and if it's added as a subdirectory, turn everything off (examples, docs, etc.)
What is the reason for this?
So why prefix? And Why disable the other stuff?

@aljen
Copy link
Author

aljen commented Sep 26, 2019

What is the reason for this?
So why prefix? And Why disable the other stuff?

Mainly - if you're building this project alone, as a standalone library - it's working as it was, but if you're just a client and do something like add_subdirectory it from another project like most of the third party integrations are done (mainly on Windows, on *nix there are tons of package managers, from which I can install this library), it will disable all tests, benchmarks, docs, etc.

And why prefix it?

Here is a snippet from root CMakeLists.txt of my homebrew engine:

# set(BUILD_PACKAGE OFF)
# set(BUILD_UNIT_TESTS OFF)
# set(BUILD_STATIC ON)
# set(BUILD_EXAMPLES OFF)
# set(BUILD_DOCUMENTATION OFF)
# set(BUILD_INSTALLER OFF)
# set(BUILD_PACKAGE OFF)
# set(USE_PCH OFF)
# add_subdirectory(vendor/rttr)

add_subdirectory(vendor/glad)
add_subdirectory(vendor/sdl2)

Right now, it will fail to parse my project, because docs are default to ON, examples too, and me as a user of your library don't care for tests, packaging, examples, documentation, installer, etc.

But I need to set those variables before I add_subdirectory your library and those variables will leak to every add_subdirectory on the way, what if some other library used for example USE_PCH or my own (they're not :P)? 😃

So prefixing them with RTTR_ will partially solve this problem, but the real solution is to not enable them in the first place if you detect you're being built as a subproject 😃

@acki-m
Copy link
Contributor

acki-m commented Sep 28, 2019

Okay I got it, that the variable names will leak to other libraries. So this change is okay. However, I don't like this implicit behavior. When you do it in this context then the variables defaults will changes.
And then someone else is coming and still want to have the docs generated, etc...

Please just set the variables default how you like it before calling add_subdirectory(vendor/rttr) and that's it.

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