search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.57k stars 17.61k forks source link

cmd/go: pass information on incompatible versions to consuming modules #26829

Open mattfarina opened 6 years ago

mattfarina commented 6 years ago

Consider the following case where you have an app with a dependency on a module (modA) that has a dependency on another module (modB).

App --> modA --> modB

modB is released with a bug. For the examples sake, the bug causes goroutines to not be garbage collected and the number to continue to grow. This bug has been experienced in a real world application.

The developer of modA realizes this bug exists and sets the minimum version to the last stable release that does not contain the bug.

The developer of the App decides to update the dependencies to the latest releases (e.g., go get -u ./...). Alternatively, the apps developer could import a second dependency (modC) that also depends on modB but at the latest release because they did not catch the bug. Some bugs don't show up on tests but rather in long running situations which is why I choose the goroutine example.

There is currently no way for the author of modA to pass along information that modB has a bad releases. We need a way to communicate that information for those periods where that release is in the wild and one to use.

Existing tools like glide and dep let you say not to use a release. This is idea shows up in most other languages dependency management as well (e.g., !=1.2.3).

An idea was suggested, rather quickly and off the cuff, by @rsc to setup a web service to hold the information on buggy versions that should not be used. There are a few constraints that make this idea difficult:

This is a problem because of two things:

How will the Go toolchain handle this situation?

ucirello commented 6 years ago

Existing tools like glide and dep let you say not to use a release. This is idea shows up in most other languages dependency management as well (e.g., !=1.2.3).

Isn't what the exclude clause is?

mattfarina commented 6 years ago

@ucirello great question. The answer is no according to the docs. Specifically, it says:

exclude and replace directives only operate on the current (“main”) module. exclude and replace directives in modules other than the main module are ignored when building the main module. The replace and exclude statements therefore allow the main module complete control over its own build, without also being subject to complete control by dependencies.

OneOfOne commented 6 years ago

@gopherbot, please add labels modules

ucirello commented 6 years ago

@mattfarina OK - I am not the one equipped to debate on this. But it does seem exclude is the right primitive. You are right in the sense that exclude does not cascade down to the dependency tree, but you could still state the exclusion in the main module. In the latest version you can even see when packages are imported indirectly, and thus take action accordingly.

Again, I am not the one equipped to debate on this, so I could be missing something important.

mattfarina commented 6 years ago

@ucirello You bring up some valid questions that are worth discussing and going deeper on. I'm happy to expand for future readers.

To expand on the example... if you had...

App -> modD -> modA -> modB

If modB had the issue and modA had the exclude it would not automatically be honored. Since App imports modD the author of App may very likely not be aware of the details of modA or modB.

And, since it only works for the current main module, if modA or modB were libraries without a main it would not work for their testing situations, either.

kardianos commented 6 years ago

If this issue is just about marking releases as insecure / buggy, then this is a duplicate of https://github.com/golang/go/issues/24031#issuecomment-407798552 .

If this issue is about code review and diffing changes, then I have a few thoughts. I'd like make a to address some of the issues you mentioned with code review, but I don't it should be part of cmd/go. Maybe we could discuss something over in Athens tracker or offline first.

If this issues is about the lack of a way for go.mod to transitively exclude module versions, then I think this is working as designed as you noted. One tool you could build could read all the mod files from dependencies and inform the user of any excludes constraints they request locally, and optionally include them locally. As you noted, this isn't what you designed in glide and Russ has acknowledged this situation a number of times and it seems fundamental to the principal of the design.

flibustenet commented 6 years ago

When modB has a bug and make a release to correct it, it will increment at least the patch number. Then go list -m -u all will show that, it's then to maintainer of the App to take care of this and go to look if one of theses dependencies need to be upgraded for it.

mattfarina commented 6 years ago

@kardianos This is similar to https://github.com/golang/go/issues/24031#issuecomment-407798552 but not the same.

In this case the latest release has a problem with it. There is no newer release to include information about a previous buggy release. There are numerous reasons this could happen. For example,

  1. While a module is useful the author is generally not a great maintainer and is slow to respond to issues
  2. The module maintainers went on vacation or are otherwise temporarily unavailable
  3. The module maintainers don't see the bug as something they immediately need to issue a release for

The design in https://github.com/golang/go/issues/24031#issuecomment-407798552 is one where a module, in this case modB, describes information about its previous releases. The modules modA and modD in the examples here have no way to communicate information themselves.

The design in https://github.com/golang/go/issues/24031#issuecomment-407798552 would work if module maintainers were always on the ball and got releases for issues out quickly. In practice people simply don't do that all the time which is where the problem comes in.

Don't we need to be fault tolerant of people? If so, how would that work? How can the ecosystem route around an issue in a release where the maintainer(s) of the module aren't handling it?

I believe any solution to this problem should be part of cmd/go because it's a real world situation and people need the default tooling to handle this well.

mattfarina commented 6 years ago

@flibustenet How do people handle a long time before a bug fix release to modB? How do they work around that situation?

ucirello commented 6 years ago

@mattfarina If I understand you correctly, I think it is not a goal to protect against every possible way bad imports can land in your code, rather the idea is to provide the necessary primitives to handle them (in this case, exclude clauses), and data structures on which the ecosystem supporting modules can evolve around.

Although the modules implementation does not address this concern you put forward directly, it does offer the necessary primitives and the go.mod has the necessary information to dig further your dependency tree through tooling.

Thus, if this case you put forward is important, other use cases are also important (like having a tool that indicates that an update is available for both transitive and intransitive dependencies) - but neither are necessarily dealt by cmd/go.

Personally, I am working on a tool that crystalizes into the main's go.dep all the dependencies - even the intransitive ones, mostly giving me control of what lands or not in my dependency tree.

Edit: Added last paragraph.

thepudds commented 6 years ago

I'm pretty sure @mattfarina would have seen all these given his role in glide, etc., but for other people that might land here, briefly adding some more specific URLs to some conversations about related possible future directions (either previously considered or still being considered):

As far as I am aware, there is a strong desire to do something here, but still an open question as to the exact mechanism(s).

It seems like there is very likely a successful path forward that is possible here, but given its significance, this seems like an important area for the community to discuss, analyze, offer possible solutions, etc. (My 2 cents, anyway...)

mattfarina commented 6 years ago

@thepudds I'd like to add some more detail to your possible future directions...

  • declaring deprecated versions as described in the initial vgo blog series

That's already touched on over at https://github.com/golang/go/issues/24031#issuecomment-407798552 and in the blog series. That setup requires the author of modB to take an action. The problem here is when the maintainer of modB isn't taking an action.

  • declaring pair-wise incompatibility between modules in an external system (maybe a renamed godoc.org?) as discussed for example here during the proposal process

As I noted in the original issue, a problem arises here with information disclosure. If Google operates this site they are in the business of many things. A competitor to Google would be leaking the sets of dependencies they use to Google by using Go. That's not going to be acceptable for many.

  • declaring buggy, insecure, or potentially incompatible versions of a module after a release has been published in #24031 (which was mentioned above already)

Another case where the author of modB takes an action.

The case here is when the author of modB does not take an action or has not taken one, yet. The ecosystem needs to route around the issue they face until action is taken.

thepudds commented 6 years ago

@mattfarina Regarding your last point, I had added a comment https://github.com/golang/go/issues/24031#issuecomment-410872522 that at least attempted to outline how the author of modB wouldn’t be required to take an action in this scenario you outlined here?

But I could very easily be off base here…

ianlancetaylor commented 6 years ago

I agree that it would be nice if Google didn't run this service. But even if they did we could design it so that the complete set of information was downloaded to the local system, and then queried. Although it would grow over time I can't imagine that it would ever be very large.

thepudds commented 6 years ago

@mattfarina and more than any other point I might make, I do strongly agree with your points here around this being an extremely important use case to handle well.

mattfarina commented 6 years ago

@ianlancetaylor I've learned a little about this...

we could design it so that the complete set of information was downloaded to the local system, and then queried. Although it would grow over time I can't imagine that it would ever be very large.

This is a good idea and would likely scale in data for some time. There are other problems in a system like this and I'm curious how those would be solved. Here are a couple that immediately come to mind:

flibustenet commented 6 years ago

Even not trolls, how about a situation where modA vX as a part broken by modB vY but in an other situation maybe an App use only a little part of modA and modB and will not be affected with this incompatibility ?

That said, how often do we meet this kind of situation ? And how was it handled ? Do you have some examples with other languages ?

kardianos commented 6 years ago

@mattfarina

It is my understand you are asking "How do we communicate broken releases for transitive dependencies?" Is this correct?

Don't we need to be fault tolerant of people? ...

I don't know what you mean by this. Or rather, in my mind the current design is fault tolerant of people, so what do you mean by this?

I believe any solution to this problem should be part of cmd/go because it's a real world situation and people need the default tooling to handle this well.

When you say this, are you implying that displaying broken releases to the developer should be a go mod X sub-command or that it should be part of the existing go build, go get, go mod sync commands?

You are well aware of the Go modules design. I'm assuming you aren't saying that modA can automatically prevent App from getting the latest modB if requested. I'm assuming you want to determine how to best record and communicate breakage as an advisory notice. But please correct me if I'm wrong so we can be on the same page.

mattfarina commented 6 years ago

@kardianos I think part of the problem is there is wiggle room on some of this an opinions. I will try to limit those to be more clear.

in my mind the current design is fault tolerant of people

I think we have different opinions on this. It's the details below or surrounding this we should discuss to be explicit. I will try to be more clear on that.

We need to deal with lazy developers. Specifically those who do not look at the details of their transitive dependencies and may not look at changes to their explicit ones often after they are included. This includes when they perform updates to them.

The case can be when a chain of lazy developers are involved. For example,

App -> modD -> modA -> modB

In this situation imagine you had the case where App and modD developers were these lazy developers who didn't look at the details. modA may know about the issue and even document it. But, the developers of modD and App may not read the docs or changelogs. They may not even look at a diff either. This way of updating dependencies is common.

I'm assuming you aren't saying that modA can automatically prevent App from getting the latest modB if requested.

I'm attempting to not suggest an implementation. Rather, I am trying to share the details on the need. Others can design a solution. There are no assumptions on how this should be handled in what I'm saying. If anything, I will try to ferret out more details on a proposed solution but those are from others and I'm trying not to propose one.

When you say this, are you implying that displaying broken releases to the developer should be a go mod X sub-command or that it should be part of the existing go build, go get, go mod sync commands?

This is a good question. This is really targeted at the user experience so the information should be displayed in a way that helps to prevent them from getting into a bad state. With that in mind, I would suggest displaying it for commands like go build, go run, and so forth as well as go mod commands. If someone does a go build in CI to generate a binary that's deployed it would be nice to have that in the logs.

The goal would be to aide the end user in situations where they would need it.

bcmills commented 6 years ago

If we suppose that exclude directives in checked-in modules exist for good reason (and aren't just there, say, because someone accidentally leaked debug code into the repo), the proposed go release subcommand could report when a module ends up using some version that is explicitly excluded (or replaced) by one of its dependencies.

Then it suffices for the author of “App” to run go release on their module before cutting a release (and, perhaps, periodically thereafter).

(I've already suggested that in https://github.com/golang/go/issues/26420#issuecomment-408966294.)

mattfarina commented 6 years ago

@bcmills What about apps that do not have releases.

For example, say one has a SaaS and the tip of master is what is running in production. A CI/CD pipeline could even auto-deploy.

If I read #26420 right, it's about releasing software that others can consume. For example, looking for API level changes. In a SaaS (especially a private codebase) these don't matter.

Would it be better to catch these issues at build time since the running build is the things being affected? That way someone running a dev build of a releasable codebase would also have the notifications.

kardianos commented 6 years ago

I'm sure you could have a go release -check-only and CI/CD could run that before go build.

No, it would not be better to catch this during build time. Building is for, well, building and debugging, and running, not going out to the network and checking.

thepudds commented 6 years ago

@mattfarina mentioned elsewhere that this issue #26829 is about how a consumer of modules route around where module authors have yet to fix bugs, and that #24031 is really a separate issue.

At the risk of committing a large faux pas, I am therefore moving some of my commentary from #24031 to here and updating my comments here to more clearly target the #26829 usecase outlined in the opening comment above.

Setting all that aside, using the start of @mattfarina’s example from #26829:

Consider the following case where you have an app with a dependency on a module (modA) that has a dependency on another module (modB).

App --> modA --> modB

modB is released with a bug.

My understanding of part of the goal here in #26829 is how a consumer of modules can easily be notified of an incompatibility where module authors have yet to fix bugs, or in particular using this example:

... which I think is targeting the use case outlined here in #26829?

Mechanism still TBD, but perhaps the mechanism (if I've followed the discussion in #24031) to target the use case here in #26829 might be something like:

But sorry if this is off base or just noise...

edit: made slightly more explicit comment (now in bold above) about where the mechanism discussed in https://github.com/golang/go/issues/24031#issuecomment-407798552 could kick in for the initial problem scenario outlined here in #26829 by @mattfarina.

thepudds commented 6 years ago

In other words, in that scenario outlined in my immediately prior comment:

By checking in and releasing a new modA with a new go.mod, the author of modA is able to communicate to consumers of modA (in this case, the author of 'App') that already released versions of modA are incompatible with the buggy modB, without the author of modB needing to take any action, and without the author of 'App' needing to know they should try to get a new modA (because go tooling automatically checked the latest modA's go.mod to discover the newly published incompatibility information wherein modA's author is declaring modB to be buggy or declaring pair-wise incompatibility between certain versions of modA/modB).

With this mechanism, this incompatibility information could be communicated using the same core mechanisms of releasing code (and hence the information moves with the same strengths and weaknesses of how the code itself is communicated).

In this example, it is the author of modA who both determines there is a problem and communicates the details of the problem, which is good because it is modA that directly depends on modB, and hence the author of modA is much better positioned to determine there is a bug in modB than the less-directly-connected author of 'App' (who is the integrator in this scenario, and who is integrating modB as an indirect dependency and as such the author of 'App' is likely not very aware of exactly how modA interacts with modB. Of all the different actors here, the author of 'App' is likely least well positioned to determine modB is buggy, though the author of 'App' might be the first actor here to experience the pain induced by a buggy modB when 'App' doesn't work). The author of modA and the author of 'App' do not need to rely on any immediate action by the author of the buggy modB, which is good because maintainers can be busy or otherwise might not be able to act for days or weeks or months.

Ultimately, hopefully the story has a happy ending when the buggy modB is eventually fixed and released, but no one needed to wait for that to occur.

In any event, it seems the mechanism outlined in https://github.com/golang/go/issues/24031#issuecomment-407798552 could be used in the scenario outlined by @mattfarina here, but perhaps there is a much better or different solution that would be more appropriate.

edit: couple typos

mattfarina commented 6 years ago

Building is for, well, building and debugging, and running, not going out to the network and checking.

@kardianos This is all brought on by bugs in dependencies. If someone discovers the effects of a bug, does work trying to debug it, and the information is captured but it's not exposed until you use a releaser tool the information isn't being used in a needed context.

When go build is run and a dependency/version isn't available locally won't the tooling go out to the network to get the dependency? Isn't it already hitting the network?

bcmills commented 6 years ago

When go build is run and a dependency/version isn't available locally won't the tooling go out to the network to get the dependency? Isn't it already hitting the network?

Most builds won't require dependencies from outside the local module cache, so either you'd be adding a lot more network latency to typical go build operations, or you'd be providing a false sense of security.

Seems better to have an explicit “yes, check the network for warnings” step, so that you can run it and be confident that there really aren't any warnings (rather than “well, if I added something new maybe it will warn me...”).

mattfarina commented 6 years ago

@bcmills doesn't this depend on implementation detail?

A few thoughts, quickly thrown together and off the top of my head:

Just some ideas around the experience.

kardianos commented 6 years ago

proposal: create a mechanism to report issues discovered in package dependencies

This proposal is similar to https://github.com/golang/go/issues/24031#issuecomment-407798552 but rather then for reporting a module's own bugs, this reports problems with the current module interacting with dependent module versions.

There are roughly two general ways we could solve this:

  1. Read dependencies go.mod files "excludes" directive and report them up to the developer.
  2. Create some type of web service that these issues could be reported to. This could be totally separate from the Go modules download specification or it could be an extension to it.

While I don't have any concrete design specs, I do have a few ideas. They are: ...

Edit: to be clear, I'm trying to re-cap what I understand from the above conversation. This is not my proposal.

kardianos commented 6 years ago

proposal: inform the developer of any stated version exclusion used by dependencies at build time

This relies on the previous proposal XXX for fetching and reporting the list of modules. This pertains to the UX.

Whenever a go build or go install is invoked, make a network request to determine if there has been any updates to all the dependencies in use exclusion list (implementation TBD). Alternatively, make the go build or go install command only periodically perform this check.

One advantage of bundling it in with go build is that CI servers would contain this information in the log output, unless the CI server was isolated from the network or the implementation was periodic then it may or may not perform this check.

Edit: to be clear, I'm trying to re-cap what I understand from the above conversation. This is not my proposal.

bcmills commented 6 years ago

The workflow I'm envisioning for CI tools is, roughly:

go get -u=patch && go release -test=all && go install $PKG

The go get -u=patch step should fetch any updated exclusions and deprecations. The go release -test=all step (or however it ends up spelled) would run the tests and also report any detected incompatibilities.

That doesn't require any annotation mechanism beyond issuing a patch release with an exclusion in the go.mod file, and doesn't require any extra network access during go build or go test.

mattfarina commented 6 years ago

@bcmills If I'm running a SaaS where the internals of the app code are private why do I want to use go release and be interested in things like "detected incompatibilities" for the Go API?

bcmills commented 6 years ago

If I'm running a SaaS where the internals of the app code are private why do I want to use go release and be interested in things like "detected incompatibilities" for the Go API?

If you're running SaaS where the internals of the app code are private, then API incompatibilities should be nonexistent: you should have only binaries and internal packages anyway. You still care about detecting incompatibilities among the other modules you use.

That said, perhaps there should be a way to tell go release to skip the API check entirely, so that you can omit the top-level /internal/ path component.