gobin icon indicating copy to clipboard operation
gobin copied to clipboard

To apply replace/exclude directives or not to apply replace/exclude directives?

Open myitcv opened this issue 5 years ago • 9 comments

The discussion in https://go-review.googlesource.com/c/go/+/148517/ touches on a key question we faced when we came to implement gobin: should replace and exclude directives in the main package's module's go.mod be applied when we install said main package?

For gobin, we decided the answer was "yes". But there are some compelling arguments for and against this position, which we try to summarise below.

This description will remain a high-level overview of the problem description. The comments that follow below will be used to summarise arguments "for", "against" and the option space.

Please comment below and we will synthesise/summarise responses.

myitcv avatar Dec 03 '18 09:12 myitcv

Arguments for applying replace/exclude directives:

  • ...

myitcv avatar Dec 03 '18 09:12 myitcv

Arguments against applying replace/exclude directives:

  • If the main package's module is used as a dependency of another module, then, all else being equal, the replace/exclude directives will not be applied. So why should installation of the main package be any different?
  • If a large fraction of binaries can't build (or don't work) using committed, mainline versions of their dependencies, that would mean that the ecosystem is in a very bad state (and we should do something more drastic to fix it, not just hack around it) - to quote @bcmills
  • CI tests for a module should probably be run against the "outside view" of a module (https://github.com/golang/go/issues/24666). Hence the argument that we should apply replace/exclude directives because that's what is tested in CI goes away
  • ...

myitcv avatar Dec 03 '18 09:12 myitcv

Option space

Here we summarise the option space we have with respect to applying (or otherwise) replace/exclude directives, in an approximate order from "do nothing" to "apply everything":

  • ignore replace/exclude directives in the main package's module's go.mod and do not provide an option to do anything else
  • ignore replace/exclude directives in the main package's module's go.mod and allow the user to override certain modules/dependencies via -replace, -exclude and -require flags
  • only apply replace/exclude directives in the main package's module's go.mod if the user asks us to, e.g. via a flag -apply. Note the default here is "apply all" as opposed to the -require/-replace/-exclude flag option which is per dependency
  • apply all replace/exclude directives in the main package's module's go.mod (this is currently what gobin does)
  • ...

myitcv avatar Dec 03 '18 09:12 myitcv

.

myitcv avatar Dec 03 '18 09:12 myitcv

Another argument against:

  • A replacement may be a local or relative file path, and even if the path is relative and in the same repo, the zip files served by a GOPROXY include only the requested module. So we can't always apply the requested replacements, and that being the case, “never apply replacements” is a much simpler policy — to describe, to implement, and to use — than “only apply replacements if they are [available, not file paths, in the same repository, etc.]”.

bcmills avatar Dec 03 '18 13:12 bcmills

One more against:

  • A user may need to supply their own replace directive to fix (or diagnose) an urgent issue in some module on which they depend. If their binaries already rely on some other set of replace directives, they'll have to apply arbitrary patches before they can do their own fix.

That is: requiring your users to build your binary with a specific replace configuration takes away your users' ability to apply their own replacements as needed. (It co-opts what is otherwise a package-user feature to instead be a package-owner feature.)

bcmills avatar Dec 03 '18 22:12 bcmills

Just noting another point as an aide memoire (will update main points in a bit): in the case of a slow-responding maintainer, replace statements are useful for both main packages and libraries. They serve as a (temporary) indicator of "use this for now". The flip of this point is that if you have n dependencies that use X, and some/all of those dependencies say "use Y_n instead" the choice of which Y_n to use becomes more complicated than a simple MVS resolution. That said, the point about slow-moving maintainers remains as the root cause of this.

myitcv avatar Dec 04 '18 17:12 myitcv

In some cases you can work around a slow-moving maintainer by using an actual fork (with its own import path) instead of a replace statement. (When the upstream is fixed, you can replace the fork with a forwarding package.)

That doesn't work for packages that involve global state, but those are already dicey with replace to begin with.

bcmills avatar Dec 04 '18 17:12 bcmills

Per our offline discussion, linking the very relevant https://github.com/golang/go/issues/26904#issuecomment-411873481:

Ideally, I think the long-term solution will be to treat replacements as rewriting the import paths rather than the source code, to allow for precisely this kind of fork-unification behavior.

myitcv avatar Dec 04 '18 21:12 myitcv