-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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/go/internal/modfetch: document known bug in isVendoredPackage
#31562
Comments
At least the failure mode here is fairly benign: we know that So the net effect is that we'll include some packages that aren't strictly necessary, but fail to prune some contents that really aren't needed. |
isVendoredPackage
returns false for some packages that are actually vendored
As @rsc put it in #30369 (comment): Since this error won't break builds (it includes packages that should be excluded, but does not exclude packages that should be included), it doesn't seem worth the churn of fixing at this time. We can revisit if we discover more severe bugs that are worth breaking the hash function over; for now, we should just document the known issue in the code. |
isVendoredPackage
returns false for some packages that are actually vendoredisVendoredPackage
Change https://golang.org/cl/172977 mentions this issue: |
Change https://golang.org/cl/220657 mentions this issue: |
I had thought that the known bug in isVendoredPackage was strictly conservative, but it turns out not to be. Updates golang/go#37397 Updates golang/go#31562 Change-Id: I60f6062c41ec2d116766930f751d7df083535355 Reviewed-on: https://go-review.googlesource.com/c/mod/+/220657 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
@samthanawalla I think post-1.21 we can now fix this, but only on modules declaring a new enough version of go. |
This issue from 2019 was resolved and frozen due to age. For new work, maybe it'd be better to file a new issue and refer to this one. |
Oops! Closing |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
go/src/cmd/go/internal/modfetch/coderepo.go
Lines 632 to 642 in c8aaec2
What did you expect to see?
What did you see instead?
The function is not documented, but it is obvious that it is not doing what the intention is.
It seems that the intention is to match against this regex:
(^vendor/|/vendor/).*/
But in fact it does random match by skipping first 8 bytes in case the input contains
/vendor/
.The fix is to add the j to the i as states above.
It is quite important to fix it early on, because it may cause go-modules with different hashes and a late fix can cause more hash mismatch of the same content (defy the immutability logic).
The text was updated successfully, but these errors were encountered: