types: remove no longer used version gate #77260

yuzefovich · 2022-03-02T05:03:14Z

This is done to break the dependency of sql/types on roachpb (which
is a part of the effort to clean up the dependencies of execgen). This
commit brings the dependencies of execgen to a much more reasonable
set.

Although there is an ask in place to not remove this code even if it's
not used at the moment (so that we have the necessary infra in place
if/when the need arises), in order to keep the functionality we would
need to pull out roachpb.Version out of roachpb, and I don't want
to embark on that since we're in stability period.

Fixes: #77234.

Release note: None

Release justification: low risk change to clean up the dependencies.

cockroach-teamcity · 2022-03-02T05:03:25Z

This change is

yuzefovich · 2022-03-02T05:06:08Z

With this change, we get

bazel query "deps(//pkg/sql/colexec/execgen/cmd/execgen)" --output package | grep -v '^@' | grep -v '^build/bazelutil'

build/toolchains
pkg/build
pkg/cmd/protoc-gen-gogoroach
pkg/col/typeconv
pkg/docs
pkg/geo
pkg/geo/geographiclib
pkg/geo/geopb
pkg/geo/geoprojbase
pkg/geo/geoprojbase/embeddedproj
pkg/keysbase
pkg/sql/colexec/execgen
pkg/sql/colexec/execgen/cmd/execgen
pkg/sql/colexecerror
pkg/sql/inverted
pkg/sql/lex
pkg/sql/oidext
pkg/sql/pgwire/pgcode
pkg/sql/pgwire/pgerror
pkg/sql/sem/tree/treebin
pkg/sql/sem/tree/treecmp
pkg/sql/sem/tree/treewindow
pkg/sql/types
pkg/testutils/lint
pkg/testutils/lint/passes/descriptormarshal
pkg/testutils/lint/passes/errcheck
pkg/testutils/lint/passes/errcmp
pkg/testutils/lint/passes/errwrap
pkg/testutils/lint/passes/fmtsafe
pkg/testutils/lint/passes/forbiddenmethod
pkg/testutils/lint/passes/grpcclientconnclose
pkg/testutils/lint/passes/grpcstatuswithdetails
pkg/testutils/lint/passes/hash
pkg/testutils/lint/passes/leaktestcall
pkg/testutils/lint/passes/nilness
pkg/testutils/lint/passes/nocopy
pkg/testutils/lint/passes/passesutil
pkg/testutils/lint/passes/returnerrcheck
pkg/testutils/lint/passes/staticcheck
pkg/testutils/lint/passes/timer
pkg/testutils/lint/passes/unconvert
pkg/util
pkg/util/arith
pkg/util/bitarray
pkg/util/buildutil
pkg/util/duration
pkg/util/encoding
pkg/util/encoding/encodingtype
pkg/util/envutil
pkg/util/errorutil/unimplemented
pkg/util/humanizeutil
pkg/util/ipaddr
pkg/util/json
pkg/util/log/logpb
pkg/util/netutil/addr
pkg/util/protoutil
pkg/util/randutil
pkg/util/syncutil
pkg/util/timeofday
pkg/util/timetz
pkg/util/timeutil
pkg/util/timeutil/gen
pkg/util/timeutil/pgdate
pkg/util/treeprinter
pkg/util/uint128
pkg/util/unique
pkg/util/uuid
pkg/util/version

which seems good on a quick glance.

At the moment, the ~~third~~last commit breaks the bazel build, and I'm not sure what I didn't do when extracting a part of roachpb/data.proto:

ERROR: /Users/yuzefovich/go/src/github.com/cockroachdb/cockroach/pkg/roachpb/BUILD.bazel:9:11: GoCompilePkg pkg/roachpb/roachpb.a failed: (Exit 1): builder failed: error executing command bazel-out/darwin-opt-exec-2B5CBBC6/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix darwin_amd64 -tags bazel,gss,bazel,gss -src ... (remaining 139 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox
compilepkg: missing strict dependencies:
	/private/var/tmp/_bazel_yahor/e311b1c2454b7c301a6b73bddff450e7/sandbox/darwin-sandbox/928/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/roachpb/roachpb_go_proto_/github.com/cockroachdb/cockroach/pkg/roachpb/api.pb.go: import of "keysbase"
	/private/var/tmp/_bazel_yahor/e311b1c2454b7c301a6b73bddff450e7/sandbox/darwin-sandbox/928/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/roachpb/roachpb_go_proto_/github.com/cockroachdb/cockroach/pkg/roachpb/data.pb.go: import of "keysbase"
	/private/var/tmp/_bazel_yahor/e311b1c2454b7c301a6b73bddff450e7/sandbox/darwin-sandbox/928/execroot/cockroach/bazel-out/darwin-fastbuild/bin/pkg/roachpb/roachpb_go_proto_/github.com/cockroachdb/cockroach/pkg/roachpb/span_config.pb.go: import of "keysbase"

cc @rickystewart @irfansharif @ajwerner do you have some hints?

yuzefovich · 2022-03-02T05:06:47Z

Sorry, the last commit breaks the bazel build, edited above.

rickystewart · 2022-03-02T18:03:25Z

do you have some hints?

I think you need to add "//pkg/keysbase" to the list of deps for roachpb_go_proto in pkg/roachpb/BUILD.bazel? Not 100% sure. I'm going to pull your branch and poke around with it.

rickystewart · 2022-03-02T18:13:00Z

Yeah, that change was necessary but not sufficient. I am able to build cockroach with all of the following changes:

diff --git a/pkg/roachpb/BUILD.bazel b/pkg/roachpb/BUILD.bazel
index 192965a550..76bcf880e7 100644
--- a/pkg/roachpb/BUILD.bazel
+++ b/pkg/roachpb/BUILD.bazel
@@ -177,6 +177,7 @@ go_proto_library(
     proto = ":roachpb_proto",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/keysbase",
         "//pkg/kv/kvserver/concurrency/lock",
         "//pkg/kv/kvserver/readsummary/rspb",
         "//pkg/settings",
diff --git a/pkg/sql/alter_tenant.go b/pkg/sql/alter_tenant.go
index 57d0f85fa7..4fd7d6be4b 100644
--- a/pkg/sql/alter_tenant.go
+++ b/pkg/sql/alter_tenant.go
@@ -14,7 +14,7 @@ import (
 	"context"
 	"strings"
 
-	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/keysbase"
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
@@ -28,7 +28,7 @@ import (
 // ALTER TENANT ... SET CLUSTER SETTING statement.
 type alterTenantSetClusterSettingNode struct {
 	name      string
-	tenantID  roachpb.TenantID
+	tenantID  keysbase.TenantID
 	tenantAll bool
 	st        *cluster.Settings
 	setting   settings.NonMaskedSetting
diff --git a/pkg/sql/parser/BUILD.bazel b/pkg/sql/parser/BUILD.bazel
index 37c4db40da..3d88e76bcc 100644
--- a/pkg/sql/parser/BUILD.bazel
+++ b/pkg/sql/parser/BUILD.bazel
@@ -22,7 +22,7 @@ go_library(
     deps = [
         "//pkg/docs",
         "//pkg/geo/geopb",  # keep
-        "//pkg/roachpb",  # keep
+        "//pkg/keysbase",  # keep
         "//pkg/security",  # keep
         "//pkg/sql/lexbase",
         "//pkg/sql/pgwire/pgcode",
diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y
index 9a3433b213..5f44cc7689 100644
--- a/pkg/sql/parser/sql.y
+++ b/pkg/sql/parser/sql.y
@@ -30,7 +30,7 @@ import (
     "go/constant"
 
     "github.com/cockroachdb/cockroach/pkg/geo/geopb"
-    "github.com/cockroachdb/cockroach/pkg/roachpb"
+    "github.com/cockroachdb/cockroach/pkg/keysbase"
     "github.com/cockroachdb/cockroach/pkg/security"
     "github.com/cockroachdb/cockroach/pkg/sql/lexbase"
     "github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -4967,7 +4967,7 @@ alter_tenant_csetting_stmt:
     $$.val = &tree.AlterTenantSetClusterSetting{
       Name: strings.Join($7.strs(), "."),
       Value: $9.expr(),
-      TenantID: roachpb.MakeTenantID(tenID),
+      TenantID: keysbase.MakeTenantID(tenID),
     }
   }
 | ALTER TENANT ALL SET CLUSTER SETTING var_name to_or_eq var_value
@@ -4987,7 +4987,7 @@ alter_tenant_csetting_stmt:
     $$.val = &tree.AlterTenantSetClusterSetting{
       Name: strings.Join($7.strs(), "."),
       Value: tree.DefaultVal{},
-      TenantID: roachpb.MakeTenantID(tenID),
+      TenantID: keysbase.MakeTenantID(tenID),
     }
   }
 | ALTER TENANT ALL RESET CLUSTER SETTING var_name
@@ -5728,7 +5728,7 @@ show_csettings_stmt:
     if tenID == 0 {
       return setErr(sqllex, errors.New("invalid tenant ID"))
     }
-    $$.val = &tree.ShowClusterSetting{Name: strings.Join($4.strs(), "."), TenantID: roachpb.MakeTenantID(tenID)}
+    $$.val = &tree.ShowClusterSetting{Name: strings.Join($4.strs(), "."), TenantID: keysbase.MakeTenantID(tenID)}
   }
 | SHOW ALL CLUSTER SETTINGS FOR TENANT iconst64
   {
@@ -5736,7 +5736,7 @@ show_csettings_stmt:
     if tenID == 0 {
       return setErr(sqllex, errors.New("invalid tenant ID"))
     }
-    $$.val = &tree.ShowClusterSettingList{All: true, TenantID: roachpb.MakeTenantID(tenID)}
+    $$.val = &tree.ShowClusterSettingList{All: true, TenantID: keysbase.MakeTenantID(tenID)}
   }
 | SHOW CLUSTER SETTINGS FOR TENANT iconst64
   {
@@ -5744,7 +5744,7 @@ show_csettings_stmt:
     if tenID == 0 {
       return setErr(sqllex, errors.New("invalid tenant ID"))
     }
-    $$.val = &tree.ShowClusterSettingList{TenantID: roachpb.MakeTenantID(tenID)}
+    $$.val = &tree.ShowClusterSettingList{TenantID: keysbase.MakeTenantID(tenID)}
   }
 | SHOW PUBLIC CLUSTER SETTINGS FOR TENANT iconst64
   {
@@ -5752,7 +5752,7 @@ show_csettings_stmt:
     if tenID == 0 {
       return setErr(sqllex, errors.New("invalid tenant ID"))
     }
-    $$.val = &tree.ShowClusterSettingList{TenantID: roachpb.MakeTenantID(tenID)}
+    $$.val = &tree.ShowClusterSettingList{TenantID: keysbase.MakeTenantID(tenID)}
   }
 | SHOW PUBLIC CLUSTER error // SHOW HELP: SHOW CLUSTER SETTING
 
@@ -6750,7 +6750,7 @@ targets:
     if tenID == 0 {
       return setErr(sqllex, errors.New("invalid tenant ID"))
     }
-    $$.val = tree.TargetList{Tenant: roachpb.MakeTenantID(tenID)}
+    $$.val = tree.TargetList{Tenant: keysbase.MakeTenantID(tenID)}
   }
 | DATABASE name_list
   {
diff --git a/pkg/sql/sem/tree/alter_tenant.go b/pkg/sql/sem/tree/alter_tenant.go
index 1ec4485d32..c18f011772 100644
--- a/pkg/sql/sem/tree/alter_tenant.go
+++ b/pkg/sql/sem/tree/alter_tenant.go
@@ -13,7 +13,7 @@ package tree
 import (
 	"fmt"
 
-	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/keysbase"
 )
 
 // AlterTenantSetClusterSetting represents an ALTER TENANT
@@ -21,7 +21,7 @@ import (
 type AlterTenantSetClusterSetting struct {
 	Name      string
 	Value     Expr
-	TenantID  roachpb.TenantID
+	TenantID  keysbase.TenantID
 	TenantAll bool
 }
 
diff --git a/pkg/sql/sem/tree/set.go b/pkg/sql/sem/tree/set.go
index b045ba2b5c..bdcf57e77b 100644
--- a/pkg/sql/sem/tree/set.go
+++ b/pkg/sql/sem/tree/set.go
@@ -19,7 +19,7 @@
 
 package tree
 
-import "github.com/cockroachdb/cockroach/pkg/roachpb"
+import "github.com/cockroachdb/cockroach/pkg/keysbase"
 
 // SetVar represents a SET or RESET statement.
 type SetVar struct {
@@ -69,7 +69,7 @@ func (node *SetVar) Format(ctx *FmtCtx) {
 type SetClusterSetting struct {
 	Name      string
 	Value     Expr
-	TenantID  roachpb.TenantID
+	TenantID  keysbase.TenantID
 	TenantAll bool
 }
 
diff --git a/pkg/sql/sem/tree/show.go b/pkg/sql/sem/tree/show.go
index 1832d799df..3079c5b93b 100644
--- a/pkg/sql/sem/tree/show.go
+++ b/pkg/sql/sem/tree/show.go
@@ -22,7 +22,7 @@ package tree
 import (
 	"fmt"
 
-	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/keysbase"
 	"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
 )
 
@@ -44,7 +44,7 @@ func (node *ShowVar) Format(ctx *FmtCtx) {
 // ShowClusterSetting represents a SHOW CLUSTER SETTING statement.
 type ShowClusterSetting struct {
 	Name     string
-	TenantID roachpb.TenantID
+	TenantID keysbase.TenantID
 }
 
 // Format implements the NodeFormatter interface.
@@ -65,7 +65,7 @@ func (node *ShowClusterSetting) Format(ctx *FmtCtx) {
 type ShowClusterSettingList struct {
 	// All indicates whether to include non-public settings in the output.
 	All      bool
-	TenantID roachpb.TenantID
+	TenantID keysbase.TenantID
 }
 
 // Format implements the NodeFormatter interface.

rickystewart · 2022-03-02T18:13:21Z

After you apply that change you may still have to make generate or something to pass CI btw, I haven't checked.

yuzefovich · 2022-03-03T01:28:42Z

Thanks Ricky!

yuzefovich · 2022-03-03T05:40:51Z

In the end, the move of roachpb.TenantID out of roachpb package turned out to be unnecessary - not sure why I thought I needed to do that yesterday. With these four commits the dependency list of execgen is as above. All of the commits seem simple enough, so I think we'll be able to merge them during the stability period.

This PR is in a draft mode for the moment in order to not ping many aliases based on the first commit.

irfansharif

there is an ask in place to not remove this code even if it's
not used at the moment (so that we have the necessary infra in place
if/when the need arises)

Could always resurrect it from git history. Last commit LGTM.

This is done to break the dependency of `sql/types` on `roachpb` (which is a part of the effort to clean up the dependencies of `execgen`). This commit brings the dependencies of `execgen` to a much more reasonable set. Although there is an ask in place to not remove this code even if it's not used at the moment (so that we have the necessary infra in place if/when the need arises), in order to keep the functionality we would need to pull out `roachpb.Version` out of `roachpb`, and I don't want to embark on that since we're in stability period. Release note: None Release justification: low risk change to clean up the dependencies.

yuzefovich · 2022-03-04T05:53:18Z

TFTR!

bors r+

craig · 2022-03-04T07:04:45Z

Build succeeded:

GitHub CI (Cockroach)

yuzefovich force-pushed the execgen branch 3 times, most recently from cf5c96b to 9aed8f9 Compare March 3, 2022 04:05

yuzefovich mentioned this pull request Mar 3, 2022

types: remove no longer used version gate #77320

Closed

yuzefovich force-pushed the execgen branch 2 times, most recently from 552ec70 to 4165013 Compare March 3, 2022 05:37

yuzefovich changed the title ~~WIP cleanup dependencies of execgen~~ types: remove no longer used version gate Mar 3, 2022

yuzefovich force-pushed the execgen branch 3 times, most recently from 742969d to 9b44655 Compare March 3, 2022 15:59

irfansharif approved these changes Mar 3, 2022

View reviewed changes

yuzefovich force-pushed the execgen branch from 9b44655 to f09a36d Compare March 4, 2022 04:41

yuzefovich marked this pull request as ready for review March 4, 2022 04:42

yuzefovich requested a review from a team March 4, 2022 04:42

craig bot merged commit 3160bcf into cockroachdb:master Mar 4, 2022

yuzefovich deleted the execgen branch March 4, 2022 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types: remove no longer used version gate #77260

types: remove no longer used version gate #77260

yuzefovich commented Mar 2, 2022 •

edited

Loading

cockroach-teamcity commented Mar 2, 2022

yuzefovich commented Mar 2, 2022 •

edited

Loading

yuzefovich commented Mar 2, 2022

rickystewart commented Mar 2, 2022

rickystewart commented Mar 2, 2022

rickystewart commented Mar 2, 2022

yuzefovich commented Mar 3, 2022

yuzefovich commented Mar 3, 2022

irfansharif left a comment

yuzefovich commented Mar 4, 2022

craig bot commented Mar 4, 2022

types: remove no longer used version gate #77260

types: remove no longer used version gate #77260

Conversation

yuzefovich commented Mar 2, 2022 • edited Loading

cockroach-teamcity commented Mar 2, 2022

yuzefovich commented Mar 2, 2022 • edited Loading

yuzefovich commented Mar 2, 2022

rickystewart commented Mar 2, 2022

rickystewart commented Mar 2, 2022

rickystewart commented Mar 2, 2022

yuzefovich commented Mar 3, 2022

yuzefovich commented Mar 3, 2022

irfansharif left a comment

Choose a reason for hiding this comment

yuzefovich commented Mar 4, 2022

craig bot commented Mar 4, 2022

yuzefovich commented Mar 2, 2022 •

edited

Loading

yuzefovich commented Mar 2, 2022 •

edited

Loading