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

bazel: issue with cockroachdb/errors patch applied during Bazel build process #64299

Closed
rickystewart opened this issue Apr 27, 2021 · 11 comments · Fixed by #64330
Closed

bazel: issue with cockroachdb/errors patch applied during Bazel build process #64299

rickystewart opened this issue Apr 27, 2021 · 11 comments · Fixed by #64330
Labels
A-build-system C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@rickystewart
Copy link
Collaborator

rickystewart commented Apr 27, 2021

@knz reported the following error in #61913:

kena@kenax ....com/cockroachdb/cockroach % bazel test pkg/util/log:log_test
INFO: Build option --define has changed, discarding analysis cache.
INFO: Repository com_github_cockroachdb_errors instantiated at:
  /data/home/kena/src/go/src/github.com/cockroachdb/cockroach/WORKSPACE:154:8: in <toplevel>
  /data/home/kena/src/go/src/github.com/cockroachdb/cockroach/DEPS.bzl:537:18: in go_deps
Repository rule go_repository defined at:
  /data/home/kena/.cache/bazel/_bazel_kena/9b9f53b7334495d6a871cbb472c466bb/external/bazel_gazelle/internal/go_repository.bzl:194:32: in <toplevel>
ERROR: An error occurred during the fetch of repository 'com_github_cockroachdb_errors':
   Traceback (most recent call last):
        File "/data/home/kena/.cache/bazel/_bazel_kena/9b9f53b7334495d6a871cbb472c466bb/external/bazel_gazelle/internal/go_repository.bzl", line 192, column 10, in _go_repository_impl
                patch(ctx)
        File "/data/home/kena/.cache/bazel/_bazel_kena/9b9f53b7334495d6a871cbb472c466bb/external/bazel_gazelle/internal/go_repository.bzl", line 290, column 17, in patch
                fail("Error applying patch %s:\n%s%s" %
Error in fail: Error applying patch @cockroach//build/patches:com_github_cockroachdb_errors.patch:
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff -urN a/errorspb/BUILD.bazel b/errorspb/BUILD.bazel
|--- a/errorspb/BUILD.bazel     1969-12-31 19:00:00.000000000 -0500
|+++ b/errorspb/BUILD.bazel     2000-01-01 00:00:00.000000000 -0000
--------------------------
Patching file errorspb/BUILD.bazel using Plan A...
Hunk #1 failed at 1.
No such line 35 in input file, ignoring
Hunk #2 succeeded at 25 (offset -12 lines).
1 out of 2 hunks failed--saving rejects to errorspb/BUILD.bazel.rej
done
ERROR: Error fetching repository: Traceback (most recent call last):
        File "/data/home/kena/.cache/bazel/_bazel_kena/9b9f53b7334495d6a871cbb472c466bb/external/bazel_gazelle/internal/go_repository.bzl", line 192, column 10, in _go_repository_impl
                patch(ctx)
        File "/data/home/kena/.cache/bazel/_bazel_kena/9b9f53b7334495d6a871cbb472c466bb/external/bazel_gazelle/internal/go_repository.bzl", line 290, column 17, in patch
                fail("Error applying patch %s:\n%s%s" %
Error in fail: Error applying patch @cockroach//build/patches:com_github_cockroachdb_errors.patch:
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff -urN a/errorspb/BUILD.bazel b/errorspb/BUILD.bazel
|--- a/errorspb/BUILD.bazel     1969-12-31 19:00:00.000000000 -0500
|+++ b/errorspb/BUILD.bazel     2000-01-01 00:00:00.000000000 -0000
--------------------------
Patching file errorspb/BUILD.bazel using Plan A...
Hunk #1 failed at 1.
No such line 35 in input file, ignoring
Hunk #2 succeeded at 25 (offset -12 lines).
1 out of 2 hunks failed--saving rejects to errorspb/BUILD.bazel.rej
done
ERROR: /data/home/kena/src/go/src/github.com/cockroachdb/cockroach/pkg/util/log/BUILD.bazel:120:8: //pkg/util/log:log_test depends on @com_github_cockroachdb_errors//:errors in repository @com_github_cockroachdb_errors which failed to fetch. no such package '@com_github_cockroachdb_errors//': Error applying patch @cockroach//build/patches:com_github_cockroachdb_errors.patch:
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff -urN a/errorspb/BUILD.bazel b/errorspb/BUILD.bazel
|--- a/errorspb/BUILD.bazel     1969-12-31 19:00:00.000000000 -0500
|+++ b/errorspb/BUILD.bazel     2000-01-01 00:00:00.000000000 -0000
--------------------------
Patching file errorspb/BUILD.bazel using Plan A...
Hunk #1 failed at 1.
No such line 35 in input file, ignoring
Hunk #2 succeeded at 25 (offset -12 lines).
1 out of 2 hunks failed--saving rejects to errorspb/BUILD.bazel.rej
done
ERROR: Analysis of target '//pkg/util/log:log_test' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.521s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (32 packages loaded, 122 targets configured)
FAILED: Build did NOT complete successfully (32 packages loaded, 122 targets configured)
    Fetching @com_github_cockroachdb_datadriven; fetching
    Fetching @com_github_kr_pretty; fetching
    Fetching @com_github_stretchr_testify; fetching
    Fetching @org_golang_x_net; fetching

Not sure if this is unique to FreeBSD and its patch, or if there's an issue with BUILD file generation I haven't seen.

@rickystewart rickystewart added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-build-system labels Apr 27, 2021
@rickystewart
Copy link
Collaborator Author

@knz When you get a moment, can you help debug by doing the following after you reproduce the failure:

cat `bazel info output_base`/external/com_github_cockroachdb_errors/errorspb/BUILD.bazel{,.rej,.orig}

@rickystewart
Copy link
Collaborator Author

Oh, also, is there anything in your .bazelrc.user?

@knz
Copy link
Contributor

knz commented Apr 28, 2021

Oh, also, is there anything in your .bazelrc.user?

the file doesn't exist

cat $(bazel info output_base)/external/com_github_cockroachdb_errors/errorspb/BUILD.bazel{,.rej,.orig}

% cat $(bazel info output_base)/external/com_github_cockroachdb_errors/errorspb/BUILD.bazel.rej
@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@rules_proto//proto:defs.bzl", "proto_library")

 filegroup(
     name = "go_default_library_protos",
% cat $(bazel info output_base)/external/com_github_cockroachdb_errors/errorspb/BUILD.bazel.orig
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
    name = "errorspb",
    srcs = [
        "errors.pb.go",
        "hintdetail.pb.go",
        "markers.go",
        "markers.pb.go",
        "tags.pb.go",
        "testing.go",
        "testing.pb.go",
    ],
    importpath = "github.com/cockroachdb/errors/errorspb",
    visibility = ["//visibility:public"],
    deps = [
        "@com_github_gogo_protobuf//gogoproto",
        "@com_github_gogo_protobuf//proto",
        "@com_github_gogo_protobuf//types",
    ],
)

alias(
    name = "go_default_library",
    actual = ":errorspb",
    visibility = ["//visibility:public"],
)

@knz
Copy link
Contributor

knz commented Apr 28, 2021

As you can see, my original BUILD.bazel has a go_library(... directive at the beginning, not filegroup like the patch expected.

@knz
Copy link
Contributor

knz commented Apr 28, 2021

Here's another behavior I had noticed earlier, which I didn't think was relevant, but maybe it is (you tell me)

When I run make bazel-generate on a clean clone of the repository, I get a diff in the toplevel BUILD.bazel at the root:

--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -1,3 +1,5 @@
+load("@rules_proto//proto:defs.bzl", "proto_library")
+load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 load("@bazel_gazelle//:def.bzl", "gazelle")

 # The following directives inform gazelle how to auto-generate BUILD.bazel
@@ -199,3 +201,17 @@ gazelle(
 exports_files([
     "TEAMS.yaml",
 ])
+
+proto_library(
+    name = "profile_proto",
+    srcs = ["all.proto"],
+    visibility = ["//visibility:public"],
+)
+
+go_proto_library(
+    name = "profile_go_proto",
+    compilers = ["//pkg/cmd/protoc-gen-gogoroach:protoc-gen-gogoroach_grpc_compiler"],
+    importpath = "github.com/cockroachdb/cockroach",
+    proto = ":profile_proto",
+    visibility = ["//visibility:public"],
+)

I think this latter behavior started to happen at some point after I upgraded bazel, but I am not sure.

It doesn't seem to impact the issue we're studying (i.e. my bazel test run fails with the same patch error with or without this change to the toplevel BUILD.bazel).

@knz
Copy link
Contributor

knz commented Apr 28, 2021

FYI I just tried downgrading from bazel 4.0.0 to 3.7.0, and it didn't change any of the symptoms above.

@rickystewart
Copy link
Collaborator Author

My $(bazel info output_base)/external/com_github_cockroachdb_errors/errorspb/BUILD.bazel.orig actually has the same content as yours, so I assume that that patch just no longer works in general -- a Gazelle upgrade probably did that. No idea why it wasn't noticed in anyone's local runs or in CI, but I assume that your patch binary is just a little more stringent than others. Let me try to fix that really quick.

When I run make bazel-generate on a clean clone of the repository, I get a diff in the toplevel BUILD.bazel at the root:

Hmm. This shouldn't happen unless there's a file all.proto at the root of your repo. I assume there isn't because you say the clone is fresh?

@rickystewart
Copy link
Collaborator Author

rickystewart commented Apr 28, 2021

Can you cleanly bazel build pkg/cmd/cockroach-short if you apply the following patch to your workspace?

diff --git a/build/patches/com_github_cockroachdb_errors.patch b/build/patches/com_github_cockroachdb_errors.patch
index b3633e3dd7..9691c42c91 100644
--- a/build/patches/com_github_cockroachdb_errors.patch
+++ b/build/patches/com_github_cockroachdb_errors.patch
@@ -5,8 +5,8 @@ diff -urN a/errorspb/BUILD.bazel b/errorspb/BUILD.bazel
  load("@io_bazel_rules_go//go:def.bzl", "go_library")
 +load("@rules_proto//proto:defs.bzl", "proto_library")
  
- filegroup(
-     name = "go_default_library_protos",
+ go_library(
+     name = "errorspb",
 @@ -36,3 +37,14 @@
      actual = ":errorspb",
      visibility = ["//visibility:public"],

@knz
Copy link
Contributor

knz commented Apr 28, 2021

This shouldn't happen unless there's a file all.proto at the root of your repo. I assume there isn't because you say the clone is fresh?

Aha! the clone was fresh but the working directory wasn't. It was indeed all.proto. That solves it.

I will now try your patch fix next.

@knz
Copy link
Contributor

knz commented Apr 28, 2021

Ok the build doesn't fail because of the patch any more. I'll send a PR your way.

@knz
Copy link
Contributor

knz commented Apr 28, 2021

Ok so together with your PR #64330

and mine #64336

and a manual edit to work around #64334,

then my build completes without error.

craig bot pushed a commit that referenced this issue Apr 28, 2021
64330: bazel: tweak patch we apply to `cockroachdb/errors` r=rail a=rickystewart

This patch became outdated as a result of a Gazelle upgrade.
Resolves #64299.

Release note: None

Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in 1d5d21a Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-system C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
2 participants