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

http_file rule doesn't support canonical_id and is error prone against partial updates (urls values amended, but the hash256 value missed to be amended) #10612

Closed
davido opened this issue Jan 17, 2020 · 0 comments

Comments

@davido
Copy link
Contributor

davido commented Jan 17, 2020

This is the follow up of: [1]. Gerrit Code Review project had an serious build corruption incident: [2].

Because of partial upgrade of http_file() rule, the final artifact was shipped with the file called: caffeine-guava_2.8.1.jar, however, the content was actually caffeine-guava_2.8.0.jar.

The most annoying fact about that build corruption incident is that it is almost went unnoticed, and especially disappointing is the fact, that the CI successfully verified that broken patch set and voted Verified+1.

How could that happen, and what must be done to rectify the problem and prevent it from happening again?

How could that happen

Gerrit CI is using docker images, with pre-warmed caches, so that the current caffeine-guava_2.8.0.jar was shipped with the docker image as the part of the repository cache.

Because in the patch set 1 of: [2] only the URL was upgraded, but the hash value was missed to be upgraded, there was a repository cache hit, and the wrong file was delivered and final artifact was corrupted.

what must be done to rectify the problem

According to this design document: [3] repository cache API was extended with canonical id concept, to fix similar issue related to the http_archive rule: [4]. Unfortunately, http_file was missed to be upgraded, even though it is using the same extended repository cache. So that trying to use canonical_id would fail with:

ERROR: /home/jenkins/workspace/Gerrit-verifier-bazel/gerrit/WORKSPACE:309:1: //external:caffeine-guava-renamed: no such attribute 'canonical_id' in 'http_file' rule

Therefore, the http_file rule must be updated first to offer canonical_id attribute.

[...] and prevent it from happening again

The http_file rule in Gerrit build tool chain must be amended to use the canonical_id:

diff --git a/WORKSPACE b/WORKSPACE
index 6495faac5a..3d9af054ad 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -300,6 +300,8 @@ maven_jar(
     sha1 = "6000774d7f8412ced005a704188ced78beeed2bb",
 )
 
+CAFFEINE_GUAVA_SHA256 = "3a66ee3ec70971dee0bae6e56bda7b8742bc4bedd7489161bfbbaaf7137d89e1"
+
 # TODO(davido): Rename guava.jar to caffeine-guava.jar on fetch to prevent potential
 # naming collision between caffeine guava adapater and guava library itself.
 # Remove this renaming procedure, once this upstream issue is fixed:
@@ -307,7 +309,7 @@ maven_jar(
 http_file(
     name = "caffeine-guava-renamed",
     downloaded_file_path = "caffeine-guava-" + CAFFEINE_VERS + ".jar",
-    sha256 = "3a66ee3ec70971dee0bae6e56bda7b8742bc4bedd7489161bfbbaaf7137d89e1",
+    sha256 = CAFFEINE_GUAVA_SHA256,
     urls = [
         "https://repo1.maven.org/maven2/com/github/ben-manes/caffeine/guava/" +
         CAFFEINE_VERS +
@@ -315,6 +317,7 @@ http_file(
         CAFFEINE_VERS +
         ".jar",
     ],
+    canonical_id = "caffeine-guava-" + CAFFEINE_VERS + ".jar-" + CAFFEINE_GUAVA_SHA256,
 )

With patched Bazel, and with this diff in place, partial upgrades of http_file rule would fail the build:

  $ bazel build :headless
INFO: Invocation ID: 27c281bb-73e3-4ac5-bfbc-983392aeeb8a
DEBUG: /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_toolchains/rules/version_check.bzl:45:9: 
Current running Bazel is not a release version and one was not defined explicitly in rbe_autoconfig target. Falling back to '0.25.2'
DEBUG: /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_skylib/lib/versions.bzl:96:13: Current Bazel is not a release version; cannot check for compatibility. Make sure that you are running at least Bazel 2.0.0.
INFO: Call stack for the definition of repository 'caffeine-guava-renamed' which is a http_file (rule definition at /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_tools/tools/build_defs/repo/http.bzl:382:13):
 - <builtin>
 - /home/davido/projects/gerrit2/WORKSPACE:309:1
WARNING: Download from https://repo1.maven.org/maven2/com/github/ben-manes/caffeine/guava/2.8.1/guava-2.8.1.jar failed: class com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException Checksum was 25da1aee744e477c015b78fe6e015a8795ff91cd9d58be20dfa30b99c0d05ad0 but wanted 3a66ee3ec70971dee0bae6e56bda7b8742bc4bedd7489161bfbbaaf7137d89e1

The fix for http_file is here: [5]. The fix of Gerrit build tool chain on top of it is here: [6].

[1] #5144
[2] https://gerrit-review.googlesource.com/c/gerrit/+/251113/1
[3] https://github.com/bazelbuild/proposals/blob/master/designs/2019-04-29-cache.md
[4] #5144
[5] https://bazel-review.googlesource.com/c/bazel/+/127210
[6] https://gerrit-review.googlesource.com/c/gerrit/+/251095

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

No branches or pull requests

1 participant