Go: Update autobuilder to deal with the upcoming deprecation of the legacy GOPATH mode #15361

mbg · 2024-01-17T21:28:09Z

This PR makes changes to the Go autobuilder to account for the deprecation of the legacy GOPATH mode in Go 1.22. In the process we refactor parts of the Go autobuilder to both account for that change and be able to handle multiple Go modules as well as Go workspaces.

We are now able to successfully extract the code for the following, existing integration tests:

go-files-found-not-processed (Multiple go.mod files)
single-go-work-not-in-root (go.work file)
two-go-mods-nested-none-in-root (Multiple go.mod files)
two-go-mods-not-nested (Multiple go.mod files)

This PR also adds query predicates to the integration tests to query the list of extracted files, so that we can now verify that the files we expect to be extracted have been extracted.

Best reviewed commit-by-commit.

smowton · 2024-01-18T10:03:26Z

I think we should check there aren't go.mod files inside the proposed root dir before doing this -- otherwise the strategy you described of running go get in the context of those go.mod files seems more promising

smowton

I think this would benefit from a block comment or a short doc outlining the different scenarios you hope to deal with and how you want to deal with them -- that would make it clearer if there are any cases not accounted for

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

smowton · 2024-01-26T10:57:57Z

go/extractor/project/project.go

+	for i, use := range workFile.Use {
+		if filepath.IsAbs(use.Path) {
+			// TODO: This case might be problematic for some other logic (e.g. stray file detection)
+			goModFilePaths[i] = filepath.Join(use.Path, "go.mod")


Check is under baseDir?

smowton · 2024-01-26T10:58:05Z

go/extractor/project/project.go

+			// TODO: This case might be problematic for some other logic (e.g. stray file detection)
+			goModFilePaths[i] = filepath.Join(use.Path, "go.mod")
+		} else {
+			goModFilePaths[i] = filepath.Join(filepath.Dir(workFilePath), use.Path, "go.mod")


Check is under baseDir?

smowton

Looks plausible. Would still benefit from writing down explicitly what you intend to happen in odd cases, like stray go files outside modules (I think you're creating a new module at the repository root in this case?)

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

go/extractor/project/project.go

The corresponding integration test now successfully extracts the project

…sable array

smowton · 2024-02-19T11:50:20Z

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

@@ -480,9 +480,17 @@ func installDependencies(workspace project.GoWorkspace) {

 		// get dependencies for all modules
 		for _, module := range workspace.Modules {
+			path := filepath.Dir(module.Path)
+
+			if util.DirExists(filepath.Join(path, "vendor")) {


Looks like this happens on both the existing module and the temporary module for legacy project paths. What are the expected consequences in each case?

owen-mc · 2024-02-21T11:53:07Z

The point of the go-files-found-not-processed integration test is to check that the TSP message "Go files were found but not processed" works correctly. It exploited the fact that we didn't process go files in a certain project layout. You've fixed that with the PR. So we should either change the test to trigger that scenario in some other way, or we should delete the test as it isn't testing what it's meant to (the project layout that it uses is tested in the integration test ql/go/ql/integration-tests/all-platforms/go/two-go-mods-nested-one-in-root). We could even go as far as to delete the TSP diagnostic, if we're sure that it can't be triggered any more... probably best to leave it in and keep a lookout for it in the dashboard.

owen-mc · 2024-02-23T13:50:27Z

go/ql/integration-tests/all-platforms/go/diagnostics/go-files-found-not-processed/test.expected

+extractedFiles
+| work/subdir/go.mod:0:0:0:0 | work/subdir/go.mod |
+| work/subdir/test.go:0:0:0:0 | work/subdir/test.go |


Why is work/go.mod not in this list? And is this the reason that we still produce the "Go files were found but not processed" diagnostic?

owen-mc · 2024-03-02T06:10:18Z

I vaguely thought that we might be able to get rid of the "rearrange files in $GOPATH" behaviour now (which would be nice as it's a bit brittle), but I see it's still there. Can you explain a situation when we need it?

owen-mc · 2024-03-02T06:16:33Z

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

+	if semver.Compare(toolchain.GetEnvGoSemVer(), "v1.21.0") < 0 && greatestGoVersion.Found && semver.Compare("v"+greatestGoVersion.Version, toolchain.GetEnvGoSemVer()) > 0 {
+		diagnostics.EmitNewerGoVersionNeeded(toolchain.GetEnvGoSemVer(), "v"+greatestGoVersion.Version)
 		if val, _ := os.LookupEnv("GITHUB_ACTIONS"); val == "true" {
 			log.Printf(
-				"The go.mod file requires version %s of Go, but version %s is installed. Consider adding an actions/setup-go step to your workflow.\n",
-				"v"+goVersionInfo.Version,
-				getEnvGoSemVer())
+				"A go.mod file requires version %s of Go, but version %s is installed. Consider adding an actions/setup-go step to your workflow.\n",
+				"v"+greatestGoVersion.Version,
+				toolchain.GetEnvGoSemVer())
 		}
 	}


Any particular reason this was moved after the $GOPATH rearrangement?

I moved the $GOPATH rearrangement ahead of this to address @smowton's comment in https://github.com/github/codeql/pull/15361/files/008585eeba2c455deb85b58e608a1822bf494dea#r1476152501

Do you have a concern about something I may have missed as part of this rearrangement?

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

go/extractor/project/project.go

owen-mc · 2024-03-02T06:52:18Z

go/extractor/project/project.go

+	if strings.Contains(string(out), "is relative, but relative import paths are not supported in module mode") {
+		diagnostics.EmitRelativeImportPaths()


Interesting! I thought relative import paths did work in module mode. As that seems to not be the case, we should update the message for this diagnostic and remove the suggestion to use a Go module.

go/extractor/project/project.go

owen-mc · 2024-03-02T13:10:32Z

go/extractor/project/project.go

+			}
+		}
+
+		return


This preserves the old behaviour, so it should stay like this. But I wonder if in future we might want to remove this return so that we get some other diagnostics in this case as well.

go/extractor/util/util.go

owen-mc

I've finished reviewing. I think it would be okay to merge this. I've only given minor suggestions, which could be dealt with in follow-up PRs.

mbg self-assigned this Jan 17, 2024

github-actions bot added the Go label Jan 17, 2024

mbg force-pushed the mbg/go/legacy-gopath-mode-deprecated branch 2 times, most recently from 45c349a to 4ad0e0f Compare January 25, 2024 19:02

smowton reviewed Jan 26, 2024

View reviewed changes

mbg force-pushed the mbg/go/legacy-gopath-mode-deprecated branch 4 times, most recently from 1a2e829 to 6d4a31d Compare February 1, 2024 12:40

mbg marked this pull request as ready for review February 1, 2024 15:48

mbg requested a review from a team as a code owner February 1, 2024 15:48

mbg requested a review from smowton February 1, 2024 15:49

smowton previously approved these changes Feb 2, 2024

View reviewed changes

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

go/extractor/project/project.go Outdated Show resolved Hide resolved

go/extractor/project/project.go Outdated Show resolved Hide resolved

mbg dismissed smowton’s stale review via a6f697e February 9, 2024 16:21

mbg force-pushed the mbg/go/legacy-gopath-mode-deprecated branch from ef4c1cd to 021cf80 Compare February 13, 2024 14:16

mbg requested a review from smowton February 13, 2024 14:31

mbg force-pushed the mbg/go/legacy-gopath-mode-deprecated branch from 021cf80 to 8ef53f4 Compare February 14, 2024 17:21

mbg added 12 commits February 14, 2024 19:12

Go: Try to initialise go.mod file for legacy projects

df21280

Go: Move getDirs into util and document/rename

0488d1d

Go: Rename findGoModFiles to getBuildRoot

c257116

Go: Emit relative path import diagnostic if prompted by go mod tidy

bd36847

The corresponding integration test now successfully extracts the project

Go: Move getEnvGoSemVer to toolchain.go

237bf56

Go: Add SupportsWorkspaces function

82bd1d7

Go: Introduce new types for representing logical workspaces

60879bd

Go: Add helper functions for discovering go.work and go.mod files

3f53186

Go: Add new functions for discovering workspaces and modules

4f5c43a

Go: Allow GetBuildInfo to return multiple BuildInfo objects

fc75e44

Go: Use new workspace/module discovery

f013d9d

Go: Add shared TidyModule function

2546340

mbg added 5 commits February 14, 2024 19:12

Go: Only call EmitNewerGoVersionNeeded at most once

e2c6734

Go: Refactor Autobuild to use pairs of scripts and tools from a reu…

6dbb5c5

…sable array

Go: Fix missing word in comment for discoverWorkspace

4387c73

Go: Export InitGoModForLegacyProject

1055e77

Go: Postpone go.mod creation until necessary

6267506

mbg force-pushed the mbg/go/legacy-gopath-mode-deprecated branch from 8ef53f4 to 6267506 Compare February 14, 2024 19:12

mbg added 3 commits February 15, 2024 16:19

Go: Call go mod vendor to synchronise vendor directory when it exists

4d28c0d

Go: Try to ignore errors in go mod vendor calls

8886092

Go: Include arguments in RunCmd error messages

008585e

smowton reviewed Feb 19, 2024

View reviewed changes

owen-mc reviewed Feb 23, 2024

View reviewed changes