bazel: generate `//go:generate stringer` files sandbox #59146

alan-mas · 2021-01-19T17:09:44Z

First part of #57787 work.

As gazelle is not taking care by itself in any changes related to //go:generate stringer we are creating a
genrule to handle it.

From all the //go:generate stringer files, there are some that are having troubles during bazel build:

-pkg/util/encoding/encoding.go
-pkg/util/encoding/BUILD.bazel
"stringer: can't handle non-integer constant type Type"

-pkg/workload/schemachange/schemachange.go
-pkg/workload/schemachange/BUILD.bazel
"stringer: can't happen: constant is not an integer TxStatusInFailure"

Release note: None

blathers-crl · 2021-01-19T17:09:46Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2021-01-19T17:09:55Z

This change is

otan

Thanks, looks like we're on the right path.

Please replace gazzelle with gazelle in the commit message.

Also we are repeating the same code fairly often. We can simplify this into a function in, e.g. STRINGER.bzl, similar to how we do colexec:

def stringer(file, typ):
   native.genrule(
      name = "gen-" + file, # maybe replace `file` with `-` if bazel doesn't like that.
      srcs = [
          file,
      ],
      outs = [basename(file) + "_string.go"],
      cmd = """
         env PATH=`dirname $(location @go_sdk//:bin/go)` HOME=$(GENDIR) \
         $(location @org_golang_x_tools//cmd/stringer:stringer) -output=$@ - 
  type={} $<
      """.format(typ),
      tools = [
         "@go_sdk//:bin/go",
          "@org_golang_x_tools//cmd/stringer",
       ],
   )

otan · 2021-01-19T19:09:11Z

pkg/util/encoding/BUILD.bazel

@@ -58,3 +59,19 @@ go_test(
        "@com_github_stretchr_testify//require",
    ],
 )
+
+# genrule(


what happened here?

It is showing this:
"stringer: can't handle non-integer constant type Type"

You'll need to include pkg/util/encoding/encodingtype somehow, similar to what we do with x-package stringers here:

cockroach/pkg/sql/opt/BUILD.bazel

Line 109 in 731d083

$(location @org_golang_x_tools//cmd/stringer:stringer) -output=$@ \

Code is almost the same, I am using $(location @org_golang_x_tools//cmd/stringer:stringer) -output=$@ inside genrule I might be confused there but what do you mean with "include pkg/util/encoding/encodingtype somehow"

The only difference I can see is that you are manually copy one of the source files over into the location of the other. So you mean something like copy pkg/util/encoding/encodingtype inside the same location in which we are running?

the reason you are seeing this error is because Type is defined as

// Type represents the type of a value encoded by // Encode{Null,NotNull,Varint,Uvarint,Float,Bytes}. //go:generate stringer -type=Type type Type encodingtype.T

which relies on

"github.com/cockroachdb/cockroach/pkg/util/encoding/encodingtype"

So yes, we'd need to copy the source files over as otherwise bazel's only source is encoding.go and has no way of reading encodingtype.

alan-mas · 2021-01-19T19:20:34Z

Thanks, looks like we're on the right path.

Please replace gazzelle with gazelle in the commit message.

Also we are repeating the same code fairly often. We can simplify this into a function in, e.g. STRINGER.bzl, similar to how we do colexec:
def stringer(file, typ):
   native.genrule(
      name = "gen-" + file, # maybe replace `file` with `-` if bazel doesn't like that.
      srcs = [
          file,
      ],
      outs = [basename(file) + "_string.go"],
      cmd = """
         env PATH=`dirname $(location @go_sdk//:bin/go)` HOME=$(GENDIR) \
         $(location @org_golang_x_tools//cmd/stringer:stringer) -output=$@ - 
  type={} $<
      """.format(typ),
      tools = [
         "@go_sdk//:bin/go",
          "@org_golang_x_tools//cmd/stringer",
       ],
   )

This is a good idea! let me give a shot on it :)

Just did it adding pkg/STRINGER.bzlbazel file to reuse the code across project :)

blathers-crl · 2021-01-20T00:20:20Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

rickystewart

Looks pretty good after we address the comments. :)

rickystewart · 2021-01-22T18:04:10Z

pkg/base/BUILD.bazel

@@ -64,3 +65,9 @@ go_test(
        "@com_github_stretchr_testify//require",
    ],
 )
+
+# Using common function for stringer to create testclusterreplicationmode_string.go


I don't think this comment is useful, just delete it at this point.

I used it as there is no "real" clue on which will be the output (only that you go and check the function) but yes, you are right I can delete it in all of the files :)

rickystewart · 2021-01-22T18:06:03Z

pkg/base/BUILD.bazel

@@ -64,3 +65,9 @@ go_test(
        "@com_github_stretchr_testify//require",
    ],
 )
+
+# Using common function for stringer to create testclusterreplicationmode_string.go
+stringer(


Hmm. I don't like that it's unclear what the name of this rule is. Can you add a name argument to the stringer() helper function? Then the call sites would have to be updated as well.

Basically I'm worried that in the future someone will CTRL-F for gen-testclusterreplicationmode-stringer and not be able to find it, which is confusing. Also the name gen-testclusterreplicationmode-stringer itself is long and silly, you could probably pick a better one by hand.

I am using that name gen-testclusterreplicationmode-stringer using this logic gen + name of the file + stringer.
that name I think is the longest (so far) but the others are like: Type, Key, etc.

And for the name in argument I can add it, but not sure if that change will do any major change (as this is the file that has the longest name) but I do agree with you in the part of CTRL-F example, so let me add name over there to avoid this kind of issues

First part of cockroachdb#57787 work. As `gazelle` is not taking care by itself in any changes related to `//go:generate stringer` we are creating a `genrule` to handle it. From all the `//go:generate stringer` files, there are some that are having troubles during bazel build: ``` -pkg/util/encoding/encoding.go -pkg/util/encoding/BUILD.bazel "stringer: can't handle non-integer constant type Type" -pkg/workload/schemachange/schemachange.go -pkg/workload/schemachange/BUILD.bazel "stringer: can't happen: constant is not an integer TxStatusInFailure" ``` I already added some TODO task over there to keep them in track Release note: None

rickystewart · 2021-01-25T16:29:01Z

bors r+

craig · 2021-01-25T17:58:12Z

Build succeeded:

GitHub CI (Cockroach)

alan-mas requested review from a team as code owners January 19, 2021 17:09

blathers-crl bot added the O-community Originated from the community label Jan 19, 2021

alan-mas requested review from otan and removed request for a team January 19, 2021 17:21

otan reviewed Jan 19, 2021

View reviewed changes

alan-mas force-pushed the 57787-go-generate-stringer branch from 77b553e to e614ec0 Compare January 20, 2021 00:20

blathers-crl bot requested a review from otan January 20, 2021 00:20

alan-mas force-pushed the 57787-go-generate-stringer branch 5 times, most recently from 8111818 to f62b249 Compare January 21, 2021 22:08

alan-mas requested a review from rickystewart January 21, 2021 22:09

alan-mas force-pushed the 57787-go-generate-stringer branch 2 times, most recently from 7af4aeb to e144f3e Compare January 22, 2021 00:03

alan-mas changed the title *WIP* bazel: generate //go:generate stringer files sandbox bazel: generate //go:generate stringer files sandbox Jan 22, 2021

rickystewart requested changes Jan 22, 2021

View reviewed changes

alan-mas force-pushed the 57787-go-generate-stringer branch from e144f3e to bc52401 Compare January 22, 2021 19:51

rickystewart approved these changes Jan 22, 2021

View reviewed changes

cockroachdb deleted a comment from craig bot Jan 23, 2021

alan-mas force-pushed the 57787-go-generate-stringer branch from bc52401 to ef93819 Compare January 25, 2021 15:41

craig bot merged commit bf439cd into cockroachdb:master Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bazel: generate `//go:generate stringer` files sandbox #59146

bazel: generate `//go:generate stringer` files sandbox #59146

alan-mas commented Jan 19, 2021 •

edited

Loading

blathers-crl bot commented Jan 19, 2021

cockroach-teamcity commented Jan 19, 2021

otan left a comment

otan Jan 19, 2021

alan-mas Jan 19, 2021

otan Jan 19, 2021

alan-mas Jan 19, 2021

otan Jan 19, 2021

alan-mas commented Jan 19, 2021 •

edited

Loading

blathers-crl bot commented Jan 20, 2021

rickystewart left a comment

rickystewart Jan 22, 2021

alan-mas Jan 22, 2021

rickystewart Jan 22, 2021

alan-mas Jan 22, 2021 •

edited

Loading

rickystewart commented Jan 25, 2021

craig bot commented Jan 25, 2021

bazel: generate //go:generate stringer files sandbox #59146

bazel: generate //go:generate stringer files sandbox #59146

Conversation

alan-mas commented Jan 19, 2021 • edited Loading

blathers-crl bot commented Jan 19, 2021

cockroach-teamcity commented Jan 19, 2021

otan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-mas commented Jan 19, 2021 • edited Loading

blathers-crl bot commented Jan 20, 2021

rickystewart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-mas Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

rickystewart commented Jan 25, 2021

craig bot commented Jan 25, 2021

bazel: generate `//go:generate stringer` files sandbox #59146

bazel: generate `//go:generate stringer` files sandbox #59146

alan-mas commented Jan 19, 2021 •

edited

Loading

alan-mas commented Jan 19, 2021 •

edited

Loading

alan-mas Jan 22, 2021 •

edited

Loading