doctest / doctest

The fastest feature-rich C++11/14/17/20/23 single-header testing framework
https://bit.ly/doctest-docs
MIT License
5.89k stars 643 forks source link

CMake config deletes doctest.h on clean #210

Open seanmiddleditch opened 5 years ago

seanmiddleditch commented 5 years ago

When using doctest as a sub-project in CMake (e.g. via add_subdirectory), and using the Ninja backend (haven't tested others), the clean target in the parent project will result in doctest/doctest.h being deleted.

This is because that file is a target of a custom command, but it is not required as a dependency (because the doctest target for some reason doesn't include doctest.h as a header-only source file).

The practical results are that cleaning or rebuilding in a CMake-based IDE (including modern versions of Visual Studio via the CMake support, which uses Ninja by default) will result in the main doctest.h header being deleted and never recreated.

I'm observing this currently in 2.2, but a perusal of the 2.3 CMakeLists.txt leads me to believe it's still broken there.

I'd recommend checking out the solution at https://gitlab.kitware.com/cmake/cmake/issues/17366 as a potential solution. (It's something I'll be doing in my wrappers that depend on doctest until it is fixed in your upstream, anyway.)

Steps to reproduce

  1. Checkout a project like https://github.com/seanmiddleditch/formatxx (including sub-modules, which pulls in doctest)
  2. Generate a CMake build for the project
  3. Run cmake --build ${build_dir} --target clean in the project
  4. Observe that the sub-module's doctest/doctest.h is deleted
  5. Run cmake --build ${build_dir}
  6. Observe that the build fails because the doctest.h header is now missing but no dependency causes it to be recreated

Extra information

seanmiddleditch commented 5 years ago

(didn't mean to close)

This is the fragment I'm putting in my local formatxx branch and other projects, for reference:

    add_custom_target(formatxx_gen_doctest_h DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/doctest/doctest/doctest.h)
    add_dependencies(doctest formatxx_gen_doctest_h)

Which is clearly hacky, since I shouldn't be reaching into a third-party source like that.

I think the better fix, should you be able to find one, would be to just not set this header as a generated output in the source directory, as even with that fix we have a file being modified in the source directory which is bad juju.

An option might be to make sure you generate the file in the build directory, and just copy that output to the source directory for your maintenance. That way the clean command only wipes out the copy in the build directory which only really affects anything for users who are directly building doctest itself.

Another option is to hide that generation command when doctest is built as a sub-directory. There's a few ways to do that; I'm not sure which is the "best". One option is to check that CMAKE_CURRENT_SOURCE_DIR is equal to CMAKE_SOURCE_DIR. Another is to check via a fragment like the following:

get_directory_property(parent_dir PARENT_DIRECTORY)
set(is_root ON)
if(parent_dir)
    set(is_root OFF)
endif()

Another non-magic option is to just include a way to disable that generation step.

I'm seeing now that I can disable DOCTEST_WITH_MAIN_IN_STATIC_LIB and the generation step is skipped, as an example. That works for some smaller projects of mine (e.g. formatxx) but not for larger ones where I do rely on the static library. Having an independent flag to control that might be "good enough" for my needs, though I might recommend keeping that root detection logic above and setting the default for that flag to OFF if doctest isn't the root project (just to minimize surprises).

onqtam commented 5 years ago

thanks for detecting this! I'll push a fix soon and release 2.3.1

jenisys commented 5 years ago

Problem seems still to exist with current HEAD of git repository (doctest 2.3.3 approx). I mean that "doctest.h" header file is deleted with cmake --build . --target clean.

NOTE: Currently, the header file is automatically recreated during the next build (which makes it less observable / annoying).

onqtam commented 5 years ago

@jenisys thanks for reporting - seems that this needs to be revisited.

cdeln commented 2 months ago

I don't think this is a bug. The doctest header is generated and cleaning the build directory should remove it. Or am I missing something?

cdeln commented 2 months ago

Ah now I understand, the header target is in the source tree. That is a bit of a wart indeed. Or is it a feature? ;)

cdeln commented 2 months ago

I suggest we close since this actually does not affect downstream consumers. If you use doctest as a sub-module and don't tell cmake about the dependency on the generated header, and then clean your project, that can be considered expected to be broken. The crux here is that there is a checked in generated header, without it one naturally has to setup a dependency to build doctest prior to the parent project.