-
Notifications
You must be signed in to change notification settings - Fork 22
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
Catch version selector #65
Conversation
It needs to update the wiki page too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the overall project so not sure if the macro calls are sensible. But I did spot one suspicious line!
modules/LookUpCatch.cmake
Outdated
file(MAKE_DIRECTORY "${EXTERNAL_ROOT}/include") | ||
file(DOWNLOAD ${Catch_URL} "${Catch_FILE}") | ||
|
||
file(READ "${Catch_FILE}" CATCHSTRING LIMIT 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a comment explaining what this next section is doing (checking download worked by ensuring the file is long enough).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... that was on the original file. I imagine is checking whether the file is long enough as I suppose redirects or errors will be shorter ones.
scripts/AddCatchTest.cmake
Outdated
find_package(Catch REQUIRED) | ||
if(NOT Catch_FOUND AND Catch_WANTED_VERSION) | ||
lookup_package(Catch REQUIRED ARGUMENTS VERSION ${Catch_WANTED_VERSION}) | ||
else(NOT Catch_FOUND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be elseif
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right... though I don't understand what the expression on else
is for. Maybe just to know in which if
block you are in.
Updated comments by @jonc125 - Thanks!! |
as said above. Problem was found at astro-informatics/purify#190 as in astro-informatics/purify#183 was failing after an update on Catch2.