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

Change the include path of the doctest CMake interface target so users need to specify the folder as well #175

Closed
ncihnegn opened this issue Jan 24, 2019 · 7 comments

Comments

@ncihnegn
Copy link
Contributor

I am just curious about the rationale behind using "doctest.h" instead of "doctest/doctest.h".

@onqtam
Copy link
Member

onqtam commented Jan 24, 2019

well I don't know why the examples in the repository are done this way but the header is inside of a doctest folder itself so one may do it the way you describe if he chooses to. Looking back perhaps it should have been include/doctest/doctest.h but I don't want to introduce such changes - not really important. There is absolutely no consensus as to how to structure C++ code physically. Perhaps for beginners it would be the easiest to just pull the header file and place it next to their only test source file.
The good thing about single-header libraries is that they are so simple and you can integrate them any way you like.

@ncihnegn
Copy link
Contributor Author

That's true. It matters for CMake users, though. I have to change from doctest/doctest.h to doctest.h after I use target_link_library(target doctst::doctest) instead of include_directories().

@onqtam
Copy link
Member

onqtam commented Jan 24, 2019

Well perhaps you could define the doctest::doctest target on your own 1-2 lines without needing the CMake code from doctest.

I would appreciate if others give their opinion on this issue as well before deciding if I should change anything.

@bnason-nf
Copy link
Contributor

I generally prefer external libraries to use includes in the form of <library_name/header.h> since I feel like it hopefully reduces the potential for name collisions. On the other hand, I have no problem if doctest remains with the current structure.

@ncihnegn
Copy link
Contributor Author

I believe <doctest/doctest.h> is the consistent way; otherwise how do you justify putting it into the doctest folder?

@onqtam onqtam changed the title Include path Change the include path of the doctest CMake interface target so users need to specify the folder as well Jan 25, 2019
@onqtam onqtam closed this as completed in ce4523b Jan 25, 2019
@onqtam
Copy link
Member

onqtam commented Jan 25, 2019

changed in release 2.2.2 :)
https://github.com/onqtam/doctest/releases/tag/2.2.2

onqtam pushed a commit that referenced this issue Jan 27, 2019
Fix CMake include path #175
onqtam pushed a commit that referenced this issue Jan 28, 2019
* Change the include path in examples as #175

* Change include path in scripts
onqtam added a commit that referenced this issue Jan 28, 2019
…o wandbox (the online compiler) with the right directory structure - the online service wouldn't allow me to use paths for including headers... relates #175
@drTr0jan
Copy link

That's true. It matters for CMake users, though. I have to change from doctest/doctest.h to doctest.h after I use target_link_library(target doctst::doctest) instead of include_directories().

Hi @ncihnegn. I've tried to build main example from Readme via CMake (CMakeLists.txt) and have got an error.

boris@boris:~/doctest% cmake --build .
[ 50%] Building CXX object CMakeFiles/main.dir/main.cpp.o
/home/boris/doctest/main.cpp:2:10: fatal error: 'doctest.h' file not found
#include "doctest.h"
         ^~~~~~~~~~~
1 error generated.
gmake[2]: *** [CMakeFiles/main.dir/build.make:76: CMakeFiles/main.dir/main.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/main.dir/all] Error 2
gmake: *** [Makefile:91: all] Error 2

I think, it's need to specify doctest directory explicit for INCLUDES DESTINATION at:

INCLUDES DESTINATION "${include_install_dir}"

If I specify INCLUDES DESTINATION as ${include_install_dir}/doctest, I've got a successful build.

@onqtam, what do you think about it?

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