[build] internal/relui: include a package in fake golang.org/x/build module
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov has uploaded the change for review![]( "Open in Gerrit")
Dmitri Shuralyov would like Alan Donovan to review this change.
Commit message
internal/relui: include a package in fake golang.org/x/build module
The "maintain go directive" automation includes a "go fix ./..." step.
Copying the motivation from go.dev/design/69095-x-repo-continuous-go:
> The first two commands in the sequence leave the module in a tidy
> state. The go fix ./... command will apply high-confidence automated
> changes, in case any begin to apply with the updated Go language
> version. For example, go fix began to remove the now-obsolete
> // +build lines once a module is upgraded to 1.18 or later. For many
> new language versions this will be a no-op, but it is expected that
> including a go fix ./... invocation will be a net positive. We can
> decide to stop including it in the generated CLs based on experience.
CL 700795 changed the behavior of "go fix ./..." in a module where the
"./..." pattern matches no packages from reporting a 0 exit code (like
'go build' does) to reporting a non-zero exit code (like 'go vet' does).
Add a package to the fake x/build module to make the TestRelease/major
test not sensitive to go fix's exit code when encountering no packages.
Such a test should exist, but it doesn't belong in an x/ repo far away
from the actual go fix command.
For golang/go#71859.
Change diff
diff --git a/internal/relui/buildrelease_test.go b/internal/relui/buildrelease_test.go
index 66c9d4f..fba2faa 100644
--- a/internal/relui/buildrelease_test.go
+++ b/internal/relui/buildrelease_test.go
@@ -155,7 +155,8 @@
dlRepo := task.NewFakeRepo(t, "dl")
buildRepo := task.NewFakeRepo(t, "build")
buildRepo.Commit(map[string]string{
- "go.mod": "module golang.org/x/build\n",
+ "go.mod": "module golang.org/x/build\n",
+ "build.go": "package build\n",
})
toolsRepo := task.NewFakeRepo(t, "tools")
toolsRepo.Commit(map[string]string{
Change information
- M internal/relui/buildrelease_test.go
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov added 1 comment![]( "Open in Gerrit")
CL 700795 changed the behavior of "go fix ./..." in a module where the@adon...@google.com I'm not yet sure whether the new 'go fix' behavior is better, since it requires automated callers either to add logic to handle "no packages to fix" errors, or to take extra measures to avoid running go fix if "./..." happens not to match any packages in the module being automatically processed. Both
can be hard to achieve in some environments (i.e., if generating a bash script that processes many modules).
In https://go.dev/issue/71859#issuecomment-2935949731, you wrote:
`go fix` command-line interface should resemble `go vet` as much as possible, [...] `vet` is responsible for emitting diagnostics and `fix` is responsible for quietly applying suggested fixes.
> [...]
> Our goal is that users should be able to run `go fix` on a large code base and merge the resulting CL with only cursory review, confident that the tools have not introduced bugs.
I understand and agree with the rationale to make go fix and go vet as similar as possible, but I wonder if making "go fix" exit with non-zero exit code goes against the goal of making it easy for users to run "go fix" on a large code base and merge the resulting CL with only cursory review. That is, it seems "no packages" being an error is maybe more friction for callers of go fix (e.g., potential scripts that process many repos) than it is for go vet callers (more likely to be humans rather than scripts). What do you think?
I mostly want to raise your attention on this, so feel free to ack this and leave follow up (if any) to issue #71859 rather than here. Thanks. (FWIW, issues #31271 and #52287 are also somewhat relevant to this topic.)
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
CL 700795 changed the behavior of "go fix ./..." in a module where the@adon...@google.com I'm not yet sure whether the new 'go fix' behavior is better, since it requires automated callers either to add logic to handle "no packages to fix" errors, or to take extra measures to avoid running go fix if "./..." happens not to match any packages in the module being automatically processed. Both
can be hard to achieve in some environments (i.e., if generating a bash script that processes many modules).In https://go.dev/issue/71859#issuecomment-2935949731, you wrote:
`go fix` command-line interface should resemble `go vet` as much as possible, [...] `vet` is responsible for emitting diagnostics and `fix` is responsible for quietly applying suggested fixes.
> [...]
> Our goal is that users should be able to run `go fix` on a large code base and merge the resulting CL with only cursory review, confident that the tools have not introduced bugs.I understand and agree with the rationale to make go fix and go vet as similar as possible, but I wonder if making "go fix" exit with non-zero exit code goes against the goal of making it easy for users to run "go fix" on a large code base and merge the resulting CL with only cursory review. That is, it seems "no packages" being an error is maybe more friction for callers of go fix (e.g., potential scripts that process many repos) than it is for go vet callers (more likely to be humans rather than scripts). What do you think?
I mostly want to raise your attention on this, so feel free to ack this and leave follow up (if any) to issue #71859 rather than here. Thanks. (FWIW, issues #31271 and #52287 are also somewhat relevant to this topic.)
I think of go vet and go fix as analogous of go build, but running analyzers instead of compilers, and I notice that go build exits zero when asked to build ./dir/... in a tree containing no Go source files. So that argues for making go vet and fix more like go build. But I wonder how in practice it would ever arise that a script runs go fix on a tree of no Go packages. That seems like a misconfiguration.
Thanks for this CL.
Related details
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan voted Code-Review+2![]( "Open in Gerrit")
| Code-Review | +2 |
Related details
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |