Go: Add go-autobuilder --identify-environment
#12957
Conversation
owen-mc
force-pushed
the
go/autobuilder-identify-environment
branch
from
44396a8 to
c02edb0
Compare
owen-mc
force-pushed
the
go/autobuilder-identify-environment
branch
from
c02edb0 to
0c6efb8
Compare
| `%s is a wrapper script that installs dependencies and calls the extractor, or if '--identify-environment' | ||
| is passed then it produces a file 'environment.json' which specifies what go version is needed. |
There was a problem hiding this comment.
It would probably be nice to format this across multiple lines, something like:
%s is a wrapper script that installs dependencies and calls the extractor
Options:
--identify-environment If this flag is specified, then ...
| content = `{ "include": [ { "go": { "version": "` + version + `" } } ] }` | ||
| } | ||
|
|
||
| targetFile, err := os.Create("environment.json") |
There was a problem hiding this comment.
As we have talked about, this should probably be a value from an environment variable (with this as a fallback).
|
|
||
| func EmitUnsupportedVersionGoMod(msg string) { | ||
| emitDiagnostic( | ||
| "go/identify-environment/unsupported-version-in-go-mod", |
There was a problem hiding this comment.
I wonder if this should still be go/autobuilder/.. for these diagnostics, since that part of the identifier refers to the tool rather than a particular sub-command.
There was a problem hiding this comment.
I've changed it so that go/autobuilder/env- is a common prefix to all the these new diagnostics.
| func EmitVersionGoModNotHigherVersionEnvironment(msg string) { | ||
| emitDiagnostic( | ||
| "go/identify-environment/version-go-mod-not-higher-than-go-env", | ||
| "The Go version in `go.mod` file is not higher than the Go version in environment", |
There was a problem hiding this comment.
"not higher" is strange wording. Should this be "lower"? Or "not equal to or greater than"?
| if v.goDirectiveFound && outsideSupportedRange(v.goModVersion) { | ||
| msg = "The version of Go found in the `go.mod` file (" + v.goModVersion + | ||
| ") is outside of the supported range (" + minGoVersion + "-" + maxGoVersion + ")." | ||
| version = "" |
There was a problem hiding this comment.
Would it be better to return nil instead of an empty string if these functions cannot determine which go version to install? And have msg as an error rather than a string?
There was a problem hiding this comment.
I like the nil idea but strings can't be nil in go. I have effectively given the empty string a special meaning, and I've been careful to always assign a value to version even when I could leave it as the default value so that it's easier to spot any places where it hasn't been considered.
There was a problem hiding this comment.
I don't think msg should be an error because then people will assume that it's only non-nil if something has gone wrong, whereas it's actually non-nil on every path through the code.
| func getVersionToInstall(v versionInfo) (msg, version string) { | ||
| msg, version = checkForUnsupportedVersions(v) | ||
| if msg != "" { | ||
| return msg, version | ||
| } | ||
|
|
||
| msg, version = checkForVersionsNotFound(v) | ||
| if msg != "" { | ||
| return msg, version | ||
| } | ||
|
|
||
| msg, version = compareVersions(v) | ||
| return msg, version | ||
| } |
There was a problem hiding this comment.
The logic implemented in these functions is relatively complicated and tricky to follow / it is easy to miss corner cases. Once tests are added, those will increase confidence, but it would also be good to add documentation and more explicitly document assumptions/invariants throughout these functions.
There was a problem hiding this comment.
I'll add docstrings to the functions. (With a lot of help from Copilot 😄 .)
| func checkForVersionsNotFound(v versionInfo) (msg, version string) { | ||
| if !v.goInstallationFound && !v.goDirectiveFound { | ||
| msg = "No version of Go installed and no `go.mod` file found. Writing an " + | ||
| "`environment.json` file specifying the maximum supported version of Go (" + |
There was a problem hiding this comment.
For all the occurrences of environment.json here, you can probably use a generic name here.
Step 1 of https://github.com/github/codeql-go-team/issues/329.
Integration tests look tricky. I will instead use unit tests in
ql/go/extractor/cli/go-autobuilder/go-autobuilder_test.goand some manual end-to-end testing. Once other languages have also implemented this flag we can set up everything up to make integration testing possible.