-
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: standard-library vendoring in module mode #30241
Comments
(CC @rsc @bradfitz @jayconrod) |
I am strongly in favor of using modules for vendoring in the standard library, instead of ad-hoc copies, but I don't see the reason to let applications override standard library dependencies. Was this widely requested, or something we wished we had often? Speaking for x/crypto, I actually don't want to have to figure out if a crypto/tls internal failure is due to a Also, I don't see why we'd let users override vendored dependencies but not standard library packages. The difference between golang.org/x/crypto/chacha20poly1305 and crypto/aes should be an implementation detail of crypto/tls. Then why can one be overridden, and the other not? Feels arbitrary. |
cc @ianthehat I share some of @FiloSottile's concerns. Allowing users to override std dependencies, especially crypto dependencies, is scary. But not allowing this would be against the spirit of modules. There are some safeguards here, but I'd love to solve this without making the module resolution and package loading logic more complicated. Possibly dumb, low-tech idea: what if we vendor these modules without using vendoring? We could run |
We have that today, in |
It has come up at Go team summits from time to time, and while we don't see many complaints about upgrading The two are closely related, in that one way you can end up with an overly-large binary is by pulling in a redundant copy of an Per my favorite quote from C. A. R. Hoare's Hints on Programming Language Design:
It's much easier for us to reduce code duplication in the standard library than to reduce the outputs generated from duplicated code. |
We probably should start requesting the output of But consider the opposite direction, too: wouldn't it be nice for folks to be able to, say, test the interaction of TLS 1.3 with their dependencies written against 'net/http`, without having to also run a bleeding-edge version of the compiler and runtime (with any associated runtime bugs)? |
That seems like a great argument for making many of the standard-library packages thin forwarding shims around |
I'm definitely sympathetic to reducing code duplication and binary size, but I'm also concerned about complexity and special cases. I think we should resolve #26924 using the |
This is the same reaction I had when I saw the objection over updating the std lib versions. Make it easy and known how to list module versions and request it in issues. I also remember when Azure had to vendor a patched version of crypto/tls to access to hit their TLS endpoints in their SDK, then fork the world to make it reference the fork. |
@jayconrod, in looking at the details of this, it seems to be by far the simplest way to make module-mode vendoring work for vendoring in the standard library.
|
I don't disagree with including |
@bcmills I agree that this is probably the simplest way to make module-mode vendoring work in the standard library in terms of new code that needs to be written. My concern is that this makes package loading harder to reason about. In particular, this proposal means that you may get either one or two copies of some packages, depending on what version the main module requires or whether it has a replacement. We have the same risk of conflict with exported types or global state as we do today, but now the risk is more subtle. Here's a possible alternative (with some drawbacks at the end):
I'm realizing some drawbacks to this approach though, as I'm typing this out.
|
Not only that, but all of their transitive dependencies too: for example, various parts of Note that everything in
|
I can certainly sympathize with that, but in that case, is there a fundamental reason for the implementation of That is: if it's that tightly coupled to |
Change https://golang.org/cl/162989 mentions this issue: |
In CL 149609, a file was added to src/cmd/vendor/golang.org/x/tools/go/analysis/internal/analysisflags/patch.go to override the behavior of the V flag for cmd/vet. That modification causes the behavior of cmd/vet to change when a pristine copy of x/tools is vendored in, and module-mode vendoring will only support pristine copies (see golang/go#30240). Instead, allow cmd/vet to override the V flag by defining its own V flag before it invokes unitchecker.Main. Tested manually (by patching into cmd/vendor). Updates golang/go#30240 Updates golang/go#30241 Updates golang/go#26924 Updates golang/go#30228 Change-Id: I10e4523e1f4ede94fbfc745012dadeefef48e927 Reviewed-on: https://go-review.googlesource.com/c/162989 Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
To summarize so far: it seems that there are some concerns about the module-upgrade behavior, but no objections to the remainder of the proposal. Since a lot of this work needs to happen early in the cycle, I'm going to go ahead and implement a more limited form: for now, we'll always resolve dependencies of the standard library to the packages in the |
Making the implementation of Maybe we could still make main security releases which just bump the (Also, I'm not convinced by the argument that a |
Agreed, but I think the point generalizes: any package in |
I have two questions about this proposal.
The standard library has a policy on which packages are allowed to import which others. It's codified in: Lines 23 to 24 in 889aa5e
Have you considered following the L0/L1/L2/L3/(L4+everything else) tiers there to guide std lib module boundaries (e.g., having
What is the motivation to make these modules differ from ordinary modules? |
Change https://golang.org/cl/163207 mentions this issue: |
any progress for this proposal? |
We've recently had a case that touches @FiloSottile's comment. Working with a broken device containing borked certificate we needed to modify Is there anything that could move this proposal into the active column of the proposals project? |
This proposal has been added to the active column of the proposals project |
It sounds like people are generally OK with this. There is some question about whether there are problems with x/crypto being too tied to the main repo, but if so we should find and fix those. |
Are there any objections to making this change? |
Over the past few years we actually brought a lot from x/crypto into std, so these days only chacha20poly1305, hkdf, and cryptobyte are vendored into the standard library. This makes upgrading x/crypto both less troublesome and less useful. I find it a bit weird to expose upgrading these specific packages but not anything else in the stdlib to applications. Why can they upgrade ChaCha20Poly1305 but not AES-GCM? Why can a X.509 parsing bug be fixed with a Looking at the full list of vendored packages, this doesn't feel arbitrary only for x/crypto.
In other words, it's nice if by luck a bug is fixable by replacing cryptobyte, but I think it would be more useful to talk about this in the broader context of a plan that involves carving out the runtime and compiler independent pieces of |
Out of curiosity, shouldn't we be able to deduplicate code and get smaller binaries in this case as well? The benefits are only mentioned in the third case (build list has a newer version), but not in this second case (build list has the same version). |
@FiloSottile, I agree that this proposal would be more useful if more of the standard library could be upgraded. The process seems straightforward enough: for a given package in |
To be clear, this change is not really being made just for x/crypto. It's a step toward a more rational relationship between std and x. We'll see what the next step is after we take this one. |
Based on the discussion above, this proposal seems like a likely accept. |
Searching for If I understand correctly, those would break. If we want to avoid breaking them, we could use for example |
|
Looking at search result counts is inaccurate. Those are all copies of the Go source tree. |
Ah, missed that, thanks for the link! I was checking https://go.dev/ref/mod#module-path before I posted my comment and that page does not mention std nor cmd as special name.
Fair point. I haven't counted how many are copies of Go source tree. Could be that most of them are. There are some examples that aren't copies of Go source tree: To be clear - I'm fine with disallowing module names beginning with |
No change in consensus, so accepted. 🎉 |
This proposal is both a fix for #26924, and a means for users to explicitly upgrade the
golang.org/x
dependencies of the standard library (such asx/crypto
andx/net
).Proposal
The standard library will ship with three go.mod files:
src/go.mod
(with module pathstd
),src/cmd/go.mod
(with module pathcmd
), andmisc/go.mod
(with module pathmisc
).The
std
andcmd
modules will havevendor
directories, containingmodules.txt
files in the usual format and managed using the standard Go 1.13 vendoring semantics (hopefully #30240).The
std
andcmd
modules differ somewhat from ordinary modules, in that they do not appear in the build list.GOROOT/src
may declare its own module path to begin withstd
orcmd
.require
,replace
, orexclude
any module whose path begins withstd
orcmd
. (Their versions are fixed to the Go release in use.)For files within
GOROOT/src
only, when we resolve an import of a package provided by an external module, we will first check the build list for that module.vendor
orcmd/vendor
.std
orcmd
, that version is notreplace
d in the main module, and the package exists in the correspondingvendor
directory, then we will load the package fromvendor
orcmd/vendor
with its original import path.std
orcmd
itself (that is, if the user is working withinGOROOT/src
).GOROOT/src/vendor
andGOROOT/src/cmd/vendor
will remain pristine and unmodified.std
orcmd
, or if the version is equal and the module isreplace
d, then we will load the package from the module cache in the usual way.The text was updated successfully, but these errors were encountered: