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.

Sign up for GitHub

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

Jump to bottom

dev: spurious diffs appear when hoisting generated code into tree #72232

Closed
irfansharif opened this issue Oct 29, 2021 · 1 comment · Fixed by #72675
Closed

dev: spurious diffs appear when hoisting generated code into tree #72232

irfansharif opened this issue Oct 29, 2021 · 1 comment · Fixed by #72675
Assignees
@rickystewart
Labels
A-build-system C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-dev-inf

Comments

@irfansharif
Copy link
Contributor

irfansharif commented Oct 29, 2021 •
edited by rickystewart
Loading

Describe the problem

The checked in generated code (protos and various stringer files) are generated using make. When generating using bazel/dev (introduced in #68663), you can observe spurious diffs.

$ dev build cockroach-short --hoist-generated-code
$ git diff

The diffs (at the time of writing) are of three sorts:

diff --git i/pkg/base/testclusterreplicationmode_string.go w/pkg/base/testclusterreplicationmode_string.go
index 6380f66b3e..c14b8c058f 100644
--- i/pkg/base/testclusterreplicationmode_string.go
+++ w/pkg/base/testclusterreplicationmode_string.go
@@ -1,4 +1,4 @@
-// Code generated by "stringer -type=TestClusterReplicationMode"; DO NOT EDIT.
+// Code generated by "stringer -output=bazel-out/darwin-fastbuild/bin/pkg/base/testclusterreplicationmode_string.go -type=TestClusterReplicationMode pkg/base/test_server_args.go"; DO NOT EDIT.
diff --git i/pkg/ccl/baseccl/encryption_options.pb.go w/pkg/ccl/baseccl/encryption_options.pb.go
index a7edf44210..63e766c050 100644
--- i/pkg/ccl/baseccl/encryption_options.pb.go
+++ w/pkg/ccl/baseccl/encryption_options.pb.go
@@ -611,3 +611,4 @@ var (
        ErrIntOverflowEncryptionOptions          = fmt.Errorf("proto: integer overflow")
        ErrUnexpectedEndOfGroupEncryptionOptions = fmt.Errorf("proto: unexpected end of group")
 )
+

And new (go ignored) files:

$ cat pkg/server/serverpb/init.pb.gw.go
// +build ignore

package ignore

Expected behavior

For there to be no extraneous diffs. For protos the extra empty line is something gofmt gets rid of, so feels to me we should get dev/bazel to apply it when hoisting out of the sandbox. For the stringer files, we could either ignore them in the source tree or use a custom stringer binary in bazel to strip out the sandbox path? Not sure about the .pb.gw.go files.

Epic CRDB-8036
@irfansharif irfansharif added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-build-system T-dev-inf labels Oct 29, 2021
@irfansharif
Copy link
Contributor Author

irfansharif commented Dec 21, 2021 •
edited
Loading

#72675 deals with the proto files by removing them from tree. #72704 squashed the stringer diffs.

@rickystewart rickystewart self-assigned this Dec 21, 2021
rickystewart added a commit to rickystewart/cockroach that referenced this issue Dec 22, 2021
@rickystewart
*: delete a bunch of generated go code from tree
1879ea3 
Before this change, `dev build --hoist-generated-code` would introduce a
lot of spurious, benign diffs in generated code. I've validated that
you can `dev build --hoist-generated code` without introducing any
diffs with this change applied.

The code we're deleting from tree here is:
* *ALL* `.pb.go` and `.pb.gw.go` files;
* and an ad-hoc selection of generated files that cannot be hoisted into
  the worktree by `dev` without introducing diffs (see cockroachdb#72232).

My first attempt at solving this problem was by deleting "all" generated
code from tree but I quickly found that this was intractable: the large
majority of these generated files don't have bespoke support in the
`Makefile`, so deleting them from tree and running `make build` just
causes the build to fail. Instead, the approach here is more targeted:
I delete only the generated files that have diffs when hoisted by `dev`,
and add support in the `Makefile` for those specific generated files.
The remaining checked-in generated files aren't hurting anyone, and can
be deleted from tree wholesale after `make` is dead.

NB: This change can require running `make generate` (or, in certain more
constrained scenarios, `make protobuf`) to make some builds/tests work.

Closes cockroachdb#72232.

Release note: None
@rickystewart rickystewart mentioned this issue Dec 22, 2021
Merged
rickystewart added a commit to rickystewart/cockroach that referenced this issue Dec 22, 2021
@rickystewart
*: delete a bunch of generated go code from tree
cc7b2c6 
Before this change, `dev build --hoist-generated-code` would introduce a
lot of spurious, benign diffs in generated code. I've validated that
you can `dev build --hoist-generated code` without introducing any
diffs with this change applied.

The code we're deleting from tree here is:
* *ALL* `.pb.go` and `.pb.gw.go` files;
* and an ad-hoc selection of generated files that cannot be hoisted into
  the worktree by `dev` without introducing diffs (see cockroachdb#72232).

My first attempt at solving this problem was by deleting "all" generated
code from tree but I quickly found that this was intractable: the large
majority of these generated files don't have bespoke support in the
`Makefile`, so deleting them from tree and running `make build` just
causes the build to fail. Instead, the approach here is more targeted:
I delete only the generated files that have diffs when hoisted by `dev`,
and add support in the `Makefile` for those specific generated files.
The remaining checked-in generated files aren't hurting anyone, and can
be deleted from tree wholesale after `make` is dead.

NB: This change can require running `make generate` (or, in certain more
constrained scenarios, `make protobuf`) to make some builds/tests work.

Closes cockroachdb#72232.

Release note: None
craig bot pushed a commit that referenced this issue Dec 23, 2021
@rickystewart @jackcwu
Merge #72675 #74210
8e22c25 
72675: *: delete a bunch of generated go code from tree  r=irfansharif a=rickystewart

Before this change, `dev build --hoist-generated-code` would introduce a
lot of spurious, benign diffs in generated code. I've validated that
you can `dev build --hoist-generated code` without introducing any
diffs with this change applied.

The code we're deleting from tree here is:
* *ALL* `.pb.go` and `.pb.gw.go` files;
* and an ad-hoc selection of generated files that cannot be hoisted into
  the worktree by `dev` without introducing diffs (see #72232).

My first attempt at solving this problem was by deleting "all" generated
code from tree but I quickly found that this was intractable: the large
majority of these generated files don't have bespoke support in the
`Makefile`, so deleting them from tree and running `make build` just
causes the build to fail. Instead, the approach here is more targeted:
I delete only the generated files that have diffs when hoisted by `dev`,
and add support in the `Makefile` for those specific generated files.
The remaining checked-in generated files aren't hurting anyone, and can
be deleted from tree wholesale after `make` is dead.

NB: This change can require running `make generate` (or, in certain more
constrained scenarios, `make protobuf`) to make some builds/tests work.

Closes #72232.

Release note: None

74210: sql: deprecate GRANT privilege r=jackcwu a=jackcwu

Resolves #73065

Release note (sql change): We will be deprecating the GRANT privilege in 22.1
before eventually removing it in 22.2 in favor of grant options. To promote
backwards compatibility for users with code still using GRANT, we will give
grant options on every privilege a user has when they are granted GRANT and
remove all their grant options when GRANT is revoked, in addition to the
existing grant option behavior.

Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Jack Wu <[email protected]>
@craig craig bot closed this as completed in 521a561 Dec 23, 2021
gustasva pushed a commit to gustasva/cockroach that referenced this issue Jan 4, 2022
@rickystewart
*: delete a bunch of generated go code from tree
ff4cf77 
Before this change, `dev build --hoist-generated-code` would introduce a
lot of spurious, benign diffs in generated code. I've validated that
you can `dev build --hoist-generated code` without introducing any
diffs with this change applied.

The code we're deleting from tree here is:
* *ALL* `.pb.go` and `.pb.gw.go` files;
* and an ad-hoc selection of generated files that cannot be hoisted into
  the worktree by `dev` without introducing diffs (see cockroachdb#72232).

My first attempt at solving this problem was by deleting "all" generated
code from tree but I quickly found that this was intractable: the large
majority of these generated files don't have bespoke support in the
`Makefile`, so deleting them from tree and running `make build` just
causes the build to fail. Instead, the approach here is more targeted:
I delete only the generated files that have diffs when hoisted by `dev`,
and add support in the `Makefile` for those specific generated files.
The remaining checked-in generated files aren't hurting anyone, and can
be deleted from tree wholesale after `make` is dead.

NB: This change can require running `make generate` (or, in certain more
constrained scenarios, `make protobuf`) to make some builds/tests work.

Closes cockroachdb#72232.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rickystewart rickystewart

Labels
A-build-system C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-dev-inf
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

*: delete a bunch of generated go code from tree rickystewart/cockroach
2 participants
@rickystewart @irfansharif