[tools] go/packages: explicitly disable collecting vcs information when not needed
Pierre Gimalac (Gerrit)
Pierre Gimalac has uploaded the change for review![]( "Open in Gerrit")
Commit message
go/packages: explicitly disable collecting vcs information when not needed
By default VCS information is collected only when needed, which is not the case here, so this change shouldn't be necessary.
However this is currently being collected for test packages, and it seems that it has already happened before as well,
so since here we know it's not needed we can just force disable it to ensure it is fixed and stays fixed.
This fixes (some) issues with gopls hanging for a long time while initializing.
In my case it reduces the go list command duration from 2min30 to around 10 seconds.
Fixes golang/go#77419.
Fixes golang/go#51999.
Change diff
diff --git a/go/packages/golist.go b/go/packages/golist.go
index 680a70c..a11b431 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -842,6 +842,11 @@
fullargs = append(fullargs, "-pgo=off")
}
+ if !usesExportData(cfg) {
+ // VCS information is not needed when -export=false and not printing Stale or StaleReason fields
+ fullargs = append(fullargs, "-buildvcs=false")
+ }
+
fullargs = append(fullargs, cfg.BuildFlags...)
fullargs = append(fullargs, "--")
fullargs = append(fullargs, words...)
Change information
- M go/packages/golist.go
Related details
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")
Thanks for investigating this. I noticed this was an issue last week, and Peter (cc'd) has started looking into certain go list inefficiencies.
It would be nice if these tricky cross-cutting interactions had tests. I think it should be possible to set up a test by running 'git init' in a temp directory, to establish for go list that this is a git repo, and then putting a fake `git` bash wrapper script on the PATH ahead of the real git; the fake git can log its interactions so that the test can assert whether git was or was not called as expected.
Related details
- Pierre Gimalac
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")
Thanks for investigating this. I noticed this was an issue last week, and Peter (cc'd) has started looking into certain go list inefficiencies.
It would be nice if these tricky cross-cutting interactions had tests. I think it should be possible to set up a test by running 'git init' in a temp directory, to establish for go list that this is a git repo, and then putting a fake `git` bash wrapper script on the PATH ahead of the real git; the fake git can log its interactions so that the test can assert whether git was or was not called as expected.
Michael (cc'd) is the expert here and may have better ideas or know of precedents for such tests.
Related details
- Pierre Gimalac
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pierre Gimalac (Gerrit)
Pierre Gimalac uploaded new patchset![]( "Open in Gerrit")
Related details
- Pierre Gimalac
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pierre Gimalac (Gerrit)
Pierre Gimalac added 1 comment![]( "Open in Gerrit")
@p...@google.com @adon...@google.com @mat...@google.com is there anything I need to do for this ?
The fix on go itself was merged (https://go-review.googlesource.com/c/go/+/740901), but I still think this one is valuable as well, since we know VCS info is not needed here.
Related details
- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Matloob
| Code-Review | +2 |
1 comment
This change looks fine to me, but I agree with Alan that it would be nice to have tests.
Related details
- Michael Matloob
- Peter Weinberger
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 1 comment![]( "Open in Gerrit")
@p...@google.com @adon...@google.com @mat...@google.com is there anything I need to do for this ?
The fix on go itself was merged (https://go-review.googlesource.com/c/go/+/740901), but I still think this one is valuable as well, since we know VCS info is not needed here.
I think the only thing is trying to see if we can add tests?
Related details
- Michael Matloob
- Peter Weinberger
- Pierre Gimalac
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pierre Gimalac (Gerrit)
Pierre Gimalac added 1 comment![]( "Open in Gerrit")
Michael Matloob@p...@google.com @adon...@google.com @mat...@google.com is there anything I need to do for this ?
The fix on go itself was merged (https://go-review.googlesource.com/c/go/+/740901), but I still think this one is valuable as well, since we know VCS info is not needed here.
I think the only thing is trying to see if we can add tests?
Are you thinking unit tests or actual e2e tests ensuring gopls doesn't exec git under the hood ?
Related details
- Michael Matloob
- Michael Matloob
- Peter Weinberger
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")
Michael Matloob@p...@google.com @adon...@google.com @mat...@google.com is there anything I need to do for this ?
The fix on go itself was merged (https://go-review.googlesource.com/c/go/+/740901), but I still think this one is valuable as well, since we know VCS info is not needed here.
Pierre GimalacI think the only thing is trying to see if we can add tests?
Are you thinking unit tests or actual e2e tests ensuring gopls doesn't exec git under the hood ?
A test that calls packages.Load having temporarily added a directory containing a fake git command to the PATH should do the trick. The fake command just needs to do something that causes the test to fail.
Related details
- Michael Matloob
- Michael Matloob
- Peter Weinberger
- Pierre Gimalac
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |