-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Update dependencies, standardize on retrieval via tag where available #4353
Conversation
…ble. Signed-off-by: Michael Payne <michael@sooper.org>
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.
This is really nice now. Do you think we should have the sha256
verification as well https://docs.bazel.build/versions/master/be/workspace.html#git_repository?
While it seems like we're going back to raw hashes, I think by having tags provide the machine validated & human readable version, and a proper SHA256 hash for the security, we have best of both worlds.
bazel/repository_locations.bzl
Outdated
@@ -14,42 +14,40 @@ REPOSITORY_LOCATIONS = dict( | |||
strip_prefix = "thrift-0.11.0", | |||
), | |||
com_github_bombela_backward = dict( | |||
commit = "44ae9609e860e3428cd057f7052e505b4819eb84", # 2018-02-06 | |||
commit = "v1.4", |
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.
use tag
everywhere` for dependencies specified by a tag?
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 tried that but Bazel generates the following error:
ERROR: ~/Library/envoyproxy/envoy/WORKSPACE:6:1: Traceback (most recent call last):
File "~/Library/envoyproxy/envoy/WORKSPACE", line 6
envoy_dependencies()
File "~/Library/envoyproxy/envoy/bazel/repositories.bzl", line 286, in envoy_dependencies
_com_github_bombela_backward()
File "~/Library/envoyproxy/envoy/bazel/repositories.bzl", line 320, in _com_github_bombela_backward
_repository_impl(name = "com_github_bombela_backw...", ...")
File "~/Library/envoyproxy/envoy/bazel/repositories.bzl", line 36, in _repository_impl
fail(("Refusing to depend on Git tag ...)))
Refusing to depend on Git tag "v1.4" for external dependency "com_github_bombela_backward": use 'commit' instead.
ERROR: Error evaluating WORKSPACE file
ERROR: error loading package '': Encountered error while reading extension file 'bazel/repositories.bzl': no such package '@envoy_api//bazel': error loading package 'external': Could not load //external package
bazel/repository_locations.bzl
Outdated
sha256 = "46628a2f068d0e33c716be0ed9dcae4370242df135aed663a180b9fd8e36733d", | ||
strip_prefix = "fmt-4.1.0", | ||
urls = ["https://github.com/fmtlib/fmt/archive/4.1.0.tar.gz"], | ||
commit = "4.1.0", |
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.
What is the motivation moving this from http_archive
to git_repository
? (same for spdlog below as well)
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.
Consistency was the only motivator.
+1 to having |
Now that Pandora's box has been opened, should also loop in @jmillikin-stripe to make sure we're all in agreement :) |
I have a commit that add the sha256 for each of the dependencies. What is the impact of https://docs.bazel.build/versions/master/be/workspace.html#git_repository.sha256 that says
|
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
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.
This looks really great, thanks @moderation for this cleanup. One last minor request (and hopefully @jmillikin-stripe can also sign-off).
|
||
curl https://github.com/nghttp2/nghttp2/archive/"$VERSION".tar.gz -sLo nghttp2-"$VERSION".tar.gz | ||
curl https://github.com/nghttp2/nghttp2/releases/download/v"$VERSION"/nghttp2-"$VERSION".tar.gz -sLo nghttp2-"$VERSION".tar.gz |
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.
Could we add a SHA256 verification step here? Should be a quick one liner.
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.
We aren't sha256 verifying anything in ci/build_container/build_recipes/*
. I think I know how to do this - will report back.
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.
@htuch the commit to add sha256 verification is at 28f0f58 but the change is failing for the Mac build as sha256sum
doesn't exist. I'll do some testing with the solutions at ESGF/esg-search#84
LGTM, thanks! |
…to curl with sha256 check. Signed-off-by: Michael Payne <michael@sooper.org>
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.
Another nice improvement, just one last nit and good to go for me.
@@ -4,11 +4,14 @@ set -e | |||
|
|||
VERSION=1.2.11 | |||
|
|||
curl https://github.com/madler/zlib/archive/v"$VERSION".tar.gz -sLo zlib-"$VERSION".tar.gz | |||
curl https://github.com/madler/zlib/archive/v"$VERSION".tar.gz -sLo zlib-"$VERSION".tar.gz \ | |||
&& echo '629380c90a77b964d896ed37163f5c3a34f6e6d897311f1df2a7016355c45eff' zlib-"$VERSION".tar.gz | sha256sum --check |
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.
Can you restructure to make the SHA a variable like the VERSION above?
@zuercher looks like we're missing |
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.
@zuercher we already install coreutils
so it turns out all we need is sudo ln -s /usr/local/bin/gsha256sum /usr/local/bin/sha256sum
. Or is there a way to approximate this in the build recipe *.sh files?
|
||
curl https://github.com/nghttp2/nghttp2/archive/"$VERSION".tar.gz -sLo nghttp2-"$VERSION".tar.gz | ||
curl https://github.com/nghttp2/nghttp2/releases/download/v"$VERSION"/nghttp2-"$VERSION".tar.gz -sLo nghttp2-"$VERSION".tar.gz |
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.
@htuch the commit to add sha256 verification is at 28f0f58 but the change is failing for the Mac build as sha256sum
doesn't exist. I'll do some testing with the solutions at ESGF/esg-search#84
bazel/repository_locations.bzl
Outdated
@@ -14,42 +14,47 @@ REPOSITORY_LOCATIONS = dict( | |||
strip_prefix = "thrift-0.11.0", | |||
), | |||
com_github_bombela_backward = dict( | |||
commit = "44ae9609e860e3428cd057f7052e505b4819eb84", # 2018-02-06 | |||
sha256 = "ad73be31c5cfcbffbde7d34dba18158a42043a109e7f41946f0b0abd589ed55e", | |||
commit = "v1.4", |
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 sure exactly what this is trying to express, but it won't do what you want.
-
Setting a SHA256 can only work when downloading a single file, such as a tarball. Git clones don't have a deterministic sha256 so there's nowhere the
sha256
value could be used or validated. -
The
commit
kwarg togit_repository
is expected to be a Git commit ID. For fetching by tag, you want thetag
kwarg. Note that this is usually considered less secure + deterministic than fetching by commit, because Git tags can be arbitrarily changed by the repository owner.
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 tag is nicer if we can get a SHA256 for verification; we get self-documenting specs and a stronger hash than that provided by native git commit ID.
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.
What byte blob is the sha256 calculated from?
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.
See prior comments #4353 (comment) and fe04e39#r215618035. kwarg of tag
generates an error. By specifying the sha256 we are downloading the tar file.
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.
Are you sure? Envoy's repositories.bzl uses the Skylark implementation of git_repository
from https://github.com/bazelbuild/bazel/blob/master/tools/build_defs/repo/git.bzl, which doesn't have any hits for "sha256" or "github".
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 @jmillikin-stripe is right. Adding an invalid sha passes through without error in bazel/repository_locations.bzl
. Passing in fakevalue
instead of sha256
works too so Bazel is ignoring what it doesn't comprehend.
My guess is that https://github.com/envoyproxy/envoy/blob/master/bazel/repository_locations.bzl#L33-L35 works as it invokes https://github.com/bazelbuild/bazel/blob/master/tools/build_defs/repo/http.bzl which does support sha256
- https://github.com/bazelbuild/bazel/blob/master/tools/build_defs/repo/http.bzl#L122
In summary, using git means we can't check the sha. We would need to move away from tags to commit ID AND use the http method.
@htuch the original vision of greater maintainability with sha256 checked security isn't panning out :-(
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.
If you're willing to have a stricter dependency on Github and a slight risk of build breakage when they change their tarball builder, you could use Github's auto-generated tarballs with sha256 attrs.
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.
@jmillikin-stripe do you think GH have reduced their churn? I remember the original concern due to GH breaking arbitrarily URLs and SHAs, has this stabilized since? If so, that seems entirely desirable.
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.
If I remember correctly the breakage happened twice -- the first a long time ago (leaving a handful of warning messages in various blog posts), and the second last year when tons of projects got their builds broken.
The true expected rate of change in that endpoint is probably known only to Github.
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.
My hunch is that GH probably don't want to break folks deliberately for no good cause given the reputational damage, so this churn rate should be fairly low. In any case, since we're using point releases, we don't need to use the auto-generated tar balls, we can use just the release tar ball paths..
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
…fication. Standardize order of dict entries. Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
), | ||
com_github_bombela_backward = dict( | ||
commit = "44ae9609e860e3428cd057f7052e505b4819eb84", # 2018-02-06 | ||
remote = "https://github.com/bombela/backward-cpp", | ||
sha256 = "ad73be31c5cfcbffbde7d34dba18158a42043a109e7f41946f0b0abd589ed55e", |
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.
Super awesome, I think even without tags this is much clearer. One quick check; did you verify the sha256
validation is working here as intended?
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 did check the validation and it is working. I changed a bunch of sha's to validate and they fail on build as intended. I also had an incorrect sha for gcovr and pushed a commit to fix. So I think this is ready. With @zuercher's help we also get sha checking on the MacOS builds which is nice.
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.
Rad, thanks!
Signed-off-by: Michael Payne michael@sooper.org
Dependency: Upgrade to backward-cpp v1.4, grpc v1.14.2, googletest v1.8.1, rules_go 0.11.2, buildifier 0.15.0, nghttp2 1.33.0 and subpar 1.3.0. This PR does not update dependencies that rely on commit shas.
Description: nghttp1 1.33.0 release notes, backward-cpp v1.4 release notes, grpc release notes, rules_go release notes, buildifier release notes, googletest release notes and subpar commits & release notes. Add commit date comments to libprotobuf-mutator & jwt_verify_lib
Risk Level: Low
Testing: bazel test //test/... and running on local instances.
Docs Changes: none required
Release Notes: none required