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

build: add missing plpgsql build rules #101930

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Conversation

e-mbrown
Copy link
Contributor

@e-mbrown e-mbrown commented Apr 20, 2023

Resolve: #101544

The "Github CI"/ "Lint" target in TC was failing due to missing plpgsql build rules. This commit should add the rules.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@e-mbrown e-mbrown marked this pull request as ready for review April 20, 2023 17:50
@e-mbrown e-mbrown requested a review from a team as a code owner April 20, 2023 17:50
@e-mbrown e-mbrown force-pushed the eb/make branch 5 times, most recently from 15cc543 to 99dc2c8 Compare April 20, 2023 21:17
@knz
Copy link
Contributor

knz commented Apr 24, 2023

The main issue I'm seeing here is that the new plpgsql depends on the sql/parser package; and so the source code for the latter needs to be ready before the source code for the former is processed. I'll show what I mean in a separate commit.

@knz
Copy link
Contributor

knz commented Apr 24, 2023

Ok we're out of the woods - i seems the package dependency doesn't matter for file generation.

Copy link
Contributor Author

@e-mbrown e-mbrown left a comment

Choose a reason for hiding this comment

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

Hm, we are getting this error still undefined: lexbase, previously the error was undefined: lexbase.DOT_DOT or other keyword. Ricky thinks that goimports might not be getting called, or maybe it is returning an incorrect result for some reason.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@knz
Copy link
Contributor

knz commented Apr 24, 2023

drat, it failed again. I'm looking further now

@knz
Copy link
Contributor

knz commented Apr 24, 2023

This is the failure:

[21:30:53] :			 [run] # github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser
[21:30:53] :			 [run] plpgsql-gen.y:260: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:261: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:262: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:263: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:264: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:265: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:266: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:268: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:269: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:270: undefined: lexbase
[21:30:53] :			 [run] plpgsql-gen.y:270: too many errors

I think i know what's up. i'll try something.

@knz
Copy link
Contributor

knz commented Apr 24, 2023

Here's how to repro locally:

rm pkg/sql/plpgsql/parser/lexbase/tokens.go
rm pkg/sql/plpgsql/parser/lexbase/keywords.go
rm pkg/sql/plpgsql/parser/plpgsql.go
make pkg/sql/plpgsql/parser/plpgsql.go

The issue is because the code needs to import lexbase, but goimports doesn't know how to add it unless the lexbase files have been generated already.

there are 2 fixes possible:

  • add a dependency to only generate plpgsql.go after the other files have been generated already; OR
  • add the import manually to plpgsql.y

I think the latter is easier. I'll try this next.

@knz
Copy link
Contributor

knz commented Apr 24, 2023

Next hurdle below . @rickystewart why did the Bazel CI target not find this already? Is this linter not running in bazel? It's a super important linter.

    lint_test.go:95:
        pkg/sql/plpgsql/parser/plpgsql-gen.y:692: Parse(): message argument is not a constant expression
        Tip: use YourFuncf("descriptive prefix %s", ...) or list new formatting wrappers in pkg/testutils/lint/passes/fmtsafe/functions.go.
        N.B. additional entry is required functions in the main package. See functions.go#requireConstFmt

@rickystewart
Copy link
Collaborator

Generally we don't have lints on for generated code.

@knz
Copy link
Contributor

knz commented Apr 24, 2023

@ZhouXing19 you might be interested in the diff for the latest commit I added to this PR.

--- a/pkg/sql/plpgsql/parser/plpgsql.y
+++ b/pkg/sql/plpgsql/parser/plpgsql.y
@@ -688,7 +689,8 @@ getdiag_item: unreserved_keyword {
     case "returned_sqlstate":
       $$.val = plpgsqltree.PlpgsqlGetdiagReturnedSqlstate;
     default:
-      setErr(plpgsqllex, errors.New("unrecognized GET DIAGNOSTICS item " + $1 ))
+      // TODO(jane): Should this use an unimplemented error instead?
+      setErr(plpgsqllex, errors.Newf("unrecognized GET DIAGNOSTICS item: %s", redact.Safe($1)))
   }
 }

@knz
Copy link
Contributor

knz commented Apr 24, 2023

Generally we don't have lints on for generated code.

@rickystewart This is a problem: the "generated code" for all files is generated from templates that do not have the .go extension, but are written by humans. If these templates contain code that violates our lint rules and bazel doesn't detect that, it's bad.

@knz
Copy link
Contributor

knz commented Apr 24, 2023

This is a problem: the "generated code" for all files is generated from templates that do not have the .go extension, but are written by humans. If these templates contain code that violates our lint rules and bazel doesn't detect that, it's bad.

Filed this separate issue for that: #102191.

@rickystewart
Copy link
Collaborator

This is a problem: the "generated code" for all files is generated from templates that do not have the .go extension, but are written by humans. If these templates contain code that violates our lint rules and bazel doesn't detect that, it's bad.

If we can find a list of which "generated code" files should and should not be linted, then it'll be easy to apply it. I suspect that we have not fully thought through which "generated code" files are and are not lint-worthy.

@knz
Copy link
Contributor

knz commented Apr 24, 2023

Let's move the lint discussion to #102191.

@knz
Copy link
Contributor

knz commented Apr 24, 2023

I'm cautiously optimistic that my latest amend has fixed the remaining CI issues.

I'll disconnect from the computer now. If CI is indeed happy in the end feel free to squash and merge.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

LGTM modulo squashing the commits together.

NB: you'll need to git fetch (or pull) my last commit locally from your remote branch before you can squash.

@knz
Copy link
Contributor

knz commented Apr 25, 2023

i'm going to go ahead and merge this so that the remainder of the team doesn't continue to worry something is amiss.

bors r+

@knz

This comment was marked as outdated.

@craig

This comment was marked as outdated.

The "Github CI"/ "Lint" target in TC was failing due to
missing plpgsql build rules. This commit should add the
rules.

Release note: None
@knz

This comment was marked as outdated.

@craig
Copy link
Contributor

craig bot commented Apr 25, 2023

Build succeeded:

@craig craig bot merged commit 5114a6b into cockroachdb:master Apr 25, 2023
@e-mbrown
Copy link
Contributor Author

blathers backport 23.1 22.2

@blathers-crl
Copy link

blathers-crl bot commented Apr 28, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from b7ba298 to blathers/backport-release-23.1-101930: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1 failed. See errors above.


error creating merge commit from b7ba298 to blathers/backport-release-22.2-101930: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

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.

build: github-CI is currently broken due to missing plpgsql build rules
4 participants