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

Upgrade rules_go to 0.16.0 #4836

Merged

Conversation

jmillikin-stripe
Copy link
Contributor

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
The build is using a rules_go commit from around 0.15 -- this PR bumps it up to the 0.16 point release.

Risk Level: Low
Testing: CI
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: John Millikin [email protected]

Signed-off-by: John Millikin <[email protected]>
@mattklein123 mattklein123 merged commit ec3aec6 into envoyproxy:master Oct 23, 2018
@jmillikin-stripe jmillikin-stripe deleted the jmillikin_update-rules-go branch October 23, 2018 22:38
@moderation
Copy link
Contributor

@jmillikin-stripe This has already been merged but for consistency sake this should be:

    io_bazel_rules_go = dict(
        sha256 = "d0e51353639cd6ea43e614d80c29d3f7f318e88dbbc28c4d8c07b129b46e20c0",
        strip_prefix = "rules_go-0.16.0",
        urls = ["https://github.com/bazelbuild/rules_go/archive/0.16.0.tar.gz"],
    ),

@jmillikin-stripe
Copy link
Contributor Author

Why? That's the URL for Github's auto-generated tarballs, which aren't uploaded by the maintainer and can change their sha256 when Github's tar/gzip libs are upgraded. Using the uploaded tarball is more stable and more trustable (the maintainer can provide the sha).

@moderation
Copy link
Contributor

Recently bazel/repository_locations.bzl was updated (#4779) to change all artifacts that aren't referred to by commit (either because we pin to a particular release or the projects don't do releases) to the method I refer too. The approach is documented at https://github.com/envoyproxy/envoy/blob/master/bazel/EXTERNAL_DEPS.md#distdir---prefetching-dependencies.

With the exception of your PR all references to artifacts adhere to this approach.

The following dependencies don't provide maintainer uploaded tarballs - bombela/backward-cpp, Cyan4973/xxHash, eile/tclap, gabime/spdlog, gcovr/gcovr, grpc/grpc, opentracing/opentracing-cpp, lightstep/lightstep-tracer-cpp, pallets/markupsafe, Tencent/rapidjson, google/googletest & google/subpar.

Maybe we change the doco to say use maintainer uploaded tarballs if they exist?

@mattklein123
Copy link
Member

Sorry I approved this without looking at it carefully. @jmillikin-stripe it would be good to make it match the others.

@jmillikin-stripe
Copy link
Contributor Author

Sorry, could you explain in more detail why you need to use Github's auto-generated tarball instead of the official release tarball? Both should be acceptable to --distdir. Is there a script somewhere that only works if the URL contains a SHA1-shaped hex blob? If so, we should fix that.

@mattklein123
Copy link
Member

Sorry I don't know the details here. @htuch @lizan ^ ?

@moderation
Copy link
Contributor

Doing a further review of bazel/repository_locations.bzl the only repositories in addition to rules_go that provide maintainer uploaded artifacts are:

  1. bazelbuild/bazel-gazelle
  2. fmtlib/fmt (zip format - need to check if this works)
  3. golang/protobuf (currently pinned to a commit and will be removed when rules_go picks up the required commit)

None of the dependencies at api/bazel/repositories.bzl do.

Proposal - convert 1 and 2 of the above dependencies to the method @jmillikin-stripe used for rules_go and update bazel/EXTERNAL_DEPS.md to state that we should use maintainer provided artifacts and sha's when available.

@htuch
Copy link
Member

htuch commented Oct 24, 2018

@moderation that seems reasonable. I don't have a strong argument to use GH tarballs vs. official releases, presumably it's preferred to use the blessed official releases all things being equal.

@lizan
Copy link
Member

lizan commented Oct 25, 2018

TBH it doesn't matter GH tarballs vs. maintainer uploaded releases as long as we defined them with sha256 sum. Though I don't have any preference if @moderation feels strong for that I'm fine with that.

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.

5 participants