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

bazel: pkg/roachpb:bootstrap and pkg/sql/opt/optgen/lang:bootstrap are a disaster #68654

Closed
rickystewart opened this issue Aug 10, 2021 · 1 comment · Fixed by #68737
Closed

bazel: pkg/roachpb:bootstrap and pkg/sql/opt/optgen/lang:bootstrap are a disaster #68654

rickystewart opened this issue Aug 10, 2021 · 1 comment · Fixed by #68737
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

@rickystewart
Copy link
Collaborator

This target depends on batch_generated.go, which is wrong since we're depending on a checked-in generated file. But it can't be replaced with :gen-batch-generated because it introduces a cyclic dependency error:

ERROR: /Users/ricky/go/src/github.com/cockroachdb/cockroach/pkg/roachpb/gen/BUILD.bazel:39:10: in go_binary rule //pkg/roachpb/gen:gen: cycle in dependency graph:
    //pkg/cmd/cockroach-short:cockroach-short
    //pkg/cmd/cockroach-short:cockroach-short_lib
    //pkg/ccl:ccl
    //pkg/ccl/oidcccl:oidcccl
    //pkg/ui:ui
    //pkg/base:base
    //pkg/security:security
    //pkg/settings/cluster:cluster
    //pkg/clusterversion:clusterversion
    //pkg/clusterversion:clusterversion_go_proto
    //pkg/roachpb:with-mocks
    //pkg/roachpb:roachpb
    //pkg/roachpb:gen-batch-generated
.-> //pkg/roachpb/gen:gen
|   //pkg/roachpb/gen:gen_lib
|   //pkg/roachpb:bootstrap
|   //pkg/roachpb:gen-batch-generated
`-- //pkg/roachpb/gen:gen

Epic CRDB-8035
@rickystewart rickystewart 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 Aug 10, 2021
@rickystewart rickystewart changed the title bazel: pkg/roachpb:bootstrap is a disaster bazel: pkg/roachpb:bootstrap and pkg/sql/opt/optgen/lang:bootstrap are a disaster Aug 10, 2021
@rickystewart
Copy link
Collaborator Author

pkg/sql/opt/optgen/lang:bootstrap has the same problem, actually.

@rickystewart rickystewart mentioned this issue Aug 11, 2021
Merged
rickystewart added a commit to rickystewart/cockroach that referenced this issue Aug 11, 2021
@rickystewart
bazel,dev: add robust support/validation for checked-in generated code
fd60d38 
While testing cockroachdb#68663, I noticed that there were a few build targets that
didn't look as I expected since they referenced checked-in generated
code in order to generate that same code in the sandbox -- e.g., the
checked-in `batch_generated.go` was used to generate an (identical) file
`batch_generated-gen.go` within the sandbox. I filed the issue cockroachdb#68654 to
investigate this, and gave a college try to stamping out the recursive
structure by refactoring the affected code. I wasn't successful :(

While looking at this I encountered comments by Irfan in
`pkg/{roachpb/gen,sql/opt/optgen/cmd/langgen}/BUILD.bazel` explaining
that this generated code needs to remain checked-in in order to
bootstrap new versions of the code, and that our tooling needs to have
support for extracting this code back into the workspace so devs can
have a sane workflow. Fair enough, so this PR implements all of that.

* Add `dev generate go` to generate the checked-in generated code;
* fix up `dev generate` so that it generates for `bazel`, `docs`, and
  `go` altogether (so `dev generate` is a one-step thing you can do to
  generate everything you'll need to pass CI);
* build the generated Go code via the sandbox in CI and test to make
  sure it matches what's checked in.

Closes cockroachdb#68654.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Aug 12, 2021
@rickystewart
bazel,dev: add robust support/validation for checked-in generated code
cef1ee0 
While testing cockroachdb#68663, I noticed that there were a few build targets that
didn't look as I expected since they referenced checked-in generated
code in order to generate that same code in the sandbox -- e.g., the
checked-in `batch_generated.go` was used to generate an (identical) file
`batch_generated-gen.go` within the sandbox. I filed the issue cockroachdb#68654 to
investigate this, and gave a college try to stamping out the recursive
structure by refactoring the affected code. I wasn't successful :(

While looking at this I encountered comments by Irfan in
`pkg/{roachpb/gen,sql/opt/optgen/cmd/langgen}/BUILD.bazel` explaining
that this generated code needs to remain checked-in in order to
bootstrap new versions of the code, and that our tooling needs to have
support for extracting this code back into the workspace so devs can
have a sane workflow. Fair enough, so this PR implements all of that.

* Add `dev generate go` to generate the checked-in generated code;
* fix up `dev generate` so that it generates for `bazel`, `docs`, and
  `go` altogether (so `dev generate` is a one-step thing you can do to
  generate everything you'll need to pass CI);
* build the generated Go code via the sandbox in CI and test to make
  sure it matches what's checked in.

Closes cockroachdb#68654.

Release note: None
craig bot pushed a commit that referenced this issue Aug 14, 2021
@xinhaoz @rickystewart
Merge #68566 #68737 #68798
a722d9a 
68566: ui: Align db console logical plan with sql shell r=xinhaoz a=xinhaoz

Resolves: #67328

Release note (ui change): The 'Logical Plan' tab in db console
has been renamed 'Explain Plan' and the plan format displayed
has been updated to match the output of the EXPLAIN command in
the SQL shell. Global EXPLAIN properties have been added to
the logical plan in db-console which were previously missing.

The EXPLAIN format shown below should now be be reflected in db console:

```
  distribution: full
  vectorized: true

  • hash join
  │ estimated row count: 503
  │ equality: (rider_id) = (id)
  │
  ├── • scan
  │     estimated row count: 513 (100% of the table; stats collected 9 seconds ago)
  │     table: rides@primary
  │     spans: FULL SCAN
  │
  └── • scan
        estimated row count: 50 (100% of the table; stats collected 1 minute ago)
        table: users@primary
        spans: FULL SCAN
```

---------------
previous db-console view:
![image](https://user-images.githubusercontent.com/20136951/128762673-eff3f353-0b5c-4dcf-99f4-13466b45b07c.png)

updated db-console Explain Tab of above:
![image](https://user-images.githubusercontent.com/20136951/128725278-00124028-c83e-40ad-9f91-f4aaed9057e4.png)


68737: bazel,dev: add robust support/validation for checked-in generated code r=irfansharif a=rickystewart

(Only the most recent commit applies for this review.)

While testing #68663, I noticed that there were a few build targets that
didn't look as I expected since they referenced checked-in generated
code in order to generate that same code in the sandbox -- e.g., the
checked-in `batch_generated.go` was used to generate an (identical) file
`batch_generated-gen.go` within the sandbox. I filed the issue #68654 to
investigate this, and gave a college try to stamping out the recursive
structure by refactoring the affected code. I wasn't successful :(

While looking at this I encountered comments by Irfan in
`pkg/{roachpb/gen,sql/opt/optgen/cmd/langgen}/BUILD.bazel` explaining
that this generated code needs to remain checked-in in order to
bootstrap new versions of the code, and that our tooling needs to have
support for extracting this code back into the workspace so devs can
have a sane workflow. Fair enough, so this PR implements all of that.

* Add `dev generate go` to generate the checked-in generated code;
* fix up `dev generate` so that it generates for `bazel`, `docs`, and
  `go` altogether (so `dev generate` is a one-step thing you can do to
  generate everything you'll need to pass CI);
* build the generated Go code via the sandbox in CI and test to make
  sure it matches what's checked in.

Closes #68654.

Release note: None

68798: codeowners: fix sql catch-all rule r=postamar a=postamar

Previously, there was a rule assigning ownership of `pkg/sql` and all
subdirectories to a -noreview group. This rule was near the bottom of
the CODEOWNERS file and because of "last rule wins" would override any
previous rules.

This commit therefore moves this rule to the top, to serve instead as
a default rule.

This commit also assigns ownership to `pkg/startupmigrations`.

Release note: None

Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
@craig craig bot closed this as completed in 522e15b Aug 14, 2021
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.

bazel,dev: add robust support/validation for checked-in generated code rickystewart/cockroach
1 participant
@rickystewart