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

Inconsistencies in how to include doctest.h #670

Open
eli-schwartz opened this issue Jul 10, 2022 · 7 comments
Open

Inconsistencies in how to include doctest.h #670

eli-schwartz opened this issue Jul 10, 2022 · 7 comments
Labels
build-system/cmake Issues regarding the CMake build system category/build-systems Issues related to build systems type/docs Issues related to improving documentation

Comments

@eli-schwartz
Copy link
Contributor

I tried to follow the Tutorial.md with the following CMakeLists.txt:

project(test LANGUAGES CXX)

add_subdirectory(subprojects/doctest)

add_executable(foo foo.cpp)
target_link_libraries(foo doctest::doctest)

Trying to build this failed:

$ ninja -v
[1/2] /usr/bin/c++  -I/tmp/testproj/subprojects/doctest  -MD -MT CMakeFiles/foo.dir/foo.o -MF CMakeFiles/foo.dir/foo.o.d -o CMakeFiles/foo.dir/foo.o -c /tmp/testproj/foo.cpp
FAILED: CMakeFiles/foo.dir/foo.o 
/usr/bin/c++  -I/tmp/testproj/subprojects/doctest  -MD -MT CMakeFiles/foo.dir/foo.o -MF CMakeFiles/foo.dir/foo.o.d -o CMakeFiles/foo.dir/foo.o -c /tmp/testproj/foo.cpp
/tmp/testproj/foo.cpp:2:10: fatal error: doctest.h: No such file or directory
    2 | #include "doctest.h"
      |          ^~~~~~~~~~~
compilation terminated.
ninja: build stopped: subcommand failed.

The following meson.build did work though:

project('testproj', 'cpp')

executable('foo', 'foo.cpp', dependencies: dependency('doctest'))
$ ninja -v
[1/2] ccache c++ -Ifoo.p -I. -I.. -I../subprojects/doctest/doctest -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -O0 -g -MD -MQ foo.p/foo.cpp.o -MF foo.p/foo.cpp.o.d -o foo.p/foo.cpp.o -c ../foo.cpp
[2/2] c++  -o foo foo.p/foo.cpp.o -Wl,--as-needed -Wl,--no-undefined

The obvious distinction would seem to be the include path provided by the doctest target, which seems entirely insufficient in cmake.

@antonysigma
Copy link

Hi, I wonder how we resolve the include path issue? From the tutoral, it is the user's responsibility to track down the (absolute) search path to doctest.h. How can we modify Cmake definition file to automate the job for us?

(From tutorial.md) This tutorial assumes you can use the header directly: #include "doctest.h" - so it is either in the same folder with your test source files or you have set up the include paths to it in your build system properly.

@eli-schwartz
Copy link
Contributor Author

The cmake definition could be trivially modified to change this, e.g. eli-schwartz@3c27fd0

Hopefully someone could confirm whether this is indeed desirable...

@cdeln
Copy link

cdeln commented Aug 18, 2024

This is a bit of a bikeshed wart (that is, ugly but not important).
The include path differs between platforms and projects.
On Ubuntu, doctest is packaged such that you include it as <doctest/doctest.h>, which is also how you would include it if you put it in a subdirectory like this.
However, the #include "doctest.h" version is quite common (and makes sense if you copy the header into your projects source), which is the assumption set out in the tutorial.

I suggest we close this as there is no solution better than the other here.

@cdeln cdeln added the wontfix label Aug 18, 2024
@eli-schwartz
Copy link
Contributor Author

Your answer makes sense only if you assume that doctest should NOT be packaged by distros due to being impossible to use correctly.

It's not a bikeshed wart. You should document your API contract and make sure it works as documented. The current state of the documentation is inconsistent and freely mixes the idea of copying the source into your project vs getting a copy from Ubuntu.

@eli-schwartz
Copy link
Contributor Author

The include path differs between platforms and projects.

Why is this even being held up as a positive thing?

...

In fact, I am not entirely sure why C++ projects, specifically, believe that repeating the same word twice (installing a header to /usr/include/${NAME}/${NAME}.h, that is) provides some benefit over only using the name once. But if you're going to do it, you should have a consistent and documented API, because header include names are part of the API...

@cdeln
Copy link

cdeln commented Aug 19, 2024

Not sure why you think I am holding it up as a positive thing, it's just a fact, and I am quite indifferent to it.

C++ is a bit different from many other languages in the sense that there is no standardized package manager.
Hence, include paths tend to be put on the user to set up. If you put doctest header in one of your public headers then it becomes problematic since that would propagate to your clients.

Nevertheless, every project need some way of specifying doctest as their dependency. Those that do it with CMake by downloading the source code and setting up the include path as done in the tutorial, well, they are responsible for the resulting include path as well. In the tutorial the doctest include folder is appended to the source_dir property of the project. If I would have written that tutorial, I would not have done that, but included doctest as #include <doctest/doctest.h> instead.

If you use doctest distributed by a package manager it should always be #include <doctest/doctest.h> because that is the include path implied by the installation tree if you run cmake --install <build-dir> --prefix <install-dir>. TBH, I don't know why so many CMake people like to not install their sources, even if locally in your project.

If you want to do something you can update the docs to make use the install tree instead and make this more consistent.

@cdeln cdeln added the type/docs Issues related to improving documentation label Aug 19, 2024
@cdeln
Copy link

cdeln commented Aug 20, 2024

Hi @eli-schwartz . My stance in the question is shifting. This thing can only be improved to help people get a more consistent experience. I missed your previous comment also, I'll elaborate a bit.

You should document your API contract and make sure it works as documented. The current state of the documentation is inconsistent and freely mixes the idea of copying the source into your project vs getting a copy from Ubuntu.

I agree, and I think it should be <doctest/doctest.h>

In fact, I am not entirely sure why C++ projects, specifically, believe that repeating the same word twice (installing a header to /usr/include/${NAME}/${NAME}.h, that is) provides some benefit over only using the name once.

It acts as a namespace for the projects, and is necessary for extensions to work.

@cdeln cdeln added build-system/cmake Issues regarding the CMake build system category/build-systems Issues related to build systems and removed wontfix labels Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-system/cmake Issues regarding the CMake build system category/build-systems Issues related to build systems type/docs Issues related to improving documentation
Projects
None yet
Development

No branches or pull requests

3 participants