Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 11, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3f7a230
Sometimes install Go version even when one exists
owen-mc May 4, 2023
d329da6
Refactor logic for which version to install
owen-mc May 4, 2023
32b5df6
Add comments explaining version choice logic
owen-mc May 6, 2023
12f996f
Deal better with goModVersion < minGoVersion
owen-mc May 7, 2023
edebebf
Refactor for clarity
owen-mc May 10, 2023
375be68
Fix diagnostics
owen-mc May 10, 2023
97ec7a0
Address review comments
owen-mc May 10, 2023
9c5fc97
Use "Requesting" instead of "Writing environment file"
owen-mc May 10, 2023
9334cfb
Change logic when go mod version above max supported version
owen-mc May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
223 changes: 167 additions & 56 deletions go/extractor/cli/go-autobuilder/go-autobuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,82 +719,192 @@ func installDependenciesAndBuild() {
const minGoVersion = "1.11"
const maxGoVersion = "1.20"

// Check if `version` is lower than `minGoVersion`. Note that for this comparison we ignore the
// patch part of the version, so 1.20.1 and 1.20 are considered equal.
func belowSupportedRange(version string) bool {
return semver.Compare(semver.MajorMinor("v"+version), "v"+minGoVersion) < 0
}
Comment on lines +724 to +726
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Check if `version` is higher than `maxGoVersion`. Note that for this comparison we ignore the
// patch part of the version, so 1.20.1 and 1.20 are considered equal.
func aboveSupportedRange(version string) bool {
return semver.Compare(semver.MajorMinor("v"+version), "v"+maxGoVersion) > 0
}

// Check if `version` is lower than `minGoVersion` or higher than `maxGoVersion`. Note that for
// this comparison we ignore the patch part of the version, so 1.20.1 and 1.20 are considered
// equal.
func outsideSupportedRange(version string) bool {
short := semver.MajorMinor("v" + version)
return semver.Compare(short, "v"+minGoVersion) < 0 || semver.Compare(short, "v"+maxGoVersion) > 0
return belowSupportedRange(version) || aboveSupportedRange(version)
}

// Check if `v.goModVersion` or `v.goEnvVersion` are outside of the supported range. If so, emit
// a diagnostic and return an empty version to indicate that we should not attempt to install a
// different version of Go.
func checkForUnsupportedVersions(v versionInfo) (msg, version string) {
if v.goModVersionFound && outsideSupportedRange(v.goModVersion) {
msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
") is outside of the supported range (" + minGoVersion + "-" + maxGoVersion +
"). Writing an environment file not specifying any version of Go."
version = ""
diagnostics.EmitUnsupportedVersionGoMod(msg)
} else if v.goEnvVersionFound && outsideSupportedRange(v.goEnvVersion) {
msg = "The version of Go installed in the environment (" + v.goEnvVersion +
") is outside of the supported range (" + minGoVersion + "-" + maxGoVersion +
"). Writing an environment file not specifying any version of Go."
// Assuming `v.goModVersionFound` is false, emit a diagnostic and return the version to install,
// or the empty string if we should not attempt to install a version of Go.
func getVersionWhenGoModVersionNotFound(v versionInfo) (msg, version string) {
if !v.goEnvVersionFound {
// There is no Go version installed in the environment. 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.
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if outsideSupportedRange(v.goEnvVersion) {
// The Go version installed in the environment is not supported. 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.
msg = "No `go.mod` file found. The version of Go installed in the environment (" +
v.goEnvVersion + ") is outside of the supported range (" + minGoVersion + "-" +
maxGoVersion + "). Requesting the maximum supported version of Go (" + maxGoVersion +
")."
version = maxGoVersion
diagnostics.EmitNoGoModAndGoEnvUnsupported(msg)
} else {
// The version of Go that is installed is supported. We have no indication which version
// was intended to be used to build this project. We assume that the installed version is
// suitable and do not install a version of Go.
msg = "No `go.mod` file found. Version " + v.goEnvVersion + " installed in the " +
"environment is supported. Not requesting any version of Go."
version = ""
diagnostics.EmitUnsupportedVersionEnvironment(msg)
diagnostics.EmitNoGoModAndGoEnvSupported(msg)
}

return msg, version
}

// Check if either `v.goEnvVersionFound` or `v.goModVersionFound` are false. If so, emit
// a diagnostic and return the version to install, or the empty string if we should not attempt to
// install a version of Go. We assume that `checkForUnsupportedVersions` has already been
// called, so any versions that are found are within the supported range.
func checkForVersionsNotFound(v versionInfo) (msg, version string) {
if !v.goEnvVersionFound && !v.goModVersionFound {
msg = "No version of Go installed and no `go.mod` file found. Writing an environment " +
"file specifying the maximum supported version of Go (" + maxGoVersion + ")."
// 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.
Comment on lines +775 to +776
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

func getVersionWhenGoModVersionTooHigh(v versionInfo) (msg, version string) {
if !v.goEnvVersionFound {
// The version in the `go.mod` file is above the supported range. There is no Go version
// installed. We install the maximum supported version as a best effort.
msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
") is above the supported range (" + minGoVersion + "-" + maxGoVersion +
"). No version of Go installed. Requesting the maximum supported version of Go (" +
maxGoVersion + ")."
version = maxGoVersion
diagnostics.EmitNoGoModAndNoGoEnv(msg)
diagnostics.EmitGoModVersionTooHighAndNoGoEnv(msg)
} else if aboveSupportedRange(v.goEnvVersion) {
// The version in the `go.mod` file is above the supported range. The version of Go that
// is installed is above the supported range. We do not install a version of Go.
msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
") is above the supported range (" + minGoVersion + "-" + maxGoVersion +
"). The version of Go installed in the environment (" + v.goEnvVersion +
") is above the supported range (" + minGoVersion + "-" + maxGoVersion +
"). Not requesting any version of Go."
version = ""
diagnostics.EmitGoModVersionTooHighAndEnvVersionTooHigh(msg)
} else if belowSupportedRange(v.goEnvVersion) {
// The version in the `go.mod` file is above the supported range. The version of Go that
// is installed is below the supported range. We install the maximum supported version as
// a best effort.
msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
") is above the supported range (" + minGoVersion + "-" + maxGoVersion +
"). The version of Go installed in the environment (" + v.goEnvVersion +
") is below the supported range (" + minGoVersion + "-" + maxGoVersion +
"). Requesting the maximum supported version of Go (" + maxGoVersion + ")."
version = maxGoVersion
diagnostics.EmitGoModVersionTooHighAndEnvVersionTooLow(msg)
} else if semver.Compare("v"+maxGoVersion, "v"+v.goEnvVersion) > 0 {
// The version in the `go.mod` file is above the supported range. The version of Go that
// is installed is supported and below the maximum supported version. We install the
// maximum supported version as a best effort.
msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
") is above the supported range (" + minGoVersion + "-" + maxGoVersion +
"). The version of Go installed in the environment (" + v.goEnvVersion +
") is below the maximum supported version (" + maxGoVersion +
"). Requesting the maximum supported version of Go (" + maxGoVersion + ")."
version = maxGoVersion
diagnostics.EmitGoModVersionTooHighAndEnvVersionBelowMax(msg)
} else {
// The version in the `go.mod` file is above the supported range. The version of Go that
// is installed is the maximum supported version. We do not install a version of Go.
msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
") is above the supported range (" + minGoVersion + "-" + maxGoVersion +
"). The version of Go installed in the environment (" + v.goEnvVersion +
") is the maximum supported version (" + maxGoVersion +
"). Not requesting any version of Go."
version = ""
diagnostics.EmitGoModVersionTooHighAndEnvVersionMax(msg)
}

if !v.goEnvVersionFound && v.goModVersionFound {
msg = "No version of Go installed. Writing an environment file specifying the version " +
"of Go found in the `go.mod` file (" + v.goModVersion + ")."
version = v.goModVersion
diagnostics.EmitNoGoEnv(msg)
}
return msg, version
}

if v.goEnvVersionFound && !v.goModVersionFound {
msg = "No `go.mod` file found. Version " + v.goEnvVersion + " installed in the " +
"environment. Writing an environment file not specifying any 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.
func getVersionWhenGoModVersionTooLow(v versionInfo) (msg, version string) {
if !v.goEnvVersionFound {
// There is no Go version installed. The version in the `go.mod` file is below the
// supported range. Go versions are generally backwards compatible, so we install the
// minimum supported version.
msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
") is below the supported range (" + minGoVersion + "-" + maxGoVersion +
"). No version of Go installed. Requesting the minimum supported version of Go (" +
minGoVersion + ")."
version = minGoVersion
diagnostics.EmitGoModVersionTooLowAndNoGoEnv(msg)
} else if outsideSupportedRange(v.goEnvVersion) {
// The version of Go that is installed is outside of the supported range. The version
// in the `go.mod` file is below the supported range. Go versions are generally
// backwards compatible, so we install the minimum supported version.
msg = "The version of Go found in the `go.mod` file (" + v.goModVersion +
") is below the supported range (" + minGoVersion + "-" + maxGoVersion +
"). The version of Go installed in the environment (" + v.goEnvVersion +
") is outside of the supported range (" + minGoVersion + "-" + maxGoVersion + "). " +
"Requesting the minimum supported version of Go (" + minGoVersion + ")."
version = minGoVersion
diagnostics.EmitGoModVersionTooLowAndEnvVersionUnsupported(msg)
} else {
// The version of Go that is installed is supported. The version in the `go.mod` file is
// below the supported range. We do not install a version of Go.
msg = "The version of Go installed in the environment (" + v.goEnvVersion +
") is supported and is high enough for the version found in the `go.mod` file (" +
v.goModVersion + "). Not requesting any version of Go."
version = ""
diagnostics.EmitNoGoMod(msg)
diagnostics.EmitGoModVersionTooLowAndEnvVersionSupported(msg)
}

return msg, version
}

// Compare `v.goModVersion` and `v.goEnvVersion`. emit a diagnostic and return the version to
// install, or the empty string if we should not attempt to install a version of Go. We assume that
// `checkForUnsupportedVersions` and `checkForVersionsNotFound` have already been called, so both
// versions are found and are within the supported range.
func compareVersions(v versionInfo) (msg, version string) {
if semver.Compare("v"+v.goModVersion, "v"+v.goEnvVersion) > 0 {
// Assuming `v.goModVersion` is in 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.
func getVersionWhenGoModVersionSupported(v versionInfo) (msg, version string) {
if !v.goEnvVersionFound {
// There is no Go version installed. The version in the `go.mod` file is supported.
// We install the version from the `go.mod` file.
msg = "No version of Go installed. Requesting the version of Go found in the `go.mod` " +
"file (" + v.goModVersion + ")."
version = v.goModVersion
diagnostics.EmitGoModVersionSupportedAndNoGoEnv(msg)
} else if outsideSupportedRange(v.goEnvVersion) {
// The version of Go that is installed is outside of the supported range. The version in
// the `go.mod` file is supported. We install the version from the `go.mod` file.
msg = "The version of Go installed in the environment (" + v.goEnvVersion +
") is outside of the supported range (" + minGoVersion + "-" + maxGoVersion + "). " +
"Requesting the version of Go from the `go.mod` file (" +
v.goModVersion + ")."
version = v.goModVersion
diagnostics.EmitGoModVersionSupportedAndGoEnvUnsupported(msg)
} else if semver.Compare("v"+v.goModVersion, "v"+v.goEnvVersion) > 0 {
// The version of Go that is installed is supported. The version in the `go.mod` file is
// supported and is higher than the version that is installed. We install the version from
// the `go.mod` file.
msg = "The version of Go installed in the environment (" + v.goEnvVersion +
") is lower than the version found in the `go.mod` file (" + v.goModVersion +
"). Writing an environment file specifying the version of Go from the `go.mod` " +
"file (" + v.goModVersion + ")."
"). Requesting the version of Go from the `go.mod` file (" + v.goModVersion + ")."
version = v.goModVersion
diagnostics.EmitVersionGoModHigherVersionEnvironment(msg)
diagnostics.EmitGoModVersionSupportedHigherGoEnv(msg)
} else {
// The version of Go that is installed is supported. The version in the `go.mod` file is
// supported and is lower than or equal to the version that is installed. We do not install
// a version of Go.
msg = "The version of Go installed in the environment (" + v.goEnvVersion +
") is high enough for the version found in the `go.mod` file (" + v.goModVersion +
"). Writing an environment file not specifying any version of Go."
") is supported and is high enough for the version found in the `go.mod` file (" +
v.goModVersion + "). Not requesting any version of Go."
version = ""
diagnostics.EmitVersionGoModNotHigherVersionEnvironment(msg)
diagnostics.EmitGoModVersionSupportedLowerEqualGoEnv(msg)
}

return msg, version
Expand All @@ -803,18 +913,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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

func getVersionToInstall(v versionInfo) (msg, version string) {
msg, version = checkForUnsupportedVersions(v)
if msg != "" {
return msg, version
if !v.goModVersionFound {
return getVersionWhenGoModVersionNotFound(v)
}

msg, version = checkForVersionsNotFound(v)
if msg != "" {
return msg, version
if aboveSupportedRange(v.goModVersion) {
return getVersionWhenGoModVersionTooHigh(v)
}

msg, version = compareVersions(v)
return msg, version
if belowSupportedRange(v.goModVersion) {
return getVersionWhenGoModVersionTooLow(v)
}

return getVersionWhenGoModVersionSupported(v)
}

// Write an environment file to the current directory. If `version` is the empty string then
Expand Down
12 changes: 6 additions & 6 deletions go/extractor/cli/go-autobuilder/go-autobuilder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,13 @@ func TestGetVersionToInstall(t *testing.T) {
{"9999.0", true, "1.1", true}: "",
{"9999.0", true, "", false}: "",
// Go installation found with version below minGoVersion
{"1.20", true, "1.2.2", true}: "",
{"1.11", true, "1.2.2", true}: "",
{"", false, "1.2.2", true}: "",
{"1.20", true, "1.2.2", true}: "1.20",
{"1.11", true, "1.2.2", true}: "1.11",
{"", false, "1.2.2", true}: maxGoVersion,
// Go installation found with version above maxGoVersion
{"1.20", true, "9999.0.1", true}: "",
{"1.11", true, "9999.0.1", true}: "",
{"", false, "9999.0.1", true}: "",
{"1.20", true, "9999.0.1", true}: "1.20",
{"1.11", true, "9999.0.1", true}: "1.11",
{"", false, "9999.0.1", true}: maxGoVersion,

// checkForVersionsNotFound()

Expand Down
Loading