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

Add md5 validation for downloaded libraries #2087

Merged
merged 8 commits into from Nov 25, 2019

Conversation

@tejasvi
Copy link
Contributor

tejasvi commented Nov 16, 2019

Validation is done by a macro which takes .md5 file URL and the file
directory. On hash failure the downloaded files are deleted. Currently
used for stb library only.

Validation is done by a macro which takes .md5 file URL and the file
directory. On hash failure the downloaded files are deleted. Currently
used for stb library only.
@mlpack-bot

This comment has been minimized.

Copy link

mlpack-bot bot commented Nov 16, 2019

Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:

  • All code should follow the style guide
  • Documentation added for any new functionality
  • Tests added for any new functionality
  • Tests that are added follow the testing guide
  • Headers and license information added to the top of any new code files
  • HISTORY.md updated if the changes are big or user-facing
  • All CI checks should be passing

Thank you again for your contributions! 👍

@@ -3,6 +3,17 @@ project(mlpack C CXX)

include(CMake/cotire.cmake)

# Validate md5 hash give md5file url and the file directory.
# If hash check fails set HASH_CHECK_FAIL to non-zero and remove the files.
macro (check_hash MD5_URL DIR HASH_CHECK_FAIL)

This comment has been minimized.

Copy link
@zoq

zoq Nov 16, 2019

Member

So for this to work you need the md5 url right?

This comment has been minimized.

Copy link
@rcurtin

rcurtin Nov 16, 2019

Member

Right, later on http://www.mlpack.org/files/stb/hash.md5 is used as the MD5_URL, so I got that set up on mlpack.org now. 👍

Copy link
Member

rcurtin left a comment

Hey @tejasvi, thanks for taking the time to work on this! I think you are definitely approaching the problem in the right way. I just left a few comments. If you can handle those I think it's ready for merge. Also, do you want to add an entry to HISTORY.md pointing out the change? :)

Thanks! 👍

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
tejasvi added 3 commits Nov 17, 2019
Macro moved to CMake/CheckHash.cmake
A warning for hash failure also added.
CMake implementation of md5sum not supports --check. Instead parse the
.md5 and verify each file manually.
Support for md5 validation of downloaded libraries added.
Copy link
Member

rcurtin left a comment

Hey @tejasvi, thanks for handling the comments that we left. I only have a couple minor comments having to do with error reporting, and then from my end I think it is ready.

CMake/CheckHash.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
tejasvi added 3 commits Nov 18, 2019
Avoid confusing 'Could not download' message on hash failure.
Remove lingering empty files on failed download for successful cmake
next time.
@tejasvi

This comment has been minimized.

Copy link
Contributor Author

tejasvi commented Nov 19, 2019

I'm not sure why Travis is failing. Seems unrelated to the changes.

@zoq

This comment has been minimized.

Copy link
Member

zoq commented Nov 19, 2019

Right, the Travis build timed out.

Copy link
Member

rcurtin left a comment

Awesome, this all looks good to me. Thank you for the contribution! It will be nice to have this support merged in. 👍

@zoq
zoq approved these changes Nov 19, 2019
Copy link
Member

zoq left a comment

Thanks, this looks good to me as well.

@mlpack-bot

This comment has been minimized.

Copy link

mlpack-bot bot commented Nov 19, 2019

Hello there! Thanks for your contribution. I see that this is your first contribution to mlpack. If you'd like to add your name to the list of contributors in src/mlpack/core.hpp and COPYRIGHT.txt and you haven't already, please feel free to push a change to this PR---or, if it gets merged before you can, feel free to open another PR.

In addition, if you'd like some stickers to put on your laptop, I'd be happy to help get them in the mail for you. Just send an email with your physical mailing address to stickers@mlpack.org, and then one of the mlpack maintainers will put some stickers in an envelope for you. It may take a few weeks to get them, depending on your location. 👍

@rcurtin rcurtin added this to the mlpack 3.2.2 milestone Nov 21, 2019
@rcurtin rcurtin merged commit deae272 into mlpack:master Nov 25, 2019
13 of 17 checks passed
13 of 17 checks passed
mlpack.mlpack Build #20191120.1 failed
Details
mlpack.mlpack (Linux Markdown) Linux Markdown failed
Details
mlpack.mlpack (Linux Python27) Linux Python27 failed
Details
mlpack.mlpack (Linux Python37) Linux Python37 failed
Details
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mlpack.mlpack (Linux Plain) Linux Plain succeeded
Details
mlpack.mlpack (Windows VS14 Plain) Windows VS14 Plain succeeded
Details
mlpack.mlpack (Windows VS15 Plain) Windows VS15 Plain succeeded
Details
mlpack.mlpack (Windows VS16 Plain) Windows VS16 Plain succeeded
Details
mlpack.mlpack (macOS Plain) macOS Plain succeeded
Details
mlpack.mlpack (macOS Python27) macOS Python27 succeeded
Details
mlpack.mlpack (macOS Python37) macOS Python37 succeeded
Details
@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Nov 25, 2019

Thanks for the contribution! I think this is great support to have. 👍

@tejasvi tejasvi deleted the tejasvi:md5check branch Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.