Navigation Menu

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

Jump to bottom

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

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

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

bcmills opened this issue Jan 7, 2021 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

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 help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jan 7, 2021
@bcmills bcmills added this to the Backlog milestone Jan 7, 2021
@ianlancetaylor
Copy link
Contributor

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
Contributor Author

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.

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed help wanted labels Jan 8, 2021
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. 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 An issue that has been verified to require only test changes, not just a test failure. label Jan 8, 2021
cmburn added a commit to cmburn/go that referenced this issue Apr 28, 2022
@cmburn
add build mode support regression test for issue golang#43571
e37b485
@cmburn cmburn mentioned this issue Apr 28, 2022
Open
@gopherbot
Copy link

Change https://go.dev/cl/403014 mentions this issue: cmd/dist: add build mode support regression test for issue #43571

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@ianlancetaylor @bcmills @gopherbot