logictest: avoid a lint failure

[knz]

Fixes #92592

The go compiler treats calls to bazel.BuiltWithBazel() as a
compile-time constant. Therefore,

if !bazel.BuiltWithBazel() {
   skip.XXX()
}

is considered to always terminate execution (because skip does its
job by raising a panic), and any code coming after that is treated as
dead/unreachable.

[cockroach-teamcity]

This change is 

[andreimatei]

but please say more in the commit message. Say that builtWithBazel is a compile-time constant; it took me a second to make sense of what's going on here.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @rickystewart)

---

-- commits line 4 at r2:
@rafiss , any chance we can make this cockroach-go stuff work with make too? I don't see any discussion about it in #91881; it seems like a dubious precedent to me. It causes this kind of nonsense.

---

pkg/sql/logictest/logic.go line 1809 at r1 (raw file):

	// Prevent a lint failure "this function is unused" when
	// bazel.BuiltWithBazel returns false below.
	_ = t.newTestServerCluster

I think this should go next to the newTestServerCluster definition, and say that the function is only used in bazel builds.

---

pkg/sql/logictest/logic.go line 1831 at r2 (raw file):

		}

		// Prevent a lint failure "this function is unused" when

The right message is this value is never used. Consider writing that, as it provides more of a hint to what the linter is complaining about.

---

pkg/sql/logictest/logic.go line 1833 at r2 (raw file):

		// Prevent a lint failure "this function is unused" when
		// bazel.BuiltWithBazel returns false above.
		_ = bootstrapBinaryPath

So the linter detects that the read below is unreachable, but does not detect that the write is also unreachable? Grr

[knz]

please say more in the commit message

Done

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @rickystewart)

---

pkg/sql/logictest/logic.go line 1809 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think this should go next to the newTestServerCluster definition, and say that the function is only used in bazel builds.

We can't - it's a method. We can only "access" it after there's an object it's bound to.

(It would be different if we had unit tests for this feature, but we don't)

---

pkg/sql/logictest/logic.go line 1831 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

The right message is this value is never used. Consider writing that, as it provides more of a hint to what the linter is complaining about.

Done.

---

pkg/sql/logictest/logic.go line 1833 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

So the linter detects that the read below is unreachable, but does not detect that the write is also unreachable? Grr

😭

[knz]

bors r=andreimatei

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz and @rickystewart)

---

pkg/sql/logictest/logic.go line 1809 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

We can't - it's a method. We can only "access" it after there's an object it's bound to.

(It would be different if we had unit tests for this feature, but we don't)

sure you can:

var _ = (&logicTest{}).newTestServerCluster

[knz]

bors r-

[craig]

Canceled.

[knz]

bors r=andreimatei

[craig]

Canceled.

[knz]

bors r=andreimatei

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)