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

Compile tests only in top-level projects. #197

Merged
merged 3 commits into from
Jul 27, 2018
Merged

Conversation

jdumas
Copy link
Contributor

@jdumas jdumas commented Jul 26, 2018

This is a small change, which basically means that tests won't be compiled when nlopt is used in another project via add_subdirectory(). Please let me know if you have any question.

@jschueller
Copy link
Collaborator

an option to disable building of tests would be more generic

@stevengj
Copy link
Owner

Are the tests causing a problem? I agree in principle that there should be a general mechanism (e.g. -DNLopt_tests=Off) to disable building the tests, but they are so small it's not clear it's worth the trouble.

(I just encountered a problem building the tests with a cross compiler because they weren't being linked by the C++ linker, but that is hopefully fixed by 6e7f31f.)

@jdumas
Copy link
Contributor Author

jdumas commented Jul 26, 2018

I'm building my own code against nlopt, and I don't need to have both set of tests (the ones for nlopt and mine) in the same project, since I'm only interested in my own tests. Also, when a test in nlopt fails it shouldn't impact my own tests (e.g. right now I have an issue with the guile test that is not compiling).

I can add an option if you prefer, and set its default value based on whether nlopt is a top-level project or not.

@jdumas
Copy link
Contributor Author

jdumas commented Jul 26, 2018

Ok done!

CMakeLists.txt Outdated
@@ -31,6 +31,12 @@ option (NLOPT_GUILE "build guile bindings" ON)
option (NLOPT_SWIG "use SWIG to build bindings" ON)
option (NLOPT_LINK_PYTHON "link Python libs" ON)

if (CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR)
option (NLOPT_BUILD_TESTS "build unit tests" ON)
Copy link
Owner

Choose a reason for hiding this comment

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

Probably just NLOPT_TESTS to be shorter

@stevengj stevengj merged commit ae35eec into stevengj:master Jul 27, 2018
@stevengj
Copy link
Owner

LGTM, thanks!

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