-
Notifications
You must be signed in to change notification settings - Fork 157
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
Adding CMake include guard #389
Conversation
@@ -1,79 +1,82 @@ | |||
# Link against target system libs | |||
# ------------------------------- | |||
|
|||
# Independent target to make it possible to have new dependencies each build | |||
add_library(libdeps INTERFACE) | |||
if(NOT TARGET libdeps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me wonder… if I declare my own target libdeps
(the name seems pretty generic), and then include HighFive, wouldn't this lead to collision / failure, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. This is the reason I have always been favour of having a makeshift namespace. Firstly, one should:
libdeps
->HighFive::dependecies
Secondly, one could allow a finer precision:HighFive::hdf5
HighFive::use_Eigen
HighFive::use_xtensor
HighFive::use_Boost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable to me… also found this:
https://cmake.org/cmake/help/v3.10/command/include_guard.html?highlight=include_guard
unfortunately pretty late, only in 3.10 onwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include_guard
seem nice. Just as good as the if though. Let me switch to HighFive::dependencies
here, and then to the more refined things in separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change is going to take a bit more work than I hoped. Personally I find HighFive's CMake support a bit too involved... Anyway, let's do the bugfix now and improvement separately.
Ping @matz-e @alkino @ferdonline |
Fixes #387