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

Move single header to a separate folder #187

Merged
merged 2 commits into from Mar 2, 2019
Merged

Move single header to a separate folder #187

merged 2 commits into from Mar 2, 2019

Conversation

dimztimz
Copy link
Contributor

@dimztimz dimztimz commented Feb 19, 2019

Description

It is much better for tests and examples to directly depend on the original sources as errors and warnings are properly reported.

Previously, errors are warnings were reported against the single include, but had to be fixed in doctest_fwd.h and doctest.cpp.

The C++ and CMake part are done properly, the python scripts and CI that may need changing to accommodate for this change. They need to push the new single include.

@dimztimz
Copy link
Contributor Author

Now, doctest/doctest.h is very small file including the other two, and the single include is moved to single_include/doctest/doctest.h

@onqtam
Copy link
Member

onqtam commented Feb 19, 2019

@dimztimz Hi! This was actually a long-standing pain-problem for me!

I haven't looked entirely into the PR but on first thought I'm a bit hesitant to accept this.

I'm not sure I want to go the route of catch and have 2 headers with the same name, where one is in a single_include folder which is the actual final thing, and a separate one which includes the parts which is in the default location. It's a breaking change how users consume the library (the single header version will be moved to single_include.

I see that the single header is at least always rebuilt, just like before, but the example targets don't depend on the assemble_single_header target anymore and standard CMake macros/functions are used for adding targets (libraries, executables). That certainly makes things cleaner.

One thought that occurred to me is that the final header might remain where it used to be, but the examples could be using a header (which includes the 2 parts) from a different place just for development purposes - just like there is an "out of sight" special source file part of the examples located in scripts which isn't meant to showcase proper use of the library. That way only an include path has to be special for the examples and they still won't need to depend on the assemble_single_header target. That include path might be provided either by overriding the add_library macro, or just provided globally (even though that isn't "modern" CMake).

I'll have to think about this... some other opinions would be appreciated as well.

Aaand I wrote you an e-mail a few hours ago at d********p@hotmail.com - is that you?

@dimztimz
Copy link
Contributor Author

dimztimz commented Feb 19, 2019

It's a breaking change how users consume the library (the single header version will be moved to single_include.

It is a braking change only for the users who are consuming the single header directly from the source tree from the branch master with wget, curl, or similar. These users should not be doing this in the first place, as they lose their deterministic builds. At least they should be downloading from a tag.

The following users won't be affected:

  1. users who are downloading the whole source tree in some way (e.g. git submodule),
  2. users who are using the installed version with make install,
  3. users who are using it from package manager, or
  4. users who are downloading from the releases page.

One thought that occurred to me is that the final header might remain where it used to be

IMO it is a bad idea to keep generated source code along the real source files.

but the examples could be using a header (which includes the 2 parts) from a different place just for development purposes - just like there is an "out of sight" special source file part of the examples located in scripts which isn't meant to showcase proper use of the library. That way only an include path has to be special for the examples and they still won't need to depend on the assemble_single_header target. That include path might be provided either by overriding the add_library macro, or just provided globally (even though that isn't "modern" CMake).

Seems like overly complicated to me.

With this change we achieve the following:

  1. Noticeably increased maintainability
  2. Unaffected usability, except that small percent of the users will need to change a single line in their code.

It is much better for tests and examples to directly depend on the
original sources as errors and warnings are properly reported.

Previously, errors are warnings were reported against the single include,
but had to be fixed in doctest_fwd.h and doctest.cpp.
@dimztimz
Copy link
Contributor Author

CI finally passed.

@iboB
Copy link
Member

iboB commented Feb 20, 2019

There's another use case, which I personally employ often: Plain old copy doctest.h in my repo and use it from there.

Granted, it's not a huge effort to copy two files instead of one and the increased maintainability is indeed an appealing argument.

I'd personally suggest taking a bit from both:

Change the examples to use the dual file setup and the docs to suggest it. Change the unified file to include both. But! Make a CMake install step which generates a unified file and still deploy the unified file in releases as another downloadable asset besides the zipped sources from the repo.

@dimztimz
Copy link
Contributor Author

@iboB That is already handled. Give a closer look in the folder single_include.

@onqtam
Copy link
Member

onqtam commented Feb 28, 2019

Sorry this is taking so long. I have a bunch of other things to do and am not 100% sold on the idea here.

One thing that occurred to me is that I could use #line directives in the final doctest.h header to point to the actual 2 parts so errors/warnings are properly reported for the right files and lines. These #line directives could be #ifdefed and be in effect only when NOT (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR) (meaning the doctest CMake project is not a subproject but actually the main CMake project for which build files are generated for).

I recognize that this is even more complicated - your solution is certainly more clean and doesn't require the use of custom add_library macros for setting the dependency on the assemble_single_header target. I guess for some reason I really don't like having 2 files named doctest.h in the repo.

I'll think about this a bit more.

@dimztimz
Copy link
Contributor Author

dimztimz commented Feb 28, 2019

Well another solution is to not generate the single header in the source tree. Instead, generate it in the build tree and on make install install the single header. This way you will not have two files doctest.h.

If you ask me this way is the cleanest way, source files go to source dir and everything generated goes to build dir.

@onqtam
Copy link
Member

onqtam commented Mar 2, 2019

I decided to merge this, but I'll change a few things after that:

  • the final single include header will be in the old place - in the doctest folder and not in a single_include folder (even though https://github.com/nlohmann/json does it just like Catch...)
  • when CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR tests/examples will use a doctest.h header located somewhere out-of-sight in the scripts folder which includes the 2 parts so development of doctest is easier (proper reporting of file/line).
  • I'll also remove entirely the ability for the separate examples to be standalone CMake projects - after this PR that isn't possible because they don't know what the doctest target is when linking to it for the include dirs. But I guess it was pointless anyway - I also figured that if I want to iterate over doctest with the playground project and not have to rebuild everything - I could just disable all projects except for playground in the configuration manager of visual studio - so I no longer need the playground project to be self-sufficient in terms of CMake.

this way users won't be wondering which header to use because the "dev" one will be hidden.

@onqtam onqtam merged commit 0dbbd4f into doctest:dev Mar 2, 2019
onqtam added a commit that referenced this pull request Mar 2, 2019
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

Successfully merging this pull request may close these issues.

None yet

3 participants