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

Set default for BUILD_TESTS to OFF. #3316

Merged
merged 6 commits into from Nov 21, 2022

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Nov 16, 2022

We currently have the CMake option BUILD_TESTS set to ON, but then mlpack_test is not a part of the all target---it specifically has an EXCLUDE_FROM_ALL marking. The reason for this is so that users do not accidentally compile the tests, which take a really long time.

But, it's also reasonable to just set BUILD_TESTS to OFF by default! Then, when BUILD_TESTS is ON, we know the user means to build the tests, and we can make it part of the all target.

I also added a convenience script that displays this error if you try to run make mlpack_test with BUILD_TESTS set to OFF:

$ make mlpack_test
CMake Error at /home/ryan/src/mlpack/CMake/TestError.cmake:3 (message):
  To build the mlpack_test target, reconfigure CMake with the BUILD_TESTS
  option set to ON! (i.e.  `cmake -DBUILD_TESTS=ON ../`)


make[3]: *** [src/mlpack/CMakeFiles/mlpack_test.dir/build.make:70: src/mlpack/CMakeFiles/mlpack_test] Error 1
make[2]: *** [CMakeFiles/Makefile2:507: src/mlpack/CMakeFiles/mlpack_test.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:514: src/mlpack/CMakeFiles/mlpack_test.dir/rule] Error 2
make: *** [Makefile:231: mlpack_test] Error 2

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@conradsnicta conradsnicta merged commit c2e47f8 into mlpack:master Nov 21, 2022
@rcurtin rcurtin mentioned this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants