-
Notifications
You must be signed in to change notification settings - Fork 13
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
Build testing #45
Build testing #45
Conversation
Left over changes from 80b13e5
Includes, so |
CMakeLists.txt
Outdated
@@ -33,7 +33,7 @@ endif() | |||
include(GNUInstallDirs) | |||
|
|||
add_subdirectory(lib) | |||
if(NOT BUILD_EXPORTED_TARGETS_ONLY) | |||
if(NOT BUILD_EXPORTED_TARGETS_ONLY AND BUILD_TESTING) |
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 only works if you previously used include(CTest)
, further we should deprecate BUILD_EXPORTED_TARGETS_ONLY
in favor of BUILD_TESTING
or keep the old option and document it properly.
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.
Empirically, 9f812ae seemed to work correctly with cmake 3.18.4 as BUILD_TESTING was not defined.
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.
When reviewing fortran-lang/stdlib#629 not including CTest
did lead to problems, which were related to caching and restarting builds.
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.
I think we should keep the existing option rather than add another with similar meaning.
Closes #44
Closes #42
Closes #41