Add mirror support #83
Conversation
… the HASH errors to match the CMake formatting (easier to read)
|
Hi @Ninetainedo, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
| " '${FILE_HASH}' <> '${vcpkg_download_distfile_SHA512}'\n" | ||
| "Please delete the file and try again if this file should be downloaded again.") | ||
| "\nFile does not have expected hash:\n" | ||
| " File path: [${downloaded_file_path}]\n" |
There was a problem hiding this comment.
Could you leave spaces between the '[' and the hash? This enables double clicking to select in the default windows terminal.
There was a problem hiding this comment.
Yes, no problem for that. I didn't put it because I tried to stick to the exact syntax of CMake but I have to admit that it's easier with those spaces.
| @@ -1,20 +1,57 @@ | |||
| # Usage: vcpkg_download_distfile(<VAR> URL <http://...> FILENAME <output.zip> SHA512 <5981de...>) | |||
| # Usage: vcpkg_download_distfile(<VAR> URL <http://...> FILENAME <output.zip> SHA512 <5981de...> MIRRORS <http://mirror1> <http://mirror2>) | |||
There was a problem hiding this comment.
I think there's no benefit to separating the URL argument from the MIRRORS argument -- if we rename MIRRORS to URLS, in the future we could completely eliminate the URL argument (just need to move all the portfiles and the template over to using URLS).
There was a problem hiding this comment.
You are entirely right, I'll change it and update all portfiles accordingly.
| file(DOWNLOAD ${url} ${downloaded_file_path} STATUS download_status) | ||
| list(GET download_status 0 status_code) | ||
| if (NOT "${status_code}" STREQUAL "0") | ||
| message(STATUS "Downloading ${url}... Failed") |
There was a problem hiding this comment.
On failure, we should display the status code to the user for debugging.
| message(FATAL_ERROR | ||
| "\n" | ||
| " Failed to download file.\n" | ||
| " Add mirrors or submit an issue at https://github.com/Microsoft/vcpkg/issues/new\n") |
There was a problem hiding this comment.
Link to issues directly; it's possible there will be an existing issue.
| file(SHA512 ${downloaded_file_path} FILE_HASH) | ||
| if(NOT "${FILE_HASH}" STREQUAL "${vcpkg_download_distfile_SHA512}") | ||
| message(FATAL_ERROR | ||
| "\nFile does not have expected hash:\n" |
There was a problem hiding this comment.
I'd really prefer to avoid duplicating the bulk of this user output code. Perhaps an inner function that could be called in both places would be appropriate here?
There was a problem hiding this comment.
Done as well. I used a parameter named FILE_KIND which is "cached file" or "downloaded file" to have more precise logs. (I couldn't find a better name for the variable)
|
Overall this looks great. I think performing the hash check and throwing a hard failure on mismatch is the right way to go (as you've done). The one important comment I have is the |
| file(DOWNLOAD ${url} ${downloaded_file_path} STATUS download_status) | ||
| list(GET download_status 0 status_code) | ||
| if (NOT "${status_code}" STREQUAL "0") | ||
| message(STATUS "Downloading ${url}... Failed. Status: ${download_status}") |
There was a problem hiding this comment.
I print the whole download_status variable because it contains both the error code and a human-friendly message
|
Solid work, thank you! |
Add gtest, to finally devendor it form the Mixxx folder 2.5
Fixes #58
Updated
vcpkg_download_distfile.cmaketo handle MIRRORS url.