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: remove old protos when generating new ones #76163

Merged

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Feb 7, 2022

This is what the Makefile did. It was painful to have the old onces because
they'd lead to spurious diffs.

Release note: None

@ajwerner ajwerner requested a review from a team as a code owner February 7, 2022 15:29
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/remove-proto-files-when-generating branch from c75d2b2 to f5c00bd Compare February 7, 2022 20:34
@ajwerner
Copy link
Contributor Author

ajwerner commented Feb 7, 2022

@rickystewart I reworked this to make it more generally composable for where I think this is going. I also took the opportunity to generate the dependency list. PTAL

bazel query 'kind(go_proto_library, //pkg/...)' \
| sort \
| awk '
BEGIN {printf("# Generated by bazel-generate.sh\n\nprotobuf_srcs = [\n") }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, I decided to take this one step further because I think I'm going to generate a bunch of these.

@@ -0,0 +1,64 @@
# Generated by bazel-generate.sh

protobuf_srcs = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would expect this variable name to be in ALL_CAPS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ajwerner ajwerner force-pushed the ajwerner/remove-proto-files-when-generating branch from c4bca4e to 1e0695e Compare February 7, 2022 22:15
@@ -0,0 +1,131 @@
// Command genbzl is used to generate bazel files which then get imported by
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the other Go binaries we use for generating Bazel stuff are in pkg/cmd (e.g.: mirror, generate-staticcheck, generate-test-suites). This one should live in the same place IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed that, but I think, honestly, it's a mistake. We've long had binaries all over the place. The cmd directory, as far as I'm concerned, is for binaries which we might expect somebody to want to actually run as a binary. Maybe that's controversial. I'd honestly be more inclined to move all of those to some common subdirectory which makes it more obvious what they are there for.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Makes sense to me.

@ajwerner ajwerner force-pushed the ajwerner/remove-proto-files-when-generating branch 2 times, most recently from 8f9a2a7 to 1e889ea Compare February 7, 2022 23:14
It's the only checked in proto file.

Release note: None
…ones

This is what the Makefile did. It was painful to have the old onces because
they'd lead to spurious diffs.

This commit also automates the generation of the protobuf sources into a
bzl file which is now an unexported input to the rule.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/remove-proto-files-when-generating branch from 1e889ea to 8b01152 Compare February 8, 2022 02:17
@ajwerner
Copy link
Contributor Author

ajwerner commented Feb 8, 2022

bors r+

@craig craig bot merged commit a7b1c17 into cockroachdb:master Feb 8, 2022
@craig
Copy link
Contributor

craig bot commented Feb 8, 2022

Build succeeded:

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