-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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
proposal: cmd/go: add pin keyword to go.mod #51021
Comments
The "pin" keyword that you suggest would mean, as I understand it, that a single program would contain two different packages that use the same import path. The go tooling can't support that. It's also rather confusing for anybody looking at the program: the same import path would denote different packages, and how would people know which one they need? It's pretty fundamental in the design that a single import path always denotes a specific package. I haven't looked at the packages, but it sounds like github.com/coreos/go-oidc is making breaking changes without changing to a new major version. Why are they doing that? You say that this happens often: why is that? |
I looked, go-oidc did bump a major version v1 -> v2, though at that point they hadn't yet adopted modules (they're currently on v3 and are using modules). |
Why is that an "insane hack"? ISTM, that's exactly the reason go.mod supports version replacement. Your upstream is relying on broken code. The only reasonable things to do are to get the upstream to fix its dependencies and/or tell your app to ignore the broken dependency until upstream is fixed. "Pinning" just seems like a second, non-orthogonal way to do version replacement. |
How about two different upstreams use two different versions of a module? The two different versions are not compatible (it is really a problem of that module) and each version only satisfies one upstream. In such cases, the two upstreams are both innocent. |
Hey author of go-oidc. Absolutely nothing should be depending on v1. It's been depreciate for years now and has not received any security attention since. Please do not depend on the go-oidc JOSE logic. We would have updated the path when v2 landed, but go modules didn't exist then. That was corrected for v3. |
For context, we nuked the v1 branch years ago (2017) and switched to go-jose to use a better reviewed JOSE implementation. We created the v2 branch before Go modules existed, but kept the path as v3 uses the expected path If you're depending on v1, I think the solution is to upgrade to either v2, v3, or a better JOSE implementation like github.com/square/go-jose. Either way, probably a better issue for https://github.com/coreos/go-oidc/issues than Go? |
Thank you everyone for your contributions! @ericchiang I appreciate your input for go-oidc. You are correct, ideally cloudflared would not depend on go-oidc v1, but I want want to avoid focusing on the specifics of the go-oidc example. I just wanted to share one example of how a go module author could be bitten by transitive dependencies, even though they themselves might depend on two perfectly recent modules. Fundamentally I believe the go ecosystem could provide a better way to avoid these problems by providing more control over the dependency graph, specifically by allowing developers to tell go when a minor semver change is actually breaking, which is why I have opened the discussion here. Let me share a better example below that removes the go-oidc questions. Why replace isn't enough@carlmjohnson good point, let me share another example to illustrate why replace alone is insufficient. package main
import (
"fmt"
"http:https://github.com/containerd/containerd"
"http:https://github.com/hashicorp/nomad/drivers/shared/executor"
)
func main() {
var d *executor.ExecCommand
var c *containerd.Client
fmt.Println(d, c)
}
Here, containerd requires In this case I depend on two post-release modules, but nomad's dependency chains pull in an RC version of runc that is broken by v1.0.2 I am able to work around this like so: This solution works! However it will also downgrade everything to the least common denominator, the version before the breaking change.
The source of this limitation seems mostly to do with the fact that, today, go can only upgrade all or none of the dependencies' runc (as ianlancetaylor correctly points out, they are all a single import path shared in the entire graph) The Solving problems of two modules per import path@ianlancetaylor indeed I think much of the limitation is that go will treat a package by its import path as equivalent and upgrade all nodes in the dependency graph which reference that package name. Perhaps this problem could be mitigated by allowing modules to "alias" or "rename" packages.
The pre-process step could replace the import path of packages in modules with explicit dependencies on the left package with the package on the right. This is similar to how replace works, but it adds the ability to cheat MVS by separating the names in the dependency graph. (also cc @carlmjohnson, another way to explain the difference I am proposing) This could be explicit as I have shown, or an implementation detail of What's interesting is that renaming like this also solves API surface/ABI compat problems, since type names would only match for these versions if the caller and callee both agreed on the version. More examplesI compiled a few more examples by browsing my company chat. Some of these examples are v0.* or experimental packages or -rc, but they are causing real world problems consistently. If I can come up with this many in a few minutes, I imagine there must be many more.
otel was in pre-release so long that there are many packages depending on various v0.* versions
I do not know why people are importing things from the example package but apparently they are, as it broke a real application
|
I think the lengthy discussion in #44550 is relevant here, particularly #44550 (comment). |
Okay, I think I’ve gotten the problem now. It’s just the name “pinning” has nothing to do with what’s really going on here. It’s a name being used because it is used in other version managers, but what is really wanted is a way to pick versions to coexist (“Roman ride” is jargon I heard on a podcast) other than “semantic import versioning”. Basically today, you can use foo 1 and foo 2 at the same time if and only if foo 1 and 2 have different URLs (counting /v2 as part of the URL). This means library authors can decide what coexists and what doesn’t, but often application developers want to make that decision for themselves. It’s a good idea, and probably something should be done in that direction, but I don’t like the name “pin” for it, and as ianlancetaylor pointed out, there are other discussions approaching the question already. |
See #26904. I'm still actively (but slowly) working on that problem space. |
#26904 is allowing multiple names to reference a single module version. I think this is instead asking for a name that will fork a single module to use different versions. If you're going so far as to rename a module and handle coordinating between multiple external modules to match the import name, then there's already a well understood mechanism for it: a hard fork. |
@ianlancetaylor thanks much for that link it was very informative. Specifically @rsc's linked GopherConSG talk was extremely relevent, mostly beginning here where breaks like these are addressed. I know a lot of thought has gone into this design and philosophy and I am extremely respectful of it. The talk makes me think in general this issue was explicitly not addressed owing to the philosophy point of "co-operation" in the ecosystem, which I appreciate. Allow me to respectfully submit my opinion after consuming all of this as a data point for consideration: As Russ points out here here programming is increasingly organized by piecing libraries together, and libraries of libraries. This leads to go.mod files of extreme sizes and complexity. Even with the healthiest culture of cooperation amongst members of the open-source community, this complexity will cause many breaks. Making them more impactful is the fact that many of these breaks will not be discovered by the authors of the related libraries, but by the author of the service that is just trying to piece them together in novel ways. The cost of these breaks to the service author, and the cost to fix them, can be quite high. Take my example of runc and nomad - it takes quite a level of familiarity with go modules to diagnose the issue to begin with - someone just adding containerd to a program would be shocked to find that suddenly nomad doesn't build and the errors seem to have nothing to do with containerd. If they were familiar enough to use go tools to determine the root of the break, they would be distressed to realize that to fix it they have to update nomad's repo to jump 3 major docker versions (17 to 20) and fix dependencies with libraries they may or may not be familiar with and know how to test. And even if they did all of that, they would have to wait on the nomad team to accept the pull request and publish a tagged version. The nomad developers would be right to be very wary and slow to accept such a change - who knows the far reaching risk of upgrading docker this much. @seankhliao your workaround of creating a repo fork and renaming the version to the new repo may indeed work, and it is good that we have some workaround, but this cost is high and I think creates huge fragmentation undermining the community that go seeks to create. Fixing the issue by forking the module does seem like the most straight forward and potentially correct fix, but I really wish there were a better answer than true repo fork. Now that I have said that, hopefully it will be considered and feel free to close if this thread does not add more value. I appreciate all the information I have gotten from it, though I am sad the workarounds remain painful. |
This proposal has been added to the active column of the proposals project |
I added this to active yesterday without reading the entire conversation (with a note to myself to come back and reread it carefully when I could devote more time to it, which I have just done). Upon a close reading, looks like it has resolved itself, so we should move it to retracted at the next proposal review. Ultimately software is only as good as the cooperation of the overall ecosystem. The initial draft of Go modules did include functionality like pin to allow fine-grained control of "X means this in one place but this other thing in a different place", and it quickly became clear that the results would be very difficult for people to understand in practice. Unsurprisingly, I agree with the conclusion that the best temporary fix is a fork of the module containing the stale requirement (in this case, cloudflared/token depending on a 5-year-old go-oidc) combined with a replace to redirect uses of that module with the fork. And the best permanent fix is to work with cloudflared/token's owner to get the dependency updated. |
This proposal has been declined as retracted. |
I would like to have a discussion, or receive guidance, on how to deal with packages which make breaking changes within a minor version which break dependencies via MVS.
Example of the problem
If one were to include the pkg
github.com/cloudflare/cloudflared/token
they would find that its module includesgithub.com/coreos/go-oidc v0.0.0-20171002155002-a93f71fdfe73
If they were also to include
github.com/containerd/containerd
, they would find its module includesk8s.io/[email protected]
which includesgithub.com/coreos/[email protected]+incompatible
My understanding is MVS will select
[email protected]
to be used by all parties; however 2.1.0 has deleted its jose package which cloudflare/token depends on.In other words, I do not know a way, without changing one of my dependencies, to make this program work:
If you would like a contrived example see:
https://github.com/benbuzbee/module_example
Impact of this problem
I have seen this problem occur in my team at least once a month, and it has led to
I have not yet thought of a workaround except for going so far as to compile separate binaries linked by RPC.
If this problem is believed to be rare, or only caused by misuse of the module system, please let me know. I would love to share more examples and be educated in how I am incorrectly using the module system. I have seen this happen very often.
Discussion I would like to have here
First let me start by saying I am quite naïve in areas of compilers and language design, so please forgive me if that naivety is on display here and I welcome growth in this area. I do, however, consider myself a competent developer on a competent team. My team has encountered this problem many times and, as mentioned above, I have yet to come up with a satisfactory solution.
I would like to discuss the severity and frequency of this problem and the advice golang developers would give to work around it. I believe the problem to be severe as it has completely hampered my teams ability to write effective go on many occassions.
I would like to discuss if the belief that developers would correctly update semver was a bet that appears to not be paying off.
And I would like to discuss potential solutions to the problem. Either ones which exist but of which I am not aware, or which do not yet exist but could. I do not believe it is a valid answer that either "developers should more correctly update semver" or that "you can fork the code to separate the packages" or that "you should update your dependencies' dependencies so that they work".
An uneducated proposal to get the discussion started
Again noting that I am naïve in compilers and language design. I would suggest adding to go.mod a
pin
keywordpin github.com/coreos/go-oidc v0.0.0-20171002155002-a93f71fdfe73
Would prevent MVS from updating this module version in the graph, and instead create a fork containing 2 versions of modules go-oidc.
I am aware this may pose certain challenges, such as requiring separate compilation units to prevent name collisions, and interop issues for exported APIs with differing type definitions, but I am not familiar enough to know how severe these challenges may be.
Thank you for listening
The text was updated successfully, but these errors were encountered: