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

build system: verify package downloads using sha256 checksum #1597

Merged
merged 2 commits into from May 16, 2017
Merged

build system: verify package downloads using sha256 checksum #1597

merged 2 commits into from May 16, 2017

Conversation

MilhouseVH
Copy link
Contributor

As discussed with @vpeter4 on #irc.

This PR adds optional checksum verification for downloaded packages.

Package verification is entirely optional, and the behaviour of scripts/get is unchanged when a package does not specify a value for PKG_SHA256.

The value for PKG_SHA256 is the value that is written to *.sha256 once a file is successfully downloaded.

When bumping packages, one option to obtain the new checksum would be to comment out PKG_SHA256 so that the new tarball is downloaded (and not verified), and then the new checksum value from *.sha256 is added to the bumped package.mk.

scripts/get Outdated
@@ -48,25 +58,37 @@ if [ -n "$PKG_URL" -a -n "$PKG_SOURCE_NAME" ]; then
sleep 1
done

if ! [ -f $SOURCES/$1/$PKG_SOURCE_NAME -a "$(cat $STAMP 2>/dev/null)" == "$PKG_URL" ]; then
rm -f $SOURCES/$1/$PKG_SOURCE_NAME $STAMP
if ! _get_file_already_downloaded $1; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this if statement is unnecessary?
It was already unnecessary in previous version too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's required.

If there are two (or more) processes trying to download the same file at the same time (ie. concurrent builds out of the same repo, which is something I do quite a lot) then only one process will acquire the flock in line 56 and then proceed to download the file, while all the other processes will be blocked, spinning slowly in the while !flock...done loop.

Once the file is downloaded the process with the flock will exit the get script and automatically release the flock, whereupon one of the other other processes will acquire the flock and exit from the while...done loop.

This unblocked process will then execute the if condition in line 61 and - assuming the file has been downloaded successfully by the first process - avoid doing any download processing at all, dropping out of the script and releasing the flock. If the first process did not successfully download the package, then the second process will repeat the same download.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed that.
Wouldn't be code cleaner if _get_file_already_downloaded would be called again after flock loop and after this line content of if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed one more update which handles _get_file_already_downloaded() the same way in the two places it is called - if it returns true, the script exits immediately. This has also eliminated one level of if indentation in subsequent lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per irc discussion, removed one more level of indentation. I've also updated the header as this file is now a complete rewrite. :)

@vpeter4
Copy link
Contributor

vpeter4 commented May 16, 2017

Is there any objection for this functionality? If not we could merge it in master and starts adding checksums to packages.

@lrusak
Copy link
Member

lrusak commented May 16, 2017

looks fine to me

@chewitt chewitt merged commit 099fdf2 into LibreELEC:master May 16, 2017
@chewitt
Copy link
Member

chewitt commented May 16, 2017

thanks @vpeter4

@vpeter4
Copy link
Contributor

vpeter4 commented May 16, 2017

Let's start adding sums then. Only ~700.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants