Go: change logic in `go-autobuilder --identify-environment` #13040

owen-mc · 2023-05-04T14:48:47Z

Currently our supported range is 1.11-1.20. If the version of Go installed in the environment is outside of that range we decide not to do anything. It would actually be fine to just install the desired version of Go (if that’s supported). I guess this would guard against the case that the image used in Actions gets a new version of Go (1.21, say) before we start supporting it. Or if it’s a really old image, and has Go 1.10 on it.

owen-mc · 2023-05-05T12:47:57Z

The failing integration test is because an old macOS image was specified. It has since been removed.

This does not change the version returned. In the case the the go mod version is supported and the go env version is below goMinVersion, the message now talks about go env version being unsupported instead of it being less than go mod version. This seems more sensible to me.

mbg

Makes sense. One small comment about incorporating some of the reasoning behind the logic we have implemented here in the code so that it doesn't disappear over time.

go/extractor/cli/go-autobuilder/go-autobuilder.go

owen-mc · 2023-05-10T08:56:41Z

I've updated this to deal with the following situation: if the go.mod file says 1.11 and our minimum supported version is 1.12 then previously we were not installing a version of Go, whereas now we are installing version 1.12. Because our current minimum supported version is 1.11 and go.mod files were only supported in version 1.11 this is highly unlikely to happen in practice, but it seemed worth dealing with it now as we won't think of it if we ever raise our minimum supported version.

mbg · 2023-05-10T14:03:57Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+func belowSupportedRange(version string) bool {
+	return semver.Compare(semver.MajorMinor("v"+version), "v"+minGoVersion) < 0
+}


I keep thinking that I really wish Go had something like Haskell's newtype which let you create a new nominal type to distinguish e.g. between version strings that include the "v" and ones that don't.

I don't know Haskell, but perhaps you can create a new type with the same underlying type with type semverVersion string.

I admit that the treatment of version strings in this file is a mess. I've stuck with no prefix in the code I've added, but I haven't changed it elsewhere, which just stores up the problem for later. I wanted to avoid accidentally changing current behaviour.

mbg · 2023-05-10T14:06:42Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+		// We definitely need to install a version. We have no indication which version was
+		// intended to be used to build this project. Go versions are generally backwards
+		// compatible, so we install the maximum supported version.


This comment is the same as for the previous condition, so based on the comments it is not clear how these are different.

I have updated it now. I tried not to repeat information that is already in the msg in the comment, but instead focus on explaining the reasoning behind why we act that way. (No doubt I could have done better.)

mbg · 2023-05-10T14:15:06Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+		msg = "No `go.mod` file found. The version of Go installed in the environment (" +
+			v.goEnvVersion + ") is outside of the supported range (" + minGoVersion + "-" +
+			maxGoVersion + "). Writing an environment file specifying the maximum supported " +
+			"version of Go (" + maxGoVersion + ")."
+		version = maxGoVersion
+		diagnostics.EmitNoGoModAndGoEnvUnsupported(msg)


I was thinking about whether to suggest this during my last review, but I am not convinced that this is the right approach. Consider a self-hosted runner / custom image where a particular version of Go is installed on purpose. The behaviour that is implemented here would override whatever version of Go might be deliberately installed on the system. Perhaps we could do this if we find ourselves in a GHA environment (i.e. GITHUB_ACTIONS is set) but not otherwise?

We try to use the version of Go in the environment if we can. This branch is dealing with the case that the version of Go in the environment is not supported, e.g. Go 1.21 has been released and we haven't added support for it yet.

mbg · 2023-05-10T14:18:31Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+// Assuming `v.goModVersion` is above the supported range, emit a diagnostic and return the
+// version to install, or the empty string if we should not attempt to install a version of Go.


This comment suggests that this function may return a version to install, which it never does.

mbg · 2023-05-10T14:19:32Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+	msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
+		") is above the supported range (" + minGoVersion + "-" + maxGoVersion +
+		"). Writing an environment file not specifying any version of Go."
+	version = ""


Would this lead to automatic failure if no version of Go is installed on the system? Would it be better to at least try to proceed with the latest supported version?

If they have Go 1.21 in their go.mod and we don't support that yet, then the behaviour currently specified by this PR is to not install a version of Go, even if there is no version installed. I think it would be weird if we install version 1.20 and then potentially fall over on some incompatibility, not to mention showing them a diagnostic on the Tools Status Page that they need to install a newer version of Go.

mbg · 2023-05-10T14:20:26Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+// Assuming `v.goModVersion` is above the supported range, emit a diagnostic and return the
+// version to install, or the empty string if we should not attempt to install a version of Go.


The comment here is the same as for getVersionWhenGoModVersionTooHigh but should really read:

Suggested change

// Assuming `v.goModVersion` is above the supported range, emit a diagnostic and return the

// version to install, or the empty string if we should not attempt to install a version of Go.

// Assuming `v.goModVersion` is below the supported range, emit a diagnostic and return the

// version to install, or the empty string if we should not attempt to install a version of Go.

mbg · 2023-05-10T14:20:38Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+// Assuming `v.goModVersion` is above the supported range, emit a diagnostic and return the
+// version to install, or the empty string if we should not attempt to install a version of Go.


The comment here is the same as for getVersionWhenGoModVersionTooHigh but should really read:

Suggested change

// Assuming `v.goModVersion` is above the supported range, emit a diagnostic and return the

// version to install, or the empty string if we should not attempt to install a version of Go.

// Assuming `v.goModVersion` is below the supported range, emit a diagnostic and return the

// version to install, or the empty string if we should not attempt to install a version of Go.

This seems to be the same as the comment above. I'm not sure how that's possible.

smowton · 2023-05-10T14:54:34Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+		// compatible, so we install the maximum supported version.
+		msg = "No `go.mod` file found. The version of Go installed in the environment (" +
+			v.goEnvVersion + ") is outside of the supported range (" + minGoVersion + "-" +
+			maxGoVersion + "). Writing an environment file specifying the maximum supported " +


Since these diagnostics are potentially user-facing, maybe use "Requesting" instead of "Writing an environment file"

smowton · 2023-05-10T14:55:58Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

+		msg = "No version of Go installed and no `go.mod` file found. Writing an environment " +
+			"file specifying the maximum supported version of Go (" + maxGoVersion + ")."
+		version = maxGoVersion
+		diagnostics.EmitNoGoModAndNoGoEnv(msg)


Note we're liable to be running --identify-environment before codeql-init ==> no database ==> nowhere to put diagnostics at this stage. We'd need to work with the action to put diagnostics somewhere, or just emit these as log prints pending such a solution.

Are diagnostics put in the database? I thought they were just written as files somewhere, but I admit I don't know what happens after that.

I now understand what you mean. If the diagnostic directory environment variable is not set, no diagnostics will be emitted, and one log line will be printed stating that. If we want to gather the diagnostics we'll have to set the env var and collect the diagnostic files ourselves in the action. This can be a future task.

Makes sense -- if identify-environment can raise diagnostics, this could be included in its output for the Action to record once that makes sense.

smowton · 2023-05-10T15:47:09Z

go/extractor/cli/go-autobuilder/go-autobuilder.go

@@ -803,18 +871,19 @@ func compareVersions(v versionInfo) (msg, version string) {
 // Check the versions of Go found in the environment and in the `go.mod` file, and return a
 // version to install. If the version is the empty string then no installation is required.


So at the moment this is...

Requested > Installed \/ None Unsupported old Unsupported new Supported

None Install max supported Install min supported No action (!) Install requested

Unsupported old Install max supported Install min supported No action (!) Install requested

Unsupported new Install max supported Install min supported No action Install requested

Supported No action No action No action (!!) Install requested if newer than installed

Suggest the cells highlighted with (!) should be install max supported, in keeping with our practice elsewhere of not intentionally upgrading to a too-new Go, but responding constructively to a too-old or missing environmental Go.

Suggest the cell highlighted with (!!) should be install max supported if newer than installed, since it would be odd to rule that have go 1.17, request go 1.20 ==> upgrade to 1.20; have go 1.17, request go 1.21 ==> try with go 1.17.

You and @mbg seem to agree on this, so I'll make the change

owen-mc requested a review from a team as a code owner May 4, 2023 14:48

github-actions bot added the Go label May 4, 2023

owen-mc added 2 commits May 5, 2023 15:44

Sometimes install Go version even when one exists

3f7a230

owen-mc force-pushed the go/identify-environment-change-logic branch from a60b88e to d329da6 Compare May 5, 2023 14:45

mbg previously approved these changes May 5, 2023

View reviewed changes

go/extractor/cli/go-autobuilder/go-autobuilder.go Outdated Show resolved Hide resolved

owen-mc dismissed mbg’s stale review via ab1c627 May 6, 2023 07:23

mbg previously approved these changes May 9, 2023

View reviewed changes

owen-mc added 2 commits May 10, 2023 09:26

Add comments explaining version choice logic

32b5df6

Deal better with goModVersion < minGoVersion

12f996f

owen-mc dismissed mbg’s stale review via f86451e May 10, 2023 08:54

owen-mc force-pushed the go/identify-environment-change-logic branch from ab1c627 to f86451e Compare May 10, 2023 08:54

owen-mc force-pushed the go/identify-environment-change-logic branch from f86451e to c09f616 Compare May 10, 2023 08:57

owen-mc added 2 commits May 10, 2023 11:12

Refactor for clarity

edebebf

Fix diagnostics

375be68

owen-mc force-pushed the go/identify-environment-change-logic branch from c09f616 to 375be68 Compare May 10, 2023 10:12

mbg reviewed May 10, 2023

View reviewed changes

Address review comments

97ec7a0

smowton reviewed May 10, 2023

View reviewed changes

owen-mc added 2 commits May 10, 2023 21:53

Use "Requesting" instead of "Writing environment file"

9c5fc97

Change logic when go mod version above max supported version

9334cfb

smowton approved these changes May 10, 2023

View reviewed changes

owen-mc merged commit b306807 into github:main May 11, 2023

owen-mc deleted the go/identify-environment-change-logic branch May 11, 2023 07:22

		// Assuming `v.goModVersion` is above the supported range, emit a diagnostic and return the
		// version to install, or the empty string if we should not attempt to install a version of Go.

		@@ -803,18 +871,19 @@ func compareVersions(v versionInfo) (msg, version string) {
		// Check the versions of Go found in the environment and in the `go.mod` file, and return a
		// version to install. If the version is the empty string then no installation is required.

Requested > Installed \/	None	Unsupported old	Unsupported new	Supported
None	Install max supported	Install min supported	No action (!)	Install requested
Unsupported old	Install max supported	Install min supported	No action (!)	Install requested
Unsupported new	Install max supported	Install min supported	No action	Install requested
Supported	No action	No action	No action (!!)	Install requested if newer than installed

Go: change logic in go-autobuilder --identify-environment #13040

Go: change logic in go-autobuilder --identify-environment #13040

Uh oh!

Conversation

owen-mc commented May 4, 2023

Uh oh!

owen-mc commented May 5, 2023

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

owen-mc commented May 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Go: change logic in `go-autobuilder --identify-environment` #13040

Go: change logic in `go-autobuilder --identify-environment` #13040