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

Allow setting JSON_BuildTests=OFF from parent CMakeLists.txt #846

Closed
FDUdannychen opened this issue Nov 29, 2017 · 9 comments
Closed

Allow setting JSON_BuildTests=OFF from parent CMakeLists.txt #846

FDUdannychen opened this issue Nov 29, 2017 · 9 comments
Assignees
Milestone

Comments

@FDUdannychen
Copy link

I think a standard BUILD_TESTING option is better in this case?

Meanwhile, setting JSON_BuildTests=ON prevents the user from disabling the unit test build from parent CMakeLists.txt.

@nlohmann
Copy link
Owner

What do you propose? Replacing JSON_BuildTests with BUILD_TESTING?

@FDUdannychen
Copy link
Author

I want to disable building tests in my project's CMakeLists.txt. I have a thirdparty/CMakeLists.txt:

-myproj
    -src
        -CMakeLists.txt (build myproj sources)
    -thirdparty
        -json
        -another_lib
        -CMakeLists.txt (I want to disable json's unit tests here)

@nlohmann
Copy link
Owner

OK. I'm not a CMake expert. Could you describe what needs to be done or open a PR?

@FDUdannychen
Copy link
Author

@nlohmann Sorry I just notice you are not using CTest. So BUILD_TESTING is not an option here. I'll pass -DBuildTests=OFF in a script instead of specify it in my CMakeLists.txt as a workaround. This issue should be closed.

@nlohmann
Copy link
Owner

nlohmann commented Dec 4, 2017

In fact we are using ctest...

@theodelrieu
Copy link
Contributor

I've seen quite a few cmake projects using BUILD_TESTS as an option. I thought it was an official option, apparently not.

IIUC, we're not including the CTest module, so the BUILD_TESTING option is missing?

@nlohmann nlohmann added the state: help needed the issue needs help to proceed label Dec 6, 2017
@nlohmann
Copy link
Owner

Any ideas on this?

@TinyTinni
Copy link
Contributor

I am not really sure, what the original issue is.
You can set JSON_BuildTests=OFF in your CMake script before including the json CMakeLIsts with e.g. add_subdirectory (don't forget declaring it as a CACHE variable, or using option instead of set).

IIUC, we're not including the CTest module, so the BUILD_TESTING option is missing?

Yes, regarding to the CTest doc, BUILD_TESTING is included when you include the CTest module via cmake (not done by the project atm).

The BUILD_TESTING seems to be a variable where you can control all tests of all projects included by cmake.

I would propose:

  1. include CTest
  2. add test targets only, when BUILD_TESTING and JSON_BuildTests are ON

This approach allows disabling all tests (of all parent/subprojects) with BUILD_TESTING or disabling just the json tests.

The problem would be in the existence of a single inconsistency:
When BUILD_TESTING is OFF, but JSON_BuildTests is ON.
Some people might think, the json tests will be build.

The default value of BUILD_TESTING is ON, so the approach described above is backward compatible.

ATM, I see no good approach that keeps backward compatibility and/or consistency of the 2 variables, when they get replaced and/or depends on each other.

Corresponding pull request: #885

@nlohmann
Copy link
Owner

Closed by merging #885.

@nlohmann nlohmann removed the state: help needed the issue needs help to proceed label Dec 23, 2017
@nlohmann nlohmann self-assigned this Dec 23, 2017
@nlohmann nlohmann added this to the Release 3.0.1 milestone Dec 23, 2017
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

No branches or pull requests

4 participants