release-24.1: skip,metamorphic: break dependency on metamorphic from skip

pkg/BUILD.bazel

@@ -642,6 +642,7 @@ ALL_TESTS = [
     "//pkg/testutils/listenerutil:listenerutil_test",
     "//pkg/testutils/release:release_test",
     "//pkg/testutils/serverutils:serverutils_test",
+    "//pkg/testutils/skip:skip_disallowed_imports_test",
     "//pkg/testutils/sqlutils:sqlutils_test",
     "//pkg/testutils/testcluster:testcluster_test",
     "//pkg/testutils/zerofields:zerofields_test",
@@ -2464,6 +2465,7 @@ GO_TARGETS = [
     "//pkg/util/log:log",
     "//pkg/util/log:log_test",
     "//pkg/util/memzipper:memzipper",
+    "//pkg/util/metamorphic/metamorphicutil:metamorphicutil",
     "//pkg/util/metamorphic:metamorphic",
     "//pkg/util/metamorphic:metamorphic_test",
     "//pkg/util/metric/aggmetric:aggmetric",

pkg/testutils/skip/BUILD.bazel

@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("//pkg/testutils:buildutil/buildutil.bzl", "disallowed_imports_test")
 
 go_library(
     name = "skip",
@@ -14,7 +15,15 @@ go_library(
         "//pkg/util",
         "//pkg/util/buildutil",
         "//pkg/util/envutil",
-        "//pkg/util/metamorphic",
+        "//pkg/util/metamorphic/metamorphicutil",
         "//pkg/util/syncutil",
     ],
 )
+
+disallowed_imports_test(
+    "skip",
+    disallow_cdeps = True,
+    disallowed_list = [
+        "//pkg/util/metamorphic",
+    ],
+)

pkg/testutils/skip/skip.go

@@ -21,7 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
-	"github.com/cockroachdb/cockroach/pkg/util/metamorphic"
+	"github.com/cockroachdb/cockroach/pkg/util/metamorphic/metamorphicutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
 
@@ -160,7 +160,7 @@ func UnderStressRaceWithIssue(t SkippableTest, githubIssueID int, args ...interf
 // run with the metamorphic build tag.
 func UnderMetamorphic(t SkippableTest, args ...interface{}) {
 	t.Helper()
-	if metamorphic.IsMetamorphicBuild() {
+	if metamorphicutil.IsMetamorphicBuild {
 		maybeSkip(t, "disabled under metamorphic", args...)
 	}
 }
@@ -170,7 +170,7 @@ func UnderMetamorphic(t SkippableTest, args ...interface{}) {
 // reason.
 func UnderMetamorphicWithIssue(t SkippableTest, githubIssueID int, args ...interface{}) {
 	t.Helper()
-	if metamorphic.IsMetamorphicBuild() {
+	if metamorphicutil.IsMetamorphicBuild {
 		maybeSkip(t, withIssue("disabled under metamorphic", githubIssueID), args...)
 	}
 }

pkg/util/metamorphic/BUILD.bazel

@@ -13,6 +13,7 @@ go_library(
         "//pkg/build/bazel",
         "//pkg/util/buildutil",
         "//pkg/util/envutil",
+        "//pkg/util/metamorphic/metamorphicutil",
         "//pkg/util/randutil",
         "//pkg/util/syncutil",
     ],

pkg/util/metamorphic/constants.go

@@ -17,31 +17,30 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/build/bazel"
 	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
+	"github.com/cockroachdb/cockroach/pkg/util/metamorphic/metamorphicutil"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
 
 // IsMetamorphicBuild returns whether this build is metamorphic. By build being
 // "metamorphic" we mean that some magic constants in the codebase might get
 // initialized to non-default value.
-// A build will become metamorphic with metamorphicBuildProbability probability
+// A build will become metamorphic with metamorphicutil.IsMetamorphicBuildProbability probability
 // if 'crdb_test' build flag is specified (this is the case for all test
 // targets).
 func IsMetamorphicBuild() bool {
-	return metamorphicBuild
+	return metamorphicutil.IsMetamorphicBuild
 }
 
-var metamorphicBuild bool
-
 const (
-	metamorphicBuildProbability = 0.8
-	metamorphicValueProbability = 0.75
-	metamorphicBoolProbability  = 0.5
+	IsMetamorphicBuildProbability = 0.8
+	metamorphicValueProbability   = 0.75
+	metamorphicBoolProbability    = 0.5
 )
 
 // ConstantWithTestValue should be used to initialize "magic constants" that
 // should be varied during test scenarios to check for bugs at boundary
-// conditions. When metamorphicBuild is true, the test value will be used with
+// conditions. When metamorphicutil.IsMetamorphicBuild is true, the test value will be used with
 // metamorphicValueProbability probability. In all other cases, the production
 // ("default") value will be used.
 // The constant must be a "metamorphic variable": changing it cannot affect the
@@ -67,7 +66,7 @@ const (
 //
 // The given name is used for logging.
 func ConstantWithTestValue(name string, defaultValue, metamorphicValue int) int {
-	if metamorphicBuild {
+	if metamorphicutil.IsMetamorphicBuild {
 		rng.Lock()
 		defer rng.Unlock()
 		if rng.r.Float64() < metamorphicValueProbability {
@@ -110,7 +109,7 @@ func init() {
 	if metamorphicEligible() {
 		if !disableMetamorphicTesting {
 			rng.r, _ = randutil.NewTestRand()
-			metamorphicBuild = rng.r.Float64() < metamorphicBuildProbability
+			metamorphicutil.IsMetamorphicBuild = rng.r.Float64() < IsMetamorphicBuildProbability
 		}
 	}
 }
@@ -121,7 +120,7 @@ func init() {
 //
 // The given name is used for logging.
 func ConstantWithTestRange(name string, defaultValue, min, max int) int {
-	if metamorphicBuild {
+	if metamorphicutil.IsMetamorphicBuild {
 		rng.Lock()
 		defer rng.Unlock()
 		if rng.r.Float64() < metamorphicValueProbability {
@@ -145,7 +144,7 @@ func ConstantWithTestBool(name string, defaultValue bool) bool {
 }
 
 func constantWithTestBoolInternal(name string, defaultValue bool, doLog bool) bool {
-	if metamorphicBuild {
+	if metamorphicutil.IsMetamorphicBuild {
 		rng.Lock()
 		defer rng.Unlock()
 		if rng.r.Float64() < metamorphicBoolProbability {
@@ -175,7 +174,7 @@ func ConstantWithTestBoolWithoutLogging(name string, defaultValue bool) bool {
 func ConstantWithTestChoice(
 	name string, defaultValue interface{}, otherValues ...interface{},
 ) interface{} {
-	if metamorphicBuild {
+	if metamorphicutil.IsMetamorphicBuild {
 		values := append([]interface{}{defaultValue}, otherValues...)
 		rng.Lock()
 		defer rng.Unlock()

pkg/util/metamorphic/metamorphicutil/BUILD.bazel

@@ -0,0 +1,11 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "metamorphicutil",
+    srcs = ["is_metamorphic.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/util/metamorphic/metamorphicutil",
+    visibility = [
+        "//pkg/testutils/skip:__pkg__",
+        "//pkg/util/metamorphic:__pkg__",
+    ],
+)

pkg/util/metamorphic/metamorphicutil/is_metamorphic.go

@@ -0,0 +1,23 @@
+// Copyright 2024 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package metamorphicutil
+
+// NB: init() in pkg/util/metamorphic/constants.go may set this value to true.
+// We don't put this variable in that package as we would like to have a way to
+// reliably determine which packages use metamorphic constants, and
+// `bazel query somepath(_, //pkg/util/metamorphic)` should be a good way to do
+// that. However, some packages (like pkg/testutils/skip) need to check whether
+// we're running a metamorphic build, without necessarily depending on the
+// metamorphic package.
+//
+// Generally, you should use metamorphic.IsMetaMorphicBuild() instead of checking
+// this value.
+var IsMetamorphicBuild bool