Skip to content
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.

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

cmd/dist: ensure consistency between (*tester).supportedBuildmode and cmd/internal/sys.BuildModeSupported #43571

Open
bcmills opened this issue Jan 7, 2021 · 2 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 7, 2021

(*cmd/dist.tester).supportedBuildmode contains a large switch that determines whether a given buildmode is supported for a given GOOS/GOARCH:

go/src/cmd/dist/test.go

Lines 977 to 1038 in fa90aac

func (t *tester) supportedBuildmode(mode string) bool {
pair := goos + "-" + goarch
switch mode {
case "c-archive":
if !t.extLink() {
return false
}
switch pair {
case "aix-ppc64",
"darwin-amd64", "darwin-arm64", "ios-arm64",
"linux-amd64", "linux-386", "linux-ppc64le", "linux-s390x",
"freebsd-amd64",
"windows-amd64", "windows-386":
return true
}
return false
case "c-shared":
switch pair {
case "linux-386", "linux-amd64", "linux-arm", "linux-arm64", "linux-ppc64le", "linux-s390x",
"darwin-amd64", "darwin-arm64",
"freebsd-amd64",
"android-arm", "android-arm64", "android-386",
"windows-amd64", "windows-386":
return true
}
return false
case "shared":
switch pair {
case "linux-386", "linux-amd64", "linux-arm", "linux-arm64", "linux-ppc64le", "linux-s390x":
return true
}
return false
case "plugin":
// linux-arm64 is missing because it causes the external linker
// to crash, see https://golang.org/issue/17138
switch pair {
case "linux-386", "linux-amd64", "linux-arm", "linux-s390x", "linux-ppc64le":
return true
case "darwin-amd64", "darwin-arm64":
return true
case "freebsd-amd64":
return true
}
return false
case "pie":
switch pair {
case "aix/ppc64",
"linux-386", "linux-amd64", "linux-arm", "linux-arm64", "linux-ppc64le", "linux-riscv64", "linux-s390x",
"android-amd64", "android-arm", "android-arm64", "android-386":
return true
case "darwin-amd64", "darwin-arm64":
return true
case "windows-amd64", "windows-386", "windows-arm":
return true
}
return false
default:
fatalf("internal error: unknown buildmode %s", mode)
return false
}
}

That switch nearly exactly duplicates the one in cmd/internal/sys.BuildModeSupported, which was itself extracted from a portion of cmd/go/internal/work.buildModeInit.

cmd/dist should be changed to use (instead of duplicating) the function from cmd/internal/sys.

@bcmills bcmills added this to the Backlog milestone Jan 7, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 8, 2021

My understanding is that cmd/dist needs to build with Go 1.4, so it can't import cmd/internal/sys. That's why I wrote two versions initially.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Jan 8, 2021

Ah, that could be.

But in that case, we should still have something in place that ensures that the two switches are equivalent, such as a regression-test that compares the two functions.

Or, perhaps (*tester).supportedBuildmode should probe cmd/go to determine support instead of encoding its own parallel assumptions in-process.

@gopherbot gopherbot removed the NeedsFix label Jan 8, 2021
@bcmills bcmills changed the title cmd/dist: replace (*tester).supportedBuildmode with cmd/internal/sys.BuildModeSupported cmd/dist: ensure consistency between (*tester).supportedBuildmode and cmd/internal/sys.BuildModeSupported Jan 8, 2021
@bcmills bcmills added the Testing label Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants