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

Catch version selector #65

Merged
merged 3 commits into from
Sep 11, 2018
Merged

Catch version selector #65

merged 3 commits into from
Sep 11, 2018

Conversation

dpshelio
Copy link
Member

as said above. Problem was found at astro-informatics/purify#190 as in astro-informatics/purify#183 was failing after an update on Catch2.

@dpshelio
Copy link
Member Author

It needs to update the wiki page too.

Copy link
Member

@jonc125 jonc125 left a 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!

file(MAKE_DIRECTORY "${EXTERNAL_ROOT}/include")
file(DOWNLOAD ${Catch_URL} "${Catch_FILE}")

file(READ "${Catch_FILE}" CATCHSTRING LIMIT 1000)
Copy link
Member

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).

Copy link
Member Author

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.

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)
Copy link
Member

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?

Copy link
Member Author

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.

@dpshelio
Copy link
Member Author

Updated comments by @jonc125 - Thanks!!

@dpshelio dpshelio merged commit ca83198 into UCL:master Sep 11, 2018
@dpshelio dpshelio deleted the catchVersion branch September 11, 2018 15:45
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.

2 participants