golang / go

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

cmd/go: 'go install' should install executables in module mode outside a module #40276

Closed jayconrod closed 4 years ago

jayconrod commented 4 years ago

Authors: Jay Conrod, Daniel Martí

Last Updated: 2020-09-29

Design doc: CL 243077 Comments on the CL are preferred over comments on this issue.

Abstract

Authors of executables need a simple, reliable, consistent way for users to build and install exectuables in module mode without updating module requirements in the current module's go.mod file.

Background

go get is used to download and install executables, but it's also responsible for managing dependencies in go.mod files. This causes confusion and unintended side effects: for example, the command go get golang.org/x/tools/gopls builds and installs gopls. If there's a go.mod file in the current directory or any parent, this command also adds a requirement on the module golang.org/x/tools/gopls, which is usually not intended. When GO111MODULE is not set, go get will also run in GOPATH mode when invoked outside a module.

These problems lead authors to write complex installation commands such as:

(cd $(mktemp -d); GO111MODULE=on go get golang.org/x/tools/gopls)

Proposal

We propose augmenting the go install command to build and install packages at specific versions, regardless of the current module context.

go install golang.org/x/tools/gopls@v0.4.4

To eliminate redundancy and confusion, we also propose deprecating and removing go get functionality for building and installing packages.

Details

The new go install behavior will be enabled when an argument has a version suffix like @latest or @v1.5.2. Currently, go install does not allow version suffixes. When a version suffix is used:

If go install has arguments without version suffixes, its behavior will not change. It will operate in the context of the main module. If run in module mode outside of a module, go install will report an error.

With these restrictions, users can install executables using consistent commands. Authors can provide simple installation instructions without worrying about the user's working directory.

With this change, go install would overlap with go get even more, so we also propose deprecating and removing the ability for go get to install packages.

Examples

# Install a single executable at the latest version
$ go install example.com/cmd/tool@latest

# Install multiple executables at the latest version
$ go install example.com/cmd/...@latest

# Install at a specific version
$ go install example.com/cmd/tool@v1.4.2

Current go install and go get functionality

go install is used for building and installing packages within the context of the main module. go install reports an error when invoked outside of a module or when given arguments with version queries like @latest.

go get is used both for updating module dependencies in go.mod and for building and installing executables. go get also works differently depending on whether it's invoked inside or outside of a module.

These overlapping responsibilities lead to confusion. Ideally, we would have one command (go install) for installing executables and one command (go get) for changing dependencies.

Currently, when go get is invoked outside a module in module mode (with GO111MODULE=on), its primary purpose is to build and install executables. In this configuration, there is no main module, even if only one module provides packages named on the command line. The build list (the set of module versions used in the build) is calculated from requirements in go.mod files of modules providing packages named on the command line. replace or exclude directives from all modules are ignored. Vendor directories are also ignored.

When go get is invoked inside a module, its primary purpose is to update requirements in go.mod. The -d flag is often used, which instructs go get not to build or install packages. Explicit go build or go install commands are often better for installing tools when dependency versions are specified in go.mod and no update is desired. Like other build commands, go get loads the build list from the main module's go.mod file, applying any replace or exclude directives it finds there. replace and exclude directives in other modules' go.mod files are never applied. Vendor directories in the main module and in other modules are ignored; the -mod=vendor flag is not allowed.

The motivation for the current go get behavior was to make usage in module mode similar to usage in GOPATH mode. In GOPATH mode, go get would download repositories for any missing packages into $GOPATH/src, then build and install those packages into $GOPATH/bin or $GOPATH/pkg. go get -u would update repositories to their latest versions. go get -d would download repositories without building packages. In module mode, go get works with requirements in go.mod instead of repositories in $GOPATH/src.

Rationale

Why can't go get clone a git repository and build from there?

In module mode, the go command typically fetches dependencies from a proxy. Modules are distributed as zip files that contain sources for specific module versions. Even when go connects directly to a repository instead of a proxy, it still generates zip files so that builds work consistently no matter how modules are fetched. Those zip files don't contain nested modules or vendor directories.

If go get cloned repositories, it would work very differently from other build commands. That causes several problems:

Why can't vendor directories be used?

Vendor directories are not included in module zip files. Since they're not present when a module is downloaded, there's no way to build with them.

We don't plan to include vendor directories in zip files in the future either. Changing the set of files included in module zip files would break go.sum hashes.

Why can't directory replace directives be used?

For example:

replace example.com/sibling => ../sibling

replace directives with a directory path on the right side can't be used because the directory must be outside the module. These directories can't be present when the module is downloaded, so there's no way to build with them.

Why can't module replace directives be used?

For example:

replace example.com/mod v1.0.0 => example.com/fork v1.0.1-bugfix

It is technically possible to apply these directives. If we did this, we would still want some restrictions. First, an error would be reported if more than one module provided packages named on the command line: we must be able to identify a main module. Second, an error would be reported if any directory replace directives were present: we don't want to introduce a new configuration where some replace directives are applied but others are silently ignored.

However, there are two reasons to avoid applying replace directives at all.

First, applying replace directives would create inconsistency for users inside and outside a module. When a package is built within a module with go build or go install, only replace directives from the main module are applied, not the module providing the package. When a package is built outside a module with go get, no replace directives are applied. If go install applied replace directives from the module providing the package, it would not be consistent with the current behavior of any other build command. To eliminate confusion about whether replace directives are applied, we propose that go install reports errors when encountering them.

Second, if go install applied replace directives, it would take power away from developers that depend on modules that provide tools. For example, suppose the author of a popular code generation tool gogen forks a dependency genutil to add a feature. They add a replace directive pointing to their fork of genutil while waiting for a PR to merge. A user of gogen wants to track the version they use in their go.mod file to ensure everyone on their team uses a consistent version. Unfortunately, they can no longer build gogen with go install because the replace is ignored. The author of gogen might instruct their users to build with go install, but then users can't track the dependency in their go.mod file, and they can't apply their own require and replace directives to upgrade or fix other transitive dependencies. The author of gogen could also instruct their users to copy the replace directive, but this may conflict with other require and replace directives, and it may cause similar problems for users further downstream.

Why report errors instead of ignoring replace?

If go install ignored replace directives, it would be consistent with the current behavior of go get when invoked outside a module. However, in #30515 and related discussions, we found that many developers are surprised by that behavior.

It seems better to be explicit that replace directives are only applied locally within a module during development and not when users build packages from outside the module. We'd like to encourage module authors to release versions of their modules that don't rely on replace directives so that users in other modules may depend on them easily.

If this behavior turns out not to be suitable (for example, authors prefer to keep replace directives in go.mod at release versions and understand that they won't affect users), then we could start ignoring replace directives in the future, matching current go get behavior.

Should go.sum files be checked?

Because there is no main module, go install will not use a go.sum file to authenticate any downloaded module or go.mod file. The go command will still use the checksum database (sum.golang.org) to authenticate downloads, subject to privacy settings. This is consistent with the current behavior of go get: when invoked outside a module, no go.sum file is used.

The new go install command requires that only one module may provide packages named on the command line, so it may be logical to use that module's go.sum file to verify downloads. This avoids a problem in #28802, a related proposal to verify downloads against all go.sum files in dependencies: the build can't be broken by one bad go.sum file in a dependency.

However, using the go.sum from the module named on the command line only provides a marginal security benefit: it lets us authenticate private module dependencies (those not available to the checksum database) when the module on the command line is public. If the module named on the command line is private or if the checksum database isn't used, then we can't authenticate the download of its content (including the go.sum file), and we must trust the proxy. If all dependencies are public, we can authenticate all downloads without go.sum.

Why require a version suffix when outside a module?

If no version suffix were required when go install is invoked outside a module, then the meaning of the command would depend on whether the user's working directory is inside a module. For example:

go install golang.org/x/tools/gopls

When invoked outside of a module, this command would run in GOPATH mode, unless GO111MODULE=on is set. In module mode, it would install the latest version of the executable.

When invoked inside a module, this command would use the main module's go.mod file to determine the versions of the modules needed to build the package.

We currently have a similar problem with go get. Requiring the version suffix makes the meaning of a go install command unambiguous.

Why not a -g flag instead of @latest?

To install the latest version of an executable, the two commands below would be equivalent:

go install -g golang.org/x/tools/gopls
go install golang.org/x/tools/gopls@latest

The -g flag has the advantage of being shorter for a common use case. However, it would only be useful when installing the latest version of a package, since -g would be implied by any version suffix.

The @latest suffix is clearer, and it implies that the command is time-dependent and not reproducible. We prefer it for those reasons.

Compatibility

The go install part of this proposal only applies to commands with version suffixes on each argument. go install reports an error for these, and this proposal does not recommend changing other functionality of go install, so that part of the proposal is backward compatible.

The go get part of this proposal recommends deprecating and removing functionality, so it's certainly not backward compatible. go get -d commands will continue to work without modification though, and eventually, the -d flag can be dropped.

Parts of this proposal are more strict than is technically necessary (for example, requiring one module, forbidding replace directives). We could relax these restrictions without breaking compatibility in the future if it seems expedient. It would be much harder to add restrictions later.

Implementation

An initial implementation of this feature was merged in CL 254365. Please try it out!

Future directions

The behavior with respect to replace directives was discussed extensively before this proposal was written. There are three potential behaviors:

  1. Ignore replace directives in all modules. This would be consistent with other module-aware commands, which only apply replace directives from the main module (defined in the current directory or a parent directory). go install pkg@version ignores the current directory and any go.mod file that might be present, so there is no main module.
  2. Ensure only one module provides packages named on the command line, and treat that module as the main module, applying its module replace directives from it. Report errors for directory replace directives. This is feasible, but it may have wider ecosystem effects; see "Why can't module replace directives be used?" above.
  3. Ensure only one module provides packages named on the command line, and report errors for any replace directives it contains. This is the behavior currently proposed.

Most people involved in this discussion have advocated for either (1) or (2). The behavior in (3) is a compromise. If we find that the behavior in (1) is strictly better than (2) or vice versa, we can switch to that behavior from (3) without an incompatible change. Additionally, (3) eliminates ambiguity about whether replace directives are applied for users and module authors.

Note that applying directory replace directives is not considered here for the reasons in "Why can't directory replace directives be used?".

Appendix: FAQ

Why not apply replace directives from all modules?

In short, replace directives from different modules would conflict, and that would make dependency management harder for most users.

For example, consider a case where two dependencies replace the same module with different forks.

// in example.com/mod/a
replace example.com/mod/c => example.com/fork-a/c v1.0.0

// in example.com/mod/b
replace example.com/mod/c => example.com/fork-b/c v1.0.0

Another conflict would occur where two dependencies pin different versions of the same module.

// in example.com/mod/a
replace example.com/mod/c => example.com/mod/c v1.1.0

// in example.com/mod/b
replace example.com/mod/c => example.com/mod/c v1.2.0

To avoid the possibility of conflict, the go command ignores replace directives in modules other than the main module.

Modules are intended to scale to a large ecosystem, and in order for upgrades to be safe, fast, and predictable, some rules must be followed, like semantic versioning and import compatibility. Not relying on replace is one of these rules.

How can module authors avoid replace?

replace is useful in several situations for local or short-term development, for example:

replace is safe to use in a module that is not depended on by other modules. It's also safe to use in revisions that aren't depended on by other modules.

When would go install be reproducible?

The new go install command will build an executable with the same set of module versions on every invocation if both the following conditions are true:

An executable may not be bit-for-bit reproducible for other reasons. Debugging information will include system paths (unless -trimpath is used). A package may import different packages on different platforms (or may not build at all). The installed Go version and the C toolchain may also affect binary reproducibility.

What happens if a module depends on a newer version of itself?

go install will report an error, as go get already does.

This sometimes happens when two modules depend on each other, and releases are not tagged on the main branch. A command like go get example.com/m@master will resolve @master to a pseudo-version lower than any release version. The go.mod file at that pseudo-version may transitively depend on a newer release version.

go get reports an error in this situation. In general, go get reports an error when command line arguments different versions of the same module, directly or indirectly. go install doesn't support this yet, but this should be one of the conditions checked when running with version suffix arguments.

Appendix: usage of replace directives

In this proposal, go install would report errors for replace directives in the module providing packages named on the command line. go get ignores these, but the behavior may still surprise module authors and users. I've tried to estimate the impact on the existing set of open source modules.

The modules I'm most concerned about are those that use replace as a soft fork while submitting a bug fix to an upstream module; other problems have other solutions that I don't think we need to design for here. Modules using soft fork replacements are about 4% of the the modules with go.mod files I sampled (165 / 4519). This is a small enough set that I think we should move forward with the proposal above.

gopherbot commented 4 years ago

Change https://golang.org/cl/243077 mentions this issue: design: add 40276-go-get-b.md

peebs commented 4 years ago

Thanks for the proposal!

  • go get acts as if no go.mod file is present in the current directory or parent directory.
  • No module will be considered the "main" module.

Would it also be accurate to say that the go.mod being used as the main module is that of the module being built rather than no "main" module? Erroring on replaces is then simply a consequence of the implementation preventing replaces from being respected because we consider the module the "main". If the intention was not to have a main module, one might expect replaces to be ignored just like go get does today outside of a module. Regardless on how go get -b and go get outside a module end up unifying, it seems that the intention of go get -b today is to build reproducible binaries using the packages module as the main module or fail if unable to do so. I know the outcome is the same here, but I think the stated intention helps a user know what to expect of this mode.

  • The -u flag may be used together with -b. As usual, -u upgrades modules providing packages imported directly or indirectly by packages named on the command line

go get is used to download and install executables, but it's also responsible for managing dependencies in go.mod files. This causes confusion and unintended side effects

Since go get -b is a distinct mode of go get that is explicitly is not for managing dependencies of a local go.mod, my instinct here is that -u shouldn't be an option as it further confuses the distinction between using go get for building binaries vs managing dependencies. I at least don't know a scenario where I would want to build a project with updated dependencies and not view and work with the go.mod afterword. For something like that I would always have the repo cloned already as I am now building previously unused combination of dependencies with that project and would probably also want to run tests and maybe downgrade certain dependencies if necessary.

jayconrod commented 4 years ago

Would it also be accurate to say that the go.mod being used as the main module is that of the module being built rather than no "main" module? Erroring on replaces is then simply a consequence of the implementation preventing replaces from being respected because we consider the module the "main". If the intention was not to have a main module, one might expect replaces to be ignored just like go get does today outside of a module. Regardless on how go get -b and go get outside a module end up unifying, it seems that the intention of go get -b today is to build reproducible binaries using the packages module as the main module or fail if unable to do so. I know the outcome is the same here, but I think the stated intention helps a user know what to expect of this mode.

That's not quite the intent: there will actually be no main module.

In an earlier iteration on #30515, I suggested that go get -b would act the same as go get, but it would ignore the go.mod in the current directory: it would ignore replace. Many people found that unintuitive though. It seemed like we eventually had consensus that go get -b should report errors if there's more than one module on the command line or if there are replace directives. That way, you'll build the same binary whether it's: using go build inside the module providing the executable, using go get outside any module, or using go get -b anywhere.

Since go get -b is a distinct mode of go get that is explicitly is not for managing dependencies of a local go.mod, my instinct here is that -u shouldn't be an option as it further confuses the distinction between using go get for building binaries vs managing dependencies. I at least don't know a scenario where I would want to build a project with updated dependencies and not view and work with the go.mod afterword. For something like that I would always have the repo cloned already as I am now building previously unused combination of dependencies with that project and would probably also want to run tests and maybe downgrade certain dependencies if necessary.

There is some use for -u here: it lets you build and install an executable with updated dependencies. go get -u can already do that when run outside a module. I don't expect it's very common, but we get it for free, so I don't think there's a good reason to break it.

peebs commented 4 years ago

had consensus that go get -b should report errors if there's more than one module on the command line or if there are replace directives. That way, you'll build the same binary whether it's: using go build inside the module providing the executable, using go get outside any module, or using go get -b anywhere.

I think we are on the same page here then regardless!

jayconrod commented 4 years ago

Based on discussion in https://groups.google.com/g/golang-tools/c/BRCgqwWLwoY/m/pKuttL9cAwAJ and on Slack, I'd like to take this in a different direction, so I've updated CL 243077 and the copy above.

Instead of adding go get -b, go install would have the proposed functionality when invoked with arguments with a version suffix. For example the command below would install gopls v0.4.4. It could be run from any directory and would ignore the module in the current directory if there is one.

go install golang.org/x/tools/gopls@v0.4.4

go get would no longer build or install packages. It would only be used for changing dependencies.

PTAL and comment on CL 243077 if you have any thoughts. This would be a significant change, but it would be good to have a clear separation of responsibility for go get and go install.

gopherbot commented 4 years ago

Change https://golang.org/cl/254365 mentions this issue: cmd/go: implement 'go install pkg@version'

abursavich commented 4 years ago

@jayconrod

Moving conversation over from #40728, my only sticking point is that I agree with you here: https://github.com/golang/go/issues/30515#issuecomment-469371090 :100:

When called from inside a module, the current behavior in tip is great when referring to a module-local package path, but when referring to a non-local package path go install package should behave the same as go install package@latest, so as to be consistent with other module-related commands.

jayconrod commented 4 years ago

@abursavich There are a couple significant problems with that approach. It would be a break from current go install behavior, and it would be inconsistent with other build commands (go build, go run, and so on).

It is confusing that there will be two variants of go install, but instead of changing the semantics of go install pkg, I think the best way to resolve that would be to remove go install pkg and only allow go install pkg@version. That seems like too disruptive of a change to me, but I'm curious to hear other opinions.

abursavich commented 4 years ago

I think it's important to keep go install pkg for module-local packages, particularly since replace directives are going to be ignored with @version... If you really want to drop the consistency hammer, go get should require an explicit @version to add or update a specific package in module-aware mode.

abursavich commented 4 years ago

I'm just gonna withdraw my comments and step out of the way of progress. Leave it like it is in tip :)

aarzilli commented 4 years ago

Is it too late to roll back the whole "global install command" thing completely? Because I feel like the way it's being impelmented I'd rather not have it in the ecosystem at all. I think the original impetus behind it was that approximately everyone had a README file that said "use go get to install this" and in modules mode it just did a bunch of weird things.

But at this point we've lived without it for 2 years, READMEs have been changed, the community has adapted, do we still need it? And if it isn't going to do ~100% the same thing as git clone && go build is it a net good or a net negative?

A side effect of the way it's being implemented is that it discourages applications from using replace directives, heavily, if it gets adopted by the community as the default way to install applications. It's strange that replace directives are in the language but the tooling discourages from using them. If this is the design we settle in then replace directives should get a deprecation warning in the documentation. But IMHO replace directives are in fact useful, more useful than a global install command, so if they can't be accomodated maybe it's the global install command that should go away.

jayconrod commented 4 years ago

Is it too late to roll back the whole "global install command" thing completely?

This proposal hasn't been formally accepted yet. However, I think there's a pretty firm consensus to move forward with this in 1.16. The lack of a global install command has been a significant usability problem, and many people have asked for something that solves this problem.

But at this point we've lived without it for 2 years, READMEs have been changed, the community has adapted, do we still need it?

I don't think the community has adapted. Many instructions I've seen recommend changing to a directory outside a module, then running go get in either GOPATH mode or with an explicit GO111MODULE=on. I think we need a more convenient, consistent way of installing tools.

And if it isn't going to do ~100% the same thing as git clone && go build is it a net good or a net negative?

In module mode, this would be unexpected behavior. The go command usually fetches dependencies from a proxy. Even in direct mode, it doesn't fetch a whole repository. This is discussed in the design doc.

A side effect of the way it's being implemented is that it discourages applications from using replace directives, heavily, if it gets adopted by the community as the default way to install applications. It's strange that replace directives are in the language but the tooling discourages from using them. If this is the design we settle in then replace directives should get a deprecation warning in the documentation.

replace is not deprecated, and there's no issue with using it during development.

replace is discouraged in release versions that other modules may depend on. replace directives are only applied in the main module; they're ignored by go install and go get in another module or outside modules. This leads to subtle differences in selected versions depending on what command was issued where. We felt that reporting an error was preferable to that ambiguity.

This is also discussed in the design doc and at length in the previous discussion in #30515.

peebs commented 4 years ago

But at this point we've lived without it for 2 years, READMEs have been changed, the community has adapted, do we still need it?

I agree with jayconrad that people haven't really adapted and switching to modules has made things more confusing between go install and go get so I think this is a big win in tooling clarity.

And if it isn't going to do ~100% the same thing as git clone && go build is it a net good or a net negative?

I also think that anything that doesn't build the same binary as git clone && go build is not whats expected. However a really important part of this change is that now go install will refuse to build a module with replace directives and error out rather then ignoring it. Building with replace directives is intuitively what I want with go install but erroring on encountering them is the right thing to do instead. Related to this limitation, go install will be much faster then git clone && go build. Before this change go get would ignore replaces building external binaries so I think this change brings you closer to what you want.

mvdan commented 4 years ago

I want to give extra emphasis to a disadvantage of git clone && [...] that @jayconrod already mentioned. Using the VCS repository directly is:

aarzilli commented 4 years ago

Is it too late to roll back the whole "global install command" thing completely?

This proposal hasn't been formally accepted yet. However, I think there's a pretty firm consensus to move forward with this in 1.16. The lack of a global install command has been a significant usability problem, and many people have asked for something that solves this problem.

I don't disagree with this, in fact I would also like a global install command. The problem that I have is that I dislike the way this is being implemented more than I would like having a global install.

And if it isn't going to do ~100% the same thing as git clone && go build is it a net good or a net negative?

In module mode, this would be unexpected behavior. The go command usually fetches dependencies from a proxy. Even in direct mode, it doesn't fetch a whole repository. This is discussed in the design doc.

I disagree, I think that this would be the expected behavior and that the current behavior of the go command is actually a regression. go get used to do a repository clone.

Address the "Why can't go get clone a git repository and build from there?" list of problems:

It adds complication (and bugs!) to the go command to support a new build mode.

It wouldn't be a new build mode, it would be the same build mode as executing a go build in the root directory of the project. It would add an alternate way to download projects (which used to exist previously).

It creates work for authors, who would need to ensure their programs can be built with both go get and go install.

In reality most code is developed by running go build on a cloned is the normal, inevitable, way of doing development. What adds work for authors is introducing a new way of downloading the code that ignores replace directives and vendor directories. The solution is to also fix go get to build executables as if git clone && go build.

It reduces speed and reliability for users. Modules may be available on a proxy when the original repository is unavailable. Fetching modules from a proxy is roughly 5-7x faster than cloning git repositories.

I concede that this is a valid point, however correctness is more important than performance.

Speaking of unexpected behavior, I think not respecting replace directives is a far more unexpected behavior than the changing details of how a module is downloaded from the internet.

A side effect of the way it's being implemented is that it discourages applications from using replace directives, heavily, if it gets adopted by the community as the default way to install applications. It's strange that replace directives are in the language but the tooling discourages from using them. If this is the design we settle in then replace directives should get a deprecation warning in the documentation.

replace is not deprecated, and there's no issue with using it during development.

replace is discouraged in release versions that other modules may depend on. replace directives are only applied in the main module; they're ignored by go install and go get in another module or outside modules. This leads to subtle differences in selected versions depending on what command was issued where. We felt that reporting an error was preferable to that ambiguity.

When was it decided that replace directives are discouraged from release versions? I've scanned through the vgo blog series and the current Go Modules reference and I can't find anything suggesting this.

This is also discussed in the design doc and at length in the previous discussion in #30515.

It seems to me that the design doc and the previous discussion is simply arguing consistency with pathological behaviors of go get when used to build executables, but I think the point of adding a global install command would be to fix those problems not to further entrench them.

bcmills commented 4 years ago

@aarzilli, if you want users to install using git clone && go build, you can just tell them to do that — adding go install pkg@version doesn't remove or diminish that option; it just also doesn't promote that option by giving it a more concise shorthand.

peebs commented 4 years ago

@aarzilli Just want to reiterate because I share much of your perspective: Currently go get will ignore replace directives. With this change it won't do that anymore (since it will only be for dep management). Now, go install instead will error on replace directives indicating that you should be git cloning this to build it. So it will refuse to build the global binary with an alternative dep graph, which is good!

bcmills commented 4 years ago

When was it decided that replace directives are discouraged from release versions? I've scanned through the vgo blog series and the current Go Modules reference and I can't find anything suggesting this.

See https://research.swtch.com/vgo-principles#cooperation:

[W]hat we want to avoid, is an ecosystem that is fragmented, split into groups of packages that can’t work with each other.

replace directives inherently fragment the ecosystem. They indicate: “use this variant of the selected module instead of the version that everyone else sees”, and replacements are often incompatible with each other. They are a fine tool for experimentation, and in some cases a reasonable short-term option to work around a problem while you negotiate a fix that works for everyone, but they are not in line with the principle of cooperation.

aarzilli commented 4 years ago

@mvdan I don't disagree that it would be nice to use goproxy for this, but can it be? Judging from the list of caveats that accompany this design my impression is that it can't, in its current state.

@peebs Agreed, making go get error on replace directives is a good thing. I just don't like anything else. If the proposal becomes "go get errors when it encounters a replace directive" I'm 100% behind it.

@bcmills If go install becomes the default community meme for installing tools it will be antisocial not to use it. Besides "just don't use it if you don't like it" is a fully general answer to any proposal, if that was the rule of the land almost all proposals could be accepted. Then we could be like C++: everything's there just use the parts you like.

replace directives inherently fragment the ecosystem

I disagree. They do no such thing on an executable, you can have as many incompatible replace directives as you want on different executables without any interoperability problems. And if this is the belief of the go team then the documentation of replace should be changed to reflect that.

tv42 commented 4 years ago

@mvdan https://github.com/golang/go/issues/40276#issuecomment-698414602

Using the VCS repository directly is:

  • slower, since you have to download a lot more (module zips are generally much smaller)

You can use git clone --depth=1 if you don't want the VCS history.

You can also download a zip or tar from github, if you think zips are somehow inherently better than git.

mvdan commented 4 years ago

It's not just about the history. A repository can contain many modules, which is somewhat common. And module zips strip directories and files which aren't part of the set of Go packages.

mvdan commented 4 years ago

@aarzilli pretty much all the major points you bring up are already covered in the current proposal, mainly in the "Rationale" and "FAQ" sections. If you simply disagree with the conclusions, I would almost suggest that you should write a competing proposal, because you are essentially suggesting something entirely different.

We all agree that this problem needs solving, so I don't see a point in stopping or declining this proposal (which has had almost entirely positive feedback) unless there is an equally well thought-out competing proposal that should be considered instead.

jayconrod commented 4 years ago

I disagree. They do no such thing on an executable, you can have as many incompatible replace directives as you want on different executables without any interoperability problems. And if this is the belief of the go team then the documentation of replace should be changed to reflect that.

Part of the rationale of this proposal is that you should be able to install an executable either within a dependent module (applying the constraints and replace directives from that module) or globally (without a main module). The former case is important for tools like code generators or analyzers that might be needed at specific versions or with modifications.

If we honor replace directives in the latter case, module authors will rely more heavily on replace directives, making the former case non-viable. I don't think that's an acceptable tradeoff when we can serve both cases.

We also can't honor directory replace directives at all. When something is downloaded into the module cache, relative and absolute systems from another repository aren't meaningful. Honoring some replace directives but not others adds too much complication.

One more point about downloading from VCS directly: if we completely step outside the module system for downloading code (which is what's being suggested), not only is it much slower (typically 5-7x, even with --depth=1), you're also throwing out safety: there's no go.sum file, so without the checksum database, you have no guarantee that the repository sends you the same bits as anyone else.

aarzilli commented 4 years ago

@mvdan I believe to have refuted the points covered in the proposals, if you disagree with my rebuttal I would ask you to point the error in my way rather than simply stating that I am wrong.

If you simply disagree with the conclusions, I would almost suggest that you should write a competing proposal, because you are essentially suggesting something entirely different.

To my knowledge this standard has never been applied to a proposal. For example the try proposal was rejected without a competing proposal being accepted.

@jayconrod

Part of the rationale of this proposal is that you should be able to install an executable either within a dependent module (applying the constraints and replace directives from that module) or globally (without a main module). The former case is important for tools like code generators or analyzers that might be needed at specific versions or with modifications.

I think this is a bad idea that doesn't solve any problem and introduce new ones of its own. First off it is very strange, almost antithetical, that a global install command should have a behavior that depends on the current directory. Secondly, let's make an example. Let's say I'm building a module that needs a code generator, let's call it goblah. I will have this directive in one of my source files:

//go:generate goblah -some -options

Then I add to go.mod the following:

require github.com/blahworld/goblah v31.0.0

Thinking this is enough to keep track of the goblah tool dependency. But if I simply tell contributors to run go generate this won't work, go generate does not automatically install the dependency and if a version of goblah happens to be on PATH, that's what will be used, regardless of its version or how it was built. People could even end up using odious version 29.0.3.

Soon enough I'll have to write gen.sh script like this:

#!/bin/bash
go-global-install github.com/blahworld/goblah && go generate

(I'm using go-general-install here as a placeholder for whatever the name of a global install will be). This has two problems:

  1. It isn't much better than writing the version inside gen.sh directly
  2. it's going to override the user's choice of what version is globally installed, maybe the user really needed version 29.0.3

What's really needed here is a command that will download an external tool, build it as specified in the go.mod file and execute it. Let's call this command go mod execute-ext-tool, then the go:generate directive could be changed to:

//go:generate go mod execute-ext-tool goblah -some -options

This way you could ensure that your builds are never soiled by goblah on versions other than 31.0.0 while your user can keep living on the 29.0.3 sin.

One more point about downloading from VCS directly: if we completely step outside the module system for downloading code (which is what's being suggested),

That's not what I'm suggesting. I'm suggesting that whatever is implemente should behave as if git clone && go build almost 100% of the time. For example go-global-install could request a special version to goproxy, in this format: github.com/blahworld/goblah@main:v31.0.0, then goproxy would know to prepare and serve an archive of the working directory retrieved from the repository at version 31.0.0. This archive could be checksummed in a similar way and wouldn't conflict with goblah@v31.0.0 due to being in a different namespace.

But the implementation details aren't as important as the principle that it should behave as if git clone && go build almost 100% of the time.

thepudds commented 4 years ago

FWIW, my personal take is I think non-filepath replace directives should be supported for go install cmd/foo@v1.2.3.

A decent percentage of large & frequently used open source Go projects have replace directives in their go.mod files. There are different reasons, but open source is fundamentally messy, and it takes time to upstream fixes or otherwise resolve issues.

If go install cmd/foo@v1.2.3 cannot use non-filepath replace, it is a hint that go install / go get does not scale to large projects, which seems at odds with the overall philosophy of the Go project.

During the replace discussion, I think some people suggested there is a tension between:

  1. People who want to use a tool as published by the tool author (e.g., including replace directives). vs.
  2. People who want to track their dependencies.

During the discussion, some people I think have suggested that not supporting replace in go install cmd/foo@v1.2.3 under the current proposal makes it easier for consumers to track tool dependencies.

That ends up not really being true if a higher percentage of projects resolve this limitation by choosing to still use replace but pick another distribution mode (publishing binaries, install script, git commands, or similar). Those alternative distribution modes don't play well with a tools.go approach.

One can certainly debate what will happen more frequently in terms of how the proposed "no replace" limitation gets resolved: (a) in a way that does plays well with tools.go approach, vs. (b) in a way that does not play well with a tools.go approach (e.g., publishing a binary or similar).

Personally, my guess is the "no replace" limitation pushes more of the largest and most often consumed projects towards (b) at a higher frequency than (a). At a minimum, some publishers choosing (b) eats into the benefit delivered by (a).

Also, whenever someone chooses a separate distribution mode (binaries, install script, git, ...), that also has other negative ecosystem impacts, including they are side-stepping all the "securing the Go ecosystem" work of the checksum DB, and that happens to be occurring at almost the worst point in the delivery chain -- at the point of consumption by the ultimate end user.

peebs commented 4 years ago

@peebs Agreed, making go get error on replace directives is a good thing. I just don't like anything else. If the proposal becomes "go get errors when it encounters a replace directive" I'm 100% behind it.

@aarzilli Just to absolutely make sure we're on the same page (apologies if this is already clear): This is the current proposal (after much debate, me included) except that its called go install. The original proposal for go get -b would have ignored replace, but now go install will error.

It also potentially opens the door up down the road to respect some replace directives without any breaking changes. Since some of the errors would now just do the right thing.

aarzilli commented 4 years ago

@peebs The proposal is:

We propose augmenting the go install command to build and install packages at specific versions, regardless of the current module context.

go install golang.org/x/tools/gopls@v0.4.4

To eliminate redundancy and confusion, we also propose deprecating and removing go get functionality for building and installing packages.

I'm objecting to the way the first part (go install change) is designed. I'm fine with the second part and I'm also fine with changing go install to be a global install command, as long as it's behavior is approximately the same as git clone && go build.

jayconrod commented 4 years ago

@aarzilli

I believe to have refuted the points covered in the proposals, if you disagree with my rebuttal I would ask you to point the error in my way rather than simply stating that I am wrong.

I've referred you back to the design doc in a couple places where your points were covered already.

Other points about expected behavior I think are more of a matter of opinion. We've covered a lot of opinions, and this proposal is a compromise.

I think this is a bad idea that doesn't solve any problem and introduce new ones of its own.

You're not winning me over with this.

First off it is very strange, almost antithetical, that a global install command should have a behavior that depends on the current directory.

It doesn't depend on the current directory. That's the point. If you want to install an executable without modifying the go.mod file in the current directory or changing to another directory, you don't have to change to another directory anymore.

Secondly, let's make an example. Let's say I'm building a module that needs a code generator, let's call it goblah. ...

You wouldn't use the new functionality in this situation. You would use it to install tools that aren't specifically related to any project like gopls or goimports.

Your example mostly works today, and we're trying to keep it working by not encouraging widespread use of replace.

You can use go run to avoid installing an executable directly:

//go:generate go run example.com/tool

Or in a script with non-global go install:

#!/bin/bash

GOBIN=bin go install example.com/tool1 example.com/tool2 # versions come from our go.mod
bin/tool1
bin/tool2

Or with go build:

#!/bin/bash

go build -o bin/ example.com/tool1 example.com/tool2 # versions come from our go.mod
bin/tool1
bin/tool2

These commands work consistently. They use go.mod in the current module to select versions.

What's really needed here is a command that will download an external tool, build it as specified in the go.mod file and execute it. Let's call this command go mod execute-ext-tool, then the go:generate directive could be changed to:

You're describing go run. It works today, though it's a little slow because we don't cache the linked binary. We can improve that.

This proposal is not meant to address that use case.

That's not what I'm suggesting. I'm suggesting that whatever is implemente should behave as if git clone && go build almost 100% of the time. For example go-global-install could request a special version to goproxy, in this format: github.com/blahworld/goblah@main:v31.0.0, then goproxy would know to prepare and serve an archive of the working directory retrieved from the repository at version 31.0.0. This archive could be checksummed in a similar way and wouldn't conflict with goblah@v31.0.0 due to being in a different namespace.

The important difference is that a module zip file contains files from one module, not from other modules in the same repository. Directory replace directives always point outside the current module, most commonly to other modules in the same repository, but sometimes to completely different repositories. That's why we can't honor them.

Even if we packed all modules into a single zip file as you're suggesting, we couldn't make directory replacements work all the time.

aarzilli commented 4 years ago

@jayconrod

It doesn't depend on the current directory. That's the point. If you want to install an executable without modifying the go.mod file in the current directory or changing to another directory, you don't have to change to another directory anymore.

But it does. It's going to look at the go.mod file that's either in the current directory or in a parent directory and look up versions and replaces there.

You wouldn't use the new functionality in this situation. You would use it to install tools that aren't specifically related to any project like gopls or goimports.

Ok, so why is this even mentioned in the proposal? Clearly tracking tool dependencies shouldn't influence the design of a global install command.

The important difference is that a module zip file contains files from one module, not from other modules in the same repository.

Yes. Current module zips are like this. But this doesn't work for a global install command, so a different type of archive needs to be added.

Directory replace directives always point outside the current module, most commonly to other modules in the same repository, but sometimes to completely different repositories. That's why we can't honor them.

Why? Go get has the ability to recursively download dependencies already. You download the @main:version archive, unpack it and run go build inside it. I'm not really sure what's impossible about this.

Even if we packed all modules into a single zip file as you're suggesting, we couldn't make directory replacements work all the time.

Under what circumstances would they not work? Clearly .. replacements that point outside the repository won't work but nobody will expect that to work and that matches the behavior of git clone && go build anyway.

jayconrod commented 4 years ago

@aarzilli

But it does. It's going to look at the go.mod file that's either in the current directory or in a parent directory and look up versions and replaces there.

No. The proposal says:

Ok, so why is this even mentioned in the proposal? Clearly tracking tool dependencies shouldn't influence the design of a global install command.

It's mentioned because it's a design constraint. In introducing a new feature, we don't want to change the ecosystem in a way that breaks existing usage.

The section "Why can't module replace directives be used?" discusses this: it includes an example similar to the one you gave earlier.

Yes. Current module zips are like this. But this doesn't work for a global install command, so a different type of archive needs to be added. ... Under what circumstances would [directory replacements] not work?

Any directory replacement outside the current repository. Any absolute path replacement.

Even if we could honor all replacements though, I don't think we should for the same reasons we're not honoring module replacements: if used widely, they prevent other modules from depending on modules that provide tools.

aarzilli commented 4 years ago

@aarzilli

But it does. It's going to look at the go.mod file that's either in the current directory or in a parent directory and look up versions and replaces there.

No. The proposal says:

* go install runs in module mode, regardless of whether a go.mod file is
  present. If GO111MODULE=off, go install reports an error, similar to
  what go mod download and other module commands do.

* go install acts as if no go.mod file is present in the current directory
  or parent directory.

* No module will be considered the "main" module.

Then I don't understand the paragraphs following the words "However, there are two reasons to avoid applying replace directives at all", it seems to me that they mean nothing.

For example:

applying replace directives would create inconsistency for users inside and outside a module.

It wouldn't because it would behave the same, right?

When a package is built within a module with go build or go install, only replace directives from the main module are applied, not the module providing the package.

What else would you be able to install besides a main package?

When a package is built outside a module with go get, no replace directives are applied.

Why is the current behavior of go get relevant when the impetus behind this change is that it is wrong?

If go install applied replace directives from the module providing the package, it would not be consistent with the current behavior of any other build command.

What about the main build command, go build?

Second, if go install applied replace directives, it would take power away from developers that depend on modules that provide tools.

If their go.mod file is not considered how would this take away power from them?

For example, suppose the author of a popular code generation tool gogen forks a dependency genutil to add a feature. They add a replace directive pointing to their fork of genutil while waiting for a PR to merge.

Where would they add a replace directive if the go.mod isn't considered?

Ok, so why is this even mentioned in the proposal? Clearly tracking tool dependencies shouldn't influence the design of a global install command.

It's mentioned because it's a design constraint. In introducing a new feature, we don't want to change the ecosystem in a way that breaks existing usage.

But you are breaking 10% of the ecosystem with this?

The section "Why can't module replace directives be used?" discusses this: it includes an example similar to the one you gave earlier.

My impression is that the intent was to obey the go.mod file in the current (or parent) directory. If that's not the case I don't think that section actually makes a case against applying the replace directives, if the replace directives of the current go.mod can't be applied then the replace directives of the tool being installed can be applied.

Yes. Current module zips are like this. But this doesn't work for a global install command, so a different type of archive needs to be added. ... Under what circumstances would [directory replacements] not work?

Any directory replacement outside the current repository. Any absolute path replacement.

Nobody would do that and expect it to work. It also wouldn't work with git clone && go build which is what the global install command should match.

Even if we could honor all replacements though, I don't think we should for the same reasons we're not honoring module replacements: if used widely, they prevent other modules from depending on modules that provide tools.

I think the usefulness of replace directives vastly outstrips the usefulness of using an internal package of a tool as a dependency. But if you think this way I think you should propose a change to the documentation to say that replace directives shouldn't appear in published mod files ever. We could see what the community thinks about that rather than burying this change in the implementation of go install.

jayconrod commented 4 years ago

@aarzilli

It wouldn't because it would behave the same, right?

No. When you run go install example.com/A/pkg from the module example.com/Main, it applies require and replace directives from example.com/Main, not example.com/A.

When you run go get example.com/A/pkg in module mode from outside any module, it silently ignores replace directives.

Many people expect that replace directives are applied in either or both cases, but there's no command that works that way.

When you run go install example.com/A/pkg@v1.0.0 (this proposal), it will report an error if the module example.com/A has replace directives. That way, there's no ambiguity.

What else would you be able to install besides a main package?

The main module is the module defined by the go.mod file in the current directory or a parent directory. It's likely different than the module providing the main package in this context.

Why is the current behavior of go get relevant when the impetus behind this change is that it is wrong?

go get is how executables are installed in module mode today, so that's the relevant thing to compare against. It's not wrong, just inconvenient and overloaded. If you want to install an executable globally in module mode, ignoring the module in the current directory, you have to do something like this:

$ (cd /; GO111MODULE=on go get example.com/A/pkg)

What about the main build command, go build?

go build applies replace directives from the module in the current directory, not from anywhere else. Same as go test, go run, go install, go list.

If their go.mod file is not considered how would this take away power from them?

If go install example.com/A/pkg@v1.0.0 applied replace directives from example.com/A, then the author of example.com/A might release a version that didn't build without replace directives.

If the module example.com/B depends on example.com/A, they are now broken unless they copy those replace directives, which may not even be feasible: the replacements from example.com/A might conflict with replacements from example.com/B.

By encouraging replace beyond development within one module, we take power away from module users and give it to module authors.

But you are breaking 10% of the ecosystem with this?

Who does this break? This doesn't change the interpretation of existing go install commands or any other command. It addresses a use case that wasn't adequately covered before.

If a module doesn't build without replace, then there's no build command that works outside that module.

If a module has replace but it can safely be ignored, then commands that work today will keep working (though we may deprecate go get for that purpose).

But why include replace at release versions if it's not needed? It only causes confusion.

I surveyed modules with main packages in "Appendix: usage of replace directives". I think the "soft fork" case is the only one we could realistically address, and that's 4% of modules.

My impression is that the intent was to obey the go.mod file in the current (or parent) directory. If that's not the case I don't think that section actually makes a case against applying the replace directives, if the replace directives of the current go.mod can't be applied then the replace directives of the tool being installed can be applied.

Covered above.

Nobody would do that and expect it to work. It also wouldn't work with git clone && go build which is what the global install command should match.

It sounds like the only advantage of this is that directory replace directives could be applied in modules that replace other modules in the same repository. That's less than 3% of modules with main packages. That's also a problem that's entirely within module authors' power to solve. They can tag releases for each module pointing to the same commit, then tag those commits.

But if you think this way I think you should propose a change to the documentation to say that replace directives shouldn't appear in published mod files ever. We could see what the community thinks about that rather than burying this change in the implementation of go install.

We're in the process of writing more module documentation. Usage of replace is one of topics we have planned and will likely advise caution around release versions.

aarzilli commented 4 years ago

@aarzilli

It wouldn't because it would behave the same, right?

No. When you run go install example.com/A/pkg from the module example.com/Main, it applies require and replace directives from example.com/Main, not example.com/A.

Just to be clear, this is its current behavior, not the new behavior being proposed here?

When you run go get example.com/A/pkg in module mode from outside any module, it silently ignores replace directives.

Many people expect that replace directives are applied in either or both cases, but there's no command that works that way.

When you run go install example.com/A/pkg@v1.0.0 (this proposal), it will report an error if the module example.com/A has replace directives. That way, there's no ambiguity.

But if you run go install example.com/A/pkg from inside example.com/A/pkg does it use require and replace directives from its own go.mod file? Because if it doesn't then go install behavior is inconsistent with go build and go test. I have a better solution go install example.com/A/pkg always applies require and replace directives from example.com/A/pkg, this way go install, go build and go test all behave the same way.

What else would you be able to install besides a main package?

The main module is the module defined by the go.mod file in the current directory or a parent directory. It's likely different than the module providing the main package in this context.

Now I'm confused again. Could you rewrite the proposal to make it clear where you are talking about current behavior of go install and where you are talking about proposed new behavior of go install?

Why is the current behavior of go get relevant when the impetus behind this change is that it is wrong?

go get is how executables are installed in module mode today, so that's the relevant thing to compare against. It's not wrong, just inconvenient and overloaded.

I think we both agree that it's wrong since you are proposing that the new thing should error when it encounters a replace directive.

What about the main build command, go build?

go build applies replace directives from the module in the current directory, not from anywhere else. Same as go test, go run, go install, go list.

What you are saying it is that go build example.com/some/package is going to apply replace/require directives from the current directory, even if the current directory has nothing to do with example/com/some/package. How often do you think this behavior is used? Do you think this is what users expect to happen? What if instead it always applied the replace/require directives of example.com/some/package?

If their go.mod file is not considered how would this take away power from them?

If go install example.com/A/pkg@v1.0.0 applied replace directives from example.com/A, then the author of example.com/A might release a version that didn't build without replace directives.

If the module example.com/B depends on example.com/A, they are now broken unless they copy those replace directives, which may not even be feasible: the replacements from example.com/A might conflict with replacements from example.com/B.

Ok, I get it now. Thank you. I think a better solution is to always apply replace directives from example.com/A, that way the author of example.com/A won't find out about things being broken from its users.

By encouraging replace beyond development within one module, we take power away from module users and give it to module authors.

Ok, but we are talking about tool dependencies here and we have already established that there are better ways to solve that problem that using a global install command to manage local dependencies. So this isn't a problem, the module user, in this case, can use go mod run-external-tool (or whatever it gets called) to build and run the external tool with requires and replaces from the current go.mod file applied.

I think that by erroring on replace directives you also take power away from module users (that have to wait for PR to be merged) and give it to module authors.

But you are breaking 10% of the ecosystem with this?

Who does this break? This doesn't change the interpretation of existing go install commands or any other command. It addresses a use case that wasn't adequately covered before.

If the new global install command becomes the community meme for installing programs then 10% of the existing modules are no longer installable.

If a module doesn't build without replace, then there's no build command that works outside that module.

How about we change this to "all build commands should work outside that module"?

Nobody would do that and expect it to work. It also wouldn't work with git clone && go build which is what the global install command should match.

It sounds like the only advantage of this is that directory replace directives could be applied in modules that replace other modules in the same repository.

No. I think all replace directives that work with git clone && go build should be respected.

peebs commented 4 years ago

@aarzilli I think making these arguments against this proposal is counter productive because its really a big win:

With this change we now have a versioned global install tool that can be trusted to work as expected. This didn't exist in module land or gopath land and now it does. Just use go install pkg@version. It won't work on every project, but that would never be true and many projects rely on make files, other binaries, and/or c libraries anyway. Erroring on replaces is a conservative and correct change for expectations here that leaves room for respecting some replaces down the road without breaking anything.

There is tension between the use cases of global (no local module context) installation of a binary and building an external binary in the context of a local module's dependencies (as @thepudds mentioned). I too think the latter case is so odd as to confuse the tooling, but, this change is a big win and gives a clear path forward for having a distinguished global install command. Its also a big clean up in regards to go get being overloaded since modules.

I think either asking go install pkg@version to use replaces or asking to eliminate the 2nd use case entirely would have to be separate proposals, but stopping this one is a step backward. Its a great advancement/compromise and its provides functionally many have wanted for a very long time while cleaning up the tooling ux.

mvdan commented 4 years ago

First of all, thank you for sharing your concerns, @aarzilli. It shows that you're passionate about the issue at hand, like the rest of us. Having said that, the way you've come to this thread to suddenly push back against the idea comes as a surprise, at least.

I believe to have refuted the points covered in the proposals, if you disagree with my rebuttal I would ask you to point the error in my way rather than simply stating that I am wrong.

Jay is already patiently clarifying your doubts about the proposal, so I won't step in the middle of the conversation.

To my knowledge this standard has never been applied to a proposal. For example the try proposal was rejected without a competing proposal being accepted.

The bar for big language changes is almost impossibly high. There haven't been any big changes since Go 1.0, and we can't even agree if error handling is a problem worth solving. In comparison, cmd/go evolves rapidly (Modules being the prime example), and everyone seems to agree that the current state of "download a program" is broken. So, yes, I still think that the only reasonable way to push back on this proposal is to, well, propose something better.

Please bear in mind that this problem has been in our minds for years; see https://github.com/golang/go/issues/30515 and the even older issues before that. We've also discussed and iterated on this particular proposal for nearly a year in our monthly golang-tools calls. Yes, the current proposal has significant tradeoffs, but like @peebs very well put it, the solution is a step in the right direction and leaves the door open for eventually supporting replace directives in the future.

No. I think all replace directives that work with git clone && go build should be respected.

The proposal tackles this issue head-on, so I'm not sure what else to say on the topic. To be perfectly clear, it seems to me like you have multiple options if you absolutely want replace directives to always be obeyed:

  1. Support this proposal, and once implemented, propose a way to extend the design to support replace directives (since the door is left open)
  2. Make a competing proposal with a better design, to be accepted instead of this one
  3. Insist that we reject this proposal with no other plan in mind

I personally think option 1 is the most sensible; smaller incremental steps, and the first step is already implemented and ready to ship in 1.16 if this proposal is accepted. Option 2 could also be a good option if you actually come up with a better design, but with the big caveat that such a better solution would take at least one or two more cycles to be available to users.

Perhaps I'm misunderstanding you, but you seem to be going for option 3, which would be unfortunate as we would simply not move forward in any direction anytime soon.

thepudds commented 4 years ago

making these arguments against this proposal is counter productive because its really a big win

I think proposals frequently get narrowed or broadened as part of the proposal discussion process (e.g., random recent example is @jayconrod comment in https://github.com/golang/go/issues/40364#issuecomment-663207420).

Personally, I am :thumbsup: on the behavior as currently implemented in tip, but :thumbsdown: on the proposal itself as written.

At the conclusion of #30515 and some contemporaneous discussion in golang-tools calls, my (perhaps incorrect) understanding at the time was that the question of whether or not to support non-directory replace directives was being postponed and being placed outside the scope of this new proposal, and that erroring out was a more conservative approach that allows for future change.

As written, the proposal seems to too strongly indicate that non-directory replace directives won't be supported, at least as I read it, and I worry people will refer back to this proposal (if accepted as written) as deciding the fate of non-directory replace directives.

I would be :thumbsup: on the proposal if it was clearer on the point that this proposal is not deciding the fate of non-directory replace directives, and that it provides reasonable behavior (an error) that keeps more options open for the future.

One way to do that would be expanding this paragraph slightly:

If this behavior turns out not to be suitable (for example, authors prefer to keep replace directives in go.mod at release versions and understand that they won't affect users), then we could start ignoring replace directives in the future, matching current go get behavior.

Finally, I understand there are thorny issues here and competing concerns, and I certainly appreciate the effort that has already gone into creating, discussing, and implementing this proposal. :heart:

mvdan commented 4 years ago

I would be +1 on the proposal if it was clearer on that point.

Aside from it must not contain `replace` or `exclude` and Why can't module replace directives be used?, what do you think could make the proposal clearer?

I worry people will refer back to this proposal (if accepted as written) as deciding the fate of non-directory replace directives.

I hope that won't be the case, as long as we make it clear that we are leaving the door open for supporting replace directives in the future.

aarzilli commented 4 years ago

@mvdan

First of all, thank you for sharing your concerns, @aarzilli. It shows that you're passionate about the issue at hand, like the rest of us. Having said that, the way you've come to this thread to suddenly push back against the idea comes as a surprise, at least.

The old discussion was abruptly closed, I was busy at the time and I sort of lost track of where the discussion went after that.

Yes, the current proposal has significant tradeoffs, but like @peebs very well put it, the solution is a step in the right direction and leaves the door open for eventually supporting replace directives in the future.

I don't see it this way. The proposal makes a strong (if invalid) argument against supporting replaces.

So, yes, I still think that the only reasonable way to push back on this proposal is to, well, propose something better.

I think at this point I have, throughout this discussion. But the merits of this proposal should be discussed in this proposal.

mvdan commented 4 years ago

The old discussion was abruptly closed, I was busy at the time and I sort of lost track of where the discussion went after that.

I disagree on both accounts; @jayconrod mentioned that him and myself started working on this proposal a month before closing the issue, and it did get closed since noone seemed to disagree with moving the discussion to a proposal. We also linked this issue explicitly.

I don't see it this way.

That appears to be more of an optics problem, then. This proposal is not "let's actively deprecate the use of replace directives in tagged releases", and we went to great lengths to make sure that they could be supported in the future. This feedback seems to align with what @thepudds says - do you have any suggestion other than "reject this proposal"?

I think at this point I have, throughout this discussion.

Right, and I think minor adjustments to the proposal are more than reasonable. But if your idea is based on an entirely different premise (that this can't work without supporting replace directives from day one), then I do think it's essentially a different proposal, even if it solves the same core problem.

aarzilli commented 4 years ago

The old discussion was abruptly closed, I was busy at the time and I sort of lost track of where the discussion went after that.

I disagree on both accounts;

You disagre on both accounts? As in: you disagree that I was busy?

@jayconrod mentioned that him and myself started working on this proposal a month before closing the issue, and it did get closed since noone seemed to disagree with moving the discussion to a proposal. We also linked this issue explicitly.

I don't dispute any of this nor do I have any problem with it, but I don't see why this is important: random people comment on proposals all the time.

I don't see it this way.

That appears to be more of an optics problem, then. This proposal is not "let's actively deprecate the use of replace directives in tagged releases", and we went to great lengths to make sure that they could be supported in the future. This feedback seems to align with what @thepudds says - do you have any suggestion other than "reject this proposal"?

I think in my discussion with @jayconrod I have provided some constructive feedback on how to improve this proposal by making the global install command respect replace directives. Let's say we take this proposal, remove everything that mentions the replace directive and instead say "the global install command is going to respect module replace directives, directory replaces can not be supported at the moment due to a limitation of goproxy which will be addressed in a future release" and explain how it plans to support directory replaces. I'd be ok with that.

I think at this point I have, throughout this discussion.

Right, and I think minor adjustments to the proposal are more than reasonable. But if your idea is based on an entirely different premise (that this can't work without supporting replace directives from day one), then I do think it's essentially a different proposal, even if it solves the same core problem.

Let's say I make a competing proposal, the first thing people would write as a comment is "didn't you read #40276? We can't and shouldn't support replace directives" and then we'd be back here.

mvdan commented 4 years ago

You disagre on both accounts? As in: you disagree that I was busy?

I disagree that the proposal was abruptly closed, or that it wasn't clear where the discussion was moving.

random people comment on proposals all the time.

I also left a reply just before the issue was closed. I was the author of that issue, so I'm not sure if I qualify as a random person :)

explain how it plans to support directory replaces

This would be a major change to how the module proxy protocol works, and would affect a lot of other pieces of software including the existing proxies and checksum databases, as well as pkg.go.dev and any other tool interacting with the proxy protocol. I definitely don't think we should be making any promises until a proposal for such a redesign is made and accepted.

"didn't you read #40276? We can't and shouldn't support replace directives"

This would be the main difference with your proposal - you would start from a completely different premise around replace directives. As long as you make that clear, I doubt anyone would close your proposal as a duplicate.

jayconrod commented 4 years ago

@aarzilli

Just to be clear, this is its current behavior, not the new behavior being proposed here?

Yes. Currently, all module commands honor replace and exclude directives in the main module's go.mod file, that is, the go.mod file in the current directory (or any parent). replace and exclude directives in all other go.mod files are ignored.

But if you run go install example.com/A/pkg from inside example.com/A/pkg does it use require and replace directives from its own go.mod file?

Yes. If example.com/A is the main module, then its replace directives apply.

I have a better solution go install example.com/A/pkg always applies require and replace directives from example.com/A/pkg, this way go install, go build and go test all behave the same way.

go build and go test don't work that way. No module command does.

This would completely change the interpretation of replace, and it would break a lot of modules.

Could you rewrite the proposal to make it clear where you are talking about current behavior of go install and where you are talking about proposed new behavior of go install?

The new behavior is specified carefully in the "Proposal" section. The current behavior is explained in "Current go install and go get functionality".

You may also want to read the reference documentation for more clarity on the current behavior:

I think we both agree that it's wrong since you are proposing that the new thing should error when it encounters a replace directive.

I don't agree.

My personal opinion is that go install pkg@version should ignore all replace directives. That would make it behave like go get does when invoked in module mode outside a module. That would let module authors leave replace directives in place in release versions, knowing that they wouldn't affect users.

As I mentioned earlier though, this proposal was a compromise, and we decided to go with something very strict. If we find that's not the right behavior, we can relax it a bit, either by ignoring replace directives or applying module replace directives only.

What you are saying it is that go build example.com/some/package is going to apply replace/require directives from the current directory, even if the current directory has nothing to do with example/com/some/package. How often do you think this behavior is used? Do you think this is what users expect to happen? What if instead it always applied the replace/require directives of example.com/some/package?

No change to go build is proposed here. This is already the current behavior, and it's used all the time.

I certainly hope people expect this. If I run go build example.com/A/pkg from my main module example.com/Main, and the main module replaces example.com/A, I'd expect to build the package from my replacement, not something else.

Ok, but we are talking about tool dependencies here and we have already established that there are better ways to solve that problem that using a global install command to manage local dependencies. So this isn't a problem, the module user, in this case, can use go mod run-external-tool (or whatever it gets called) to build and run the external tool with requires and replaces from the current go.mod file applied.

They can use go run, go build, or go install as they work today. No change is being proposed for this use case, and I don't see any reason to break it.

We are talking about it here because we're specifically trying to avoid breaking it.

I think that by erroring on replace directives you also take power away from module users (that have to wait for PR to be merged) and give it to module authors.

If a user needs to fork and send PRs for one or more modules, then this proposal is not for them. They can run go install (without a version) from their fork.

If you mean they need to wait for the author's PRs to be merged, they already have a problem with go get and all other commands, which ignore replace directives outside the main module.

The difference here is that go install pkg@version would report an error for replace instead of silently ignoring it.

If the new global install command becomes the community meme for installing programs then 10% of the existing modules are no longer installable.

Most (nearly all?) of these issues can be resolved by adjusting go.mod and tagging a new release.

We'll monitor this over time. If we need to change course, there are a couple ways we can do that, but as I've mentioned, we're starting with something strict so that those options are available to us without making a breaking change in the future.

I think in my discussion with @jayconrod I have provided some constructive feedback on how to improve this proposal by making the global install command respect replace directives. Let's say we take this proposal, remove everything that mentions the replace directive and instead say "the global install command is going to respect module replace directives, directory replaces can not be supported at the moment due to a limitation of goproxy which will be addressed in a future release" and explain how it plans to support directory replaces. I'd be ok with that.

You've suggested a large change to the current interpretation of replace directives, which would break lots of existing modules. You've also suggested a significant change to the proxy protocol, which would be expensive to implement across the ecosystem. These are significant drawbacks, and the benefits aren't at all clear to me. It would be difficult for such a proposal to be approved.

jayconrod commented 4 years ago

@thepudds

I'm open to editing the proposal to discuss future modifications. The two possible paths we could take are 1) ignoring replace directives, or 2) applying only module replace directives, still reporting an error if a directory replace directive is present.

It would be good to talk about what kind of data would convince us to take one path or the other.

For (1), we should see how much of a hassle it is for authors to edit go.mod for release versions.

For (2) applying module replace directives, I'd like to be sure that modules that depend on tool-providing modules won't be seriously impacted by additional replace directives. I think we need a better story for tool dependencies in general (for example, an enhanced go run pkg@version or a new kind of require-like directive for tools. That won't happen for 1.16, so I'd like to defer making a decision on that for now.

Anything else on your mind in this area?

jayconrod commented 4 years ago

One more comment for everyone: this feature is implemented at tip in CL 254365. You can try it out with golang.org/dl/gotip. Please experiment and let us know how it works for you.

There's a little more than a month before the 1.16 freeze (November) to make significant changes, and we'll have about a month after that before 1.16beta1 (December) to find and fix bugs.

mvdan commented 4 years ago

It would be good to talk about what kind of data would convince us to take one path or the other.

If 1.16 ships with what's on tip at the moment, perhaps we could run a survey a couple of months after launch to get user input. If we ask the right questions, I think that would probably be the best way to get actionable feedback data.

thepudds commented 4 years ago

Hi @jayconrod, sorry to throw out another topic, but on tip, go install foo@v1.2.3 seems to accept a -modfile argument, but might not do anything with it? That might be a minor bug, but I was thinking about possible -modfile behavior with go install when this proposal was first posted, and curious what you or others might be thinking regarding -modfile...

For now, it could be reasonable to error out or otherwise disallow -modfile, but probably worth at least some brief thinking around what candidate future behavior might be just to see if there are any implications for the current proposal. Some sample questions about possible future behavior:

Example 1:

If you do: go install -modfile=install-gogen.mod example.com/cmd/gogen@v1.2.3

Then immediately do the following (without a version): go install -modfile=install-gogen.mod example.com/cmd/gogen

Do you end up with v1.2.3? (In other words, would require example.com/cmd/gogen v1.2.3 have been written down in install-gogen.md, and would it have been applied in the second invocation)?

Example 2:

If you do: go install -modfile=install-gogen.mod example.com/cmd/gogen@v1.2.3

Then edit the install-gogen.mod file to adjust versions via require or add a replace for more control of dependencies used by gogen, and then do: go install -modfile=install-gogen.mod example.com/cmd/gogen

Would it use the adjusted versions you specified in install-gogen.mod?

Example 3:

-u is not currently supported for go install (though I'm not sure I saw the rationale written down for why). If it was to be supported eventually, if you then did: go install -u=patch -modfile=install-gogen.mod example.com/cmd/gogen@some-branch

And then immediately re-use the modfile: go install -modfile=install-gogen.mod example.com/cmd/gogen@some-branch

Does it use the updated dependencies that were recorded in install-gogen.mod (and thereby providing more reproducibility for -u?)

In any event, could be reasonable to punt by just disallowing it for now, but curious to hear what you might think.

bcmills commented 4 years ago

@thepudds, we plan to reject -modfile; that part still needs to be implemented. (See https://go-review.googlesource.com/c/go/+/254365/comment/11545721_87af54fe/.)

aarzilli commented 4 years ago

@mvdan

You disagre on both accounts? As in: you disagree that I was busy?

I disagree that the proposal was abruptly closed, or that it wasn't clear where the discussion was moving.

I said I lost track, not that it wasn't clear.

random people comment on proposals all the time.

I also left a reply just before the issue was closed. I was the author of that issue, so I'm not sure if I qualify as a random person :)

The random person in question would be me.

explain how it plans to support directory replaces

This would be a major change to how the module proxy protocol works, and would affect a lot of other pieces of software including the existing proxies and checksum databases, as well as pkg.go.dev and any other tool interacting with the proxy protocol. I definitely don't think we should be making any promises until a proposal for such a redesign is made and accepted.

Yes. This proposal would also be that proposal.

aarzilli commented 4 years ago

I have a better solution go install example.com/A/pkg always applies require and replace directives from example.com/A/pkg, this way go install, go build and go test all behave the same way.

go build and go test don't work that way. No module command does.

This would completely change the interpretation of replace, and it would break a lot of modules.

After thinking more about this, I think you are right that this isn't appropriate for most subcommands of cmd/go. When executing go X P where X is build, test, etc and P is a package there are two possibilities:

a) the directives in the current go.mod are applied b) the directives in the module providing P are applied

Both options have merits. What doesn't ever have merit is "use the directives of the module providing P but error if there is a replace directive" and by virtue of the fact that a global install command should not have outcomes that depend on the directory it is run in, it's clear that for the global install command the right behavior is (b).

go get is how executables are installed in module mode today, so that's the relevant thing to compare against. It's not wrong, just inconvenient and overloaded.

I think we both agree that it's wrong since you are proposing that the new thing should error when it encounters a replace directive.

I don't agree.

Then why are you proposing a command with a different behavior and that the current behavior of go get should be deprecated and removed?

My personal opinion is that go install pkg@version should ignore all replace directives. That would make it behave like go get does when invoked in module mode outside a module. That would let module authors leave replace directives in place in release versions, knowing that they wouldn't affect users.

I don't think people add replace directives to go.mod expecting them to be ignored. I think the global install command should respect replace directives so that module authors can leave replace directives in place knowing that they will be respected.

They can use go run, go build, or go install as they work today. No change is being proposed for this use case, and I don't see any reason to break it.

We are talking about it here because we're specifically trying to avoid breaking it.

How would respecting replace directives break it?

I think that by erroring on replace directives you also take power away from module users (that have to wait for PR to be merged) and give it to module authors.

If a user needs to fork and send PRs for one or more modules, then this proposal is not for them. They can run go install (without a version) from their fork.

I'm the author of a tool, I send a PR to one of my dependencies, unfortunately the author has taken a long vacation to Mars and the PR will be merged 6 months from now. In the meantime I fork the dependency and add a temporary replace directive. People try to install my tool using go install but it doesn't work, because replace directives aren't respected. How am I not a module user, and how isn't this taking power away from me and giving it to the Mars vacationer?

If you mean they need to wait for the author's PRs to be merged, they already have a problem with go get and all other commands, which ignore replace directives outside the main module.

Yes, they are going to have a problem with go get. This is a bug. It should be fixed. We're talking about a tool, something that builds to an executable, I'm not sure what other commands you think are relevant.

I think in my discussion with @jayconrod I have provided some constructive feedback on how to improve this proposal by making the global install command respect replace directives. Let's say we take this proposal, remove everything that mentions the replace directive and instead say "the global install command is going to respect module replace directives, directory replaces can not be supported at the moment due to a limitation of goproxy which will be addressed in a future release" and explain how it plans to support directory replaces. I'd be ok with that.

You've suggested a large change to the current interpretation of replace directives, which would break lots of existing modules.

What I have suggested is that replace directives should be respected by a global install command, I don't think this will break anything.

You've also suggested a significant change to the proxy protocol, which would be expensive to implement across the ecosystem. These are significant drawbacks, and the benefits aren't at all clear to me. It would be difficult for such a proposal to be approved.

If the proxy change can not be implemented then cloning a repository and working from there can still be implemented. That would be a shame, but what can you do? Correctness is more important than performance.

aarzilli commented 4 years ago

@jayconrod

I'm open to editing the proposal to discuss future modifications. The two possible paths we could take are 1) ignoring replace directives, or 2) applying only module replace directives, still reporting an error if a directory replace directive is present.

As far as I am concerned I think (2) would be a good start. At least we could focus the discussion on the merits of directory replaces rather than litigating the entire concept of replace.