Skip to content
This repository has been archived by the owner on Mar 23, 2021. It is now read-only.

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

Open
myitcv opened this issue Dec 3, 2018 · 9 comments
Labels
question Further information is requested

Comments

@myitcv
Copy link
Owner

myitcv commented Dec 3, 2018

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
Copy link
Owner Author

myitcv commented Dec 3, 2018

Arguments for applying replace/exclude directives:

  • ...

@myitcv
Copy link
Owner Author

myitcv commented Dec 3, 2018

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 (cmd/go: add option to ignore local replace/exclude directives golang/go#24666). Hence the argument that we should apply replace/exclude directives because that's what is tested in CI goes away
  • ...

@myitcv
Copy link
Owner Author

myitcv commented Dec 3, 2018

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
Copy link
Owner Author

myitcv commented Dec 3, 2018

.

@myitcv myitcv added the question Further information is requested label Dec 3, 2018
@bcmills
Copy link

bcmills commented Dec 3, 2018

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
Copy link

bcmills commented Dec 3, 2018

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.)

@myitcv
Copy link
Owner Author

myitcv commented Dec 4, 2018

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.

@bcmills
Copy link

bcmills commented Dec 4, 2018

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.

@myitcv
Copy link
Owner Author

myitcv commented Dec 4, 2018

Per our offline discussion, linking the very relevant golang/go#26904 (comment):

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants