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

buildutil: fix disallowed_imports_test to propagate deps via embed #93477

Merged

Conversation

ajwerner
Copy link
Contributor

Before this change we wouldn't track the dependencies inherited via embedded targets. This does add some cruft because some embedded targets are not go_library targets but rather are things like go_proto_library so we have to check whether the targets have certain attributes. The aspect docs make it seem like this is normal.

Before this change, the following would not fail, now it will:

--- a/pkg/cmd/cockroach/BUILD.bazel
+++ b/pkg/cmd/cockroach/BUILD.bazel
@@ -1,5 +1,6 @@
 load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
 load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
+load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test")

 go_library(
     name = "cockroach_lib",
@@ -20,4 +21,11 @@ go_binary(
     visibility = ["//visibility:public"],
 )

+disallowed_imports_test(
+    "cockroach",
+    disallowed_list = [
+        "//pkg/sql/randgen:randgen",
+    ],
+)
+
ERROR: //pkg/cmd/cockroach:cockroach imports //pkg/sql/randgen:randgen
	check: bazel query 'somepath(//pkg/cmd/cockroach:cockroach, //pkg/sql/randgen:randgen)'

Epic: none

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/fix-disallowed_imports_test branch from 6b85fb7 to 14e5bfe Compare December 13, 2022 06:25
@ajwerner
Copy link
Contributor Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 13, 2022

Build failed (retrying...):

@rickystewart
Copy link
Collaborator

bors r-

Check failing tests.

@craig
Copy link
Contributor

craig bot commented Dec 13, 2022

Canceled.

Before this change we wouldn't track the dependencies inherited via embedded
targets. This does add some cruft because some embedded targets are not
`go_library` targets but rather are things like `go_proto_library` so we have
to check whether the targets have certain attributes. The aspect docs make it
seem like this is normal.

Before this change, the following would not fail, now it will:
```
--- a/pkg/cmd/cockroach/BUILD.bazel
+++ b/pkg/cmd/cockroach/BUILD.bazel
@@ -1,5 +1,6 @@
 load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
 load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
+load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test")

 go_library(
     name = "cockroach_lib",
@@ -20,4 +21,11 @@ go_binary(
     visibility = ["//visibility:public"],
 )

+disallowed_imports_test(
+    "cockroach",
+    disallowed_list = [
+        "//pkg/sql/randgen:randgen",
+    ],
+)
+
```

```
ERROR: //pkg/cmd/cockroach:cockroach imports //pkg/sql/randgen:randgen
	check: bazel query 'somepath(//pkg/cmd/cockroach:cockroach, //pkg/sql/randgen:randgen)'
```
Epic: none

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/fix-disallowed_imports_test branch from 14e5bfe to 127733c Compare December 14, 2022 21:52
@ajwerner
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 14, 2022

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Dec 15, 2022

Build succeeded:

@craig craig bot merged commit 7daec5f into cockroachdb:master Dec 15, 2022
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.

3 participants