[go] cmd/go: stamp VCS info when building .go files directly
Quan Tong (Gerrit)
Quan Tong has uploaded this change for review.
cmd/go: stamp VCS info when building .go files directly
The existing implementation only stamps VCS info
if the requested package is within a module in the workspace.
Fixes #51279
Change-Id: I014713138cec4cb8d1cbe7488c35389c630b011f
---
M src/cmd/go/internal/load/pkg.go
M src/cmd/go/testdata/script/build_buildvcs_auto.txt
2 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go
index 1549800..1a96b4f 100644
--- a/src/cmd/go/internal/load/pkg.go
+++ b/src/cmd/go/internal/load/pkg.go
@@ -2451,9 +2451,6 @@
setPkgErrorf("error obtaining VCS status: %v\n\tUse -buildvcs=false to disable VCS stamping.", err)
}
- var repoDir string
- var vcsCmd *vcs.Cmd
- var err error
const allowNesting = true
wantVCS := false
@@ -2467,6 +2464,11 @@
panic(fmt.Sprintf("unexpected value for cfg.BuildBuildvcs: %q", cfg.BuildBuildvcs))
}
+ repoDir, vcsCmd, err := vcs.FromDir(base.Cwd(), "", allowNesting)
+ if err != nil && !errors.Is(err, os.ErrNotExist) {
+ setVCSError(err)
+ return
+ }
if wantVCS && p.Module != nil && p.Module.Version == "" && !p.Standard {
if p.Module.Path == "bootstrap" && cfg.GOROOT == os.Getenv("GOROOT_BOOTSTRAP") {
// During bootstrapping, the bootstrap toolchain is built in module
@@ -2474,11 +2476,6 @@
// (so the bootstrap toolchain packages don't even appear to be in GOROOT).
goto omitVCS
}
- repoDir, vcsCmd, err = vcs.FromDir(base.Cwd(), "", allowNesting)
- if err != nil && !errors.Is(err, os.ErrNotExist) {
- setVCSError(err)
- return
- }
if !str.HasFilePathPrefix(p.Module.Dir, repoDir) &&
!str.HasFilePathPrefix(repoDir, p.Module.Dir) {
// The module containing the main package does not overlap with the
@@ -2494,6 +2491,19 @@
goto omitVCS
}
}
+
+ modRepoDir, _, err := vcs.FromDir(p.Module.Dir, "", allowNesting)
+ if err != nil {
+ setVCSError(err)
+ return
+ }
+ if modRepoDir != repoDir {
+ if cfg.BuildBuildvcs != "auto" {
+ setVCSError(fmt.Errorf("main module is in repository %q but current directory is in repository %q", modRepoDir, repoDir))
+ return
+ }
+ goto omitVCS
+ }
}
if repoDir != "" && vcsCmd.Status != nil {
// Check that the current directory, package, and module are in the same
@@ -2512,18 +2522,6 @@
}
goto omitVCS
}
- modRepoDir, _, err := vcs.FromDir(p.Module.Dir, "", allowNesting)
- if err != nil {
- setVCSError(err)
- return
- }
- if modRepoDir != repoDir {
- if cfg.BuildBuildvcs != "auto" {
- setVCSError(fmt.Errorf("main module is in repository %q but current directory is in repository %q", modRepoDir, repoDir))
- return
- }
- goto omitVCS
- }
st, err := vcsStatusCache.Do(repoDir, func() (vcs.Status, error) {
return vcsCmd.Status(vcsCmd, repoDir)
diff --git a/src/cmd/go/testdata/script/build_buildvcs_auto.txt b/src/cmd/go/testdata/script/build_buildvcs_auto.txt
index cfd5d82..2b49496 100644
--- a/src/cmd/go/testdata/script/build_buildvcs_auto.txt
+++ b/src/cmd/go/testdata/script/build_buildvcs_auto.txt
@@ -29,6 +29,12 @@
stdout '^\tbuild\tvcs=git$'
stdout '^\tbuild\tvcs.modified=false$'
+go build -o example.exe example.go
+go version -m example.exe
+stdout '^\tbuild\tvcs=git$'
+stdout '^\tbuild\tvcs.revision=[a-f\d]{40}'
+stdout '^\tbuild\tvcs.time=.+'
+stdout '^\tbuild\tvcs.modified=true$'
# Building a binary from a different (nested) VCS repo should not stamp VCS
# info. It should be an error if VCS stamps are requested explicitly with
To view, visit change 541777. To unsubscribe, or for help writing mail filters, visit settings.
Vegard Stikbakke (Gerrit)
Vegard Stikbakke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vegard Stikbakke
| Code-Review | +1 |
1 comment
Great patch! Would love for this to get in, we often build using `go ... cmd/<binary>/main.go` which means we need to inject the Git has differently
Related details
- Bryan Mills
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob added 2 comments![]( "Open in Gerrit")
repoDir, vcsCmd, err := vcs.FromDir(base.Cwd(), "", allowNesting)Moving this out of the `if` means that we ignore `wantVCS`, `p.Standard`, etc. which is not correct.
I think what we might want to do is to keep this in the next if block but change its condition to `wantVCS && !p.Standard`. And then add a further nested if for the case where `p.Module != nil`.
For the case where `p.Module == nil` we should check that the file itself is contained in the work module or workspace. See bcmills's issue comment: https://github.com/golang/go/issues/51279#issuecomment-1048196896
if modRepoDir != repoDir {
if cfg.BuildBuildvcs != "auto" {
setVCSError(fmt.Errorf("main module is in repository %q but current directory is in repository %q", modRepoDir, repoDir))
return
}
goto omitVCS
}we shouldn't be doing this check if repoDir is empty. i think it still belongs in the next if.
Perhaps we should keep this code in the next if but add a check that `p.Module` is not nil
Related details
- Bryan Mills
- Quan Tong
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vegard Stikbakke (Gerrit)
Vegard Stikbakke added 2 comments![]( "Open in Gerrit")
repoDir, vcsCmd, err := vcs.FromDir(base.Cwd(), "", allowNesting)Moving this out of the `if` means that we ignore `wantVCS`, `p.Standard`, etc. which is not correct.
I think what we might want to do is to keep this in the next if block but change its condition to `wantVCS && !p.Standard`. And then add a further nested if for the case where `p.Module != nil`.
For the case where `p.Module == nil` we should check that the file itself is contained in the work module or workspace. See bcmills's issue comment: https://github.com/golang/go/issues/51279#issuecomment-1048196896
Are there any tests we can add to `build_buildvcs_auto.txt` to show this isn't correct?
go build -o example.exe example.goShould this block have a matching comment section above it? The comment above does seem to apply only to the `go build .` case ("on a package ... by default").
Related details
- Bryan Mills
- Michael Matloob
- Quan Tong
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted Commit-Queue+1![]( "Open in Gerrit")
- Bryan Mills
- Quan Tong
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Matloob
| Commit-Queue | +1 |
1 comment
repoDir, vcsCmd, err := vcs.FromDir(base.Cwd(), "", allowNesting)Vegard StikbakkeMoving this out of the `if` means that we ignore `wantVCS`, `p.Standard`, etc. which is not correct.
I think what we might want to do is to keep this in the next if block but change its condition to `wantVCS && !p.Standard`. And then add a further nested if for the case where `p.Module != nil`.
For the case where `p.Module == nil` we should check that the file itself is contained in the work module or workspace. See bcmills's issue comment: https://github.com/golang/go/issues/51279#issuecomment-1048196896
Are there any tests we can add to `build_buildvcs_auto.txt` to show this isn't correct?
It looks like tests are already failing because we're triggering vcs stamping when we shouldn't be.
But here, simple test that sets -buildvcs=false should fail. For example adding the following after line 31 will fail:
# With -buildvcs=false, 'go build' should not stamp
# VCS metadata.
go build -buildvcs=false -o example.exe .
go version -m example.exe
! stdout '^\tbuild\tvcs=git$'
! stdout '^\tbuild\tvcs.modified=true$'
p.Standard is a bit harder to test: we can't simply build a standard library package because the standard library might not be in VCS. We might have to make a fake GOROOT, but I haven't tried that.
Related details
- Bryan Mills
- Michael Matloob
- Quan Tong
- Vegard Stikbakke
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |