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

Provide a configure flag to use the system doctest #915

Merged
merged 1 commit into from
Sep 26, 2021

Conversation

bowlofeggs
Copy link
Contributor

This will allow distributions to more easily maintain
incompatibilies between doctest and the rest of their software.

Fixes #912

Signed-off-by: Randy Barlow randy@electronsweatshop.com

*
* This module provides a convient way to import the requested doctest (the
* vendored version, or the system-provided one, as configured at build time)
* without having to remember to use the DOCTEST_HEADER macro in new files.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you find this approach too "wacky". I'd be happy to just find all the imports for doctest.hh and replace them with the DOCTEST_HEADER macro instead if you prefer that approach.

@@ -174,6 +177,7 @@ noinst_HEADERS = \
db_sub_source.hh \
doc_status_source.hh \
doctest.hh \
$(doctest_vendored_h) \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal with this was to avoid building the bundled doctest when we are using the system one. If you know a better way I'm all ears!

This will allow distributions to more easily maintain
incompatibilies between doctest and the rest of their software.

Fixes tstack#912

Signed-off-by: Randy Barlow <randy@electronsweatshop.com>
@tstack
Copy link
Owner

tstack commented Sep 20, 2021

I haven't looked too closely yet, but I wanted to make a minor suggestion. I've recently added a third-party directory. Would it be possible to do this by only adding the third-party/doctest directory to the include paths if the system installed one wasn't available?

@bowlofeggs
Copy link
Contributor Author

I haven't looked too closely yet, but I wanted to make a minor suggestion. I've recently added a third-party directory. Would it be possible to do this by only adding the third-party/doctest directory to the include paths if the system installed one wasn't available?

Yeah I think that should be possible. Ergonomically, I think it might be good to keep a configure flag so that it explicitly fails when the system version was requested and not found (otherwise it could be confusing for distros who may believe they are using the system version and not the bundled one). What do you think?

@tstack
Copy link
Owner

tstack commented Sep 22, 2021

Ergonomically, I think it might be good to keep a configure flag so that it explicitly fails when the system version was requested and not found (otherwise it could be confusing for distros who may believe they are using the system version and not the bundled one).

That sounds fine to me. Would you be able to make the change to move the doctest stuff to a dir in the third-party directory?

@tstack
Copy link
Owner

tstack commented Sep 24, 2021

I'd like to make a v0.10.1 release in the next few days. If you'd like this change to be a part of it, can you let me know if you have time to finish it off.

@bowlofeggs
Copy link
Contributor Author

That sounds fine to me. Would you be able to make the change to move the doctest stuff to a dir in the third-party directory?

Sure, I'd be happy to.

I'd like to make a v0.10.1 release in the next few days. If you'd like this change to be a part of it, can you let me know if you have time to finish it off.

Unfortunately I might not have time to revisit this for a few days, so don't feel like I'll be disappointed if you release without this; it can wait. I can always carry a patch in Gentoo for a time if need be.

@tstack tstack merged commit f4d9e51 into tstack:master Sep 26, 2021
@bowlofeggs bowlofeggs deleted the optional_system_doctest branch September 27, 2021 13:42
@bowlofeggs
Copy link
Contributor Author

Thanks!

@tstack
Copy link
Owner

tstack commented Sep 27, 2021

I also made the change to have it work by adjusting the include path in commit 954e368

@bowlofeggs
Copy link
Contributor Author

I also made the change to have it work by adjusting the include path in commit 954e368

Oh nice, this looks better than what I came up with. Thanks again!

bowlofeggs added a commit to bowlofeggs/gentoo that referenced this pull request Oct 1, 2021
Backport tstack/lnav#915 so we can use
the system doctest while we wait on a new lnav upstream release.

Closes: https://bugs.gentoo.org/809752
Closes: https://bugs.gentoo.org/812353
Signed-off-by: Randy Barlow <randy@electronsweatshop.com>
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Oct 2, 2021
Backport tstack/lnav#915 so we can use
the system doctest while we wait on a new lnav upstream release.

Closes: https://bugs.gentoo.org/809752
Closes: https://bugs.gentoo.org/812353
Signed-off-by: Randy Barlow <randy@electronsweatshop.com>
Closes: #22454
Signed-off-by: Sam James <sam@gentoo.org>
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.

Option to test with unbundled doctest
2 participants