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

Correct DOCTEST_NO_INSTALL logic; do install unless it is set #99

Merged
merged 1 commit into from Nov 4, 2017

Conversation

OdyX
Copy link

@OdyX OdyX commented Nov 4, 2017

Description

Commit b63bf85 (fixing #96) introduces the DOCTEST_NO_INSTALL variable, but it's use is reversed: by default it is set to OFF (meaning you do want to install), but the test says "If set to ON, do install".

This made me upload a doctest.h-less doctest-dev package to Debian.

Debian bug

This was detected through a failed build of argagg and reported as https://bugs.debian.org/880721 .

@nm17
Copy link
Contributor

nm17 commented Nov 4, 2017

Such a stupid bug I made >.<

@onqtam
Copy link
Member

onqtam commented Nov 4, 2017

Also my bad for insta-merge of that :)

this will get merged now in master as well without releasing version 1.2.7 - is that ok?

Also many thanks for maintaining a debian package!

@onqtam onqtam merged commit e0849cb into doctest:dev Nov 4, 2017
@OdyX
Copy link
Author

OdyX commented Nov 5, 2017

No problem for not releasing 1.2.7; it's patched in Debian already. Thanks for the quick merge!

@a4z
Copy link
Contributor

a4z commented Dec 20, 2017

just also got hit with this, when will 1.2.7 be available?

btw, I do not understand the original issue, adding a no install option. Just do not call make install
I think the whole thing should be removed, useless, just makes the cmake file more complex, and error prone, as proven with this issue

@onqtam
Copy link
Member

onqtam commented Dec 20, 2017

The fix is currently in master - 1 commit ahead of the 1.2.6 tag. And perhaps a no install option for doctest is necessary when it's used within a bigger cmake project and only other things need to be installed.

about 1.2.7 - perhaps around christmas :)

@a4z
Copy link
Contributor

a4z commented Dec 30, 2017

thanks for your explanation @onqtam
for this case I would suggest thinking about something like

if( ${CMAKE_PROJECT_NAME} STREQUAL ${PROJECT_NAME})
   # have the default install to true, other wise false
endif( ${CMAKE_PROJECT_NAME} STREQUAL ${PROJECT_NAME})

so you can detect if you are included or not and set the proper default per default

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

4 participants