Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: flags to control changes to go.mod, go.sum #34506

Open
jayconrod opened this issue Sep 24, 2019 · 47 comments

Comments

@jayconrod
Copy link
Contributor

commented Sep 24, 2019

Proposed changes

I propose we add three new flags to go subcommands that deal with modules.

  • -modfile=go.mod - instead of reading and writing go.mod from the current directory or a parent directory, the go command would read and write the specified file. The original go.mod would still determine the module root directory, so this file could be in any directory (perhaps /tmp). It would be an error to use this flag when no actual go.mod file is present.
  • -sumfile=go.sum - instead of reading and writing go.sum from the directory containing go.mod, the go command would read and write the specified file. It would be an error to use this flag when no original go.mod file is present.
  • -g - "global mode" - the go command would behave as if no go.mod file were present.

These flags would be allowed when GO111MODULE is set to on or auto (the current default) and rejected when GO111MODULE is set to off. -modfile and -sumfile both require an actual go.mod to be present, so modules must be enabled when they're used. -g does not require an actual go.mod to be present, and in auto mode, it implies that modules are enabled.

Background

The go command updates go.mod and go.sum after any command that needs to find a module for a package not provided by any module currently in the build list. This ensures reproducibility: if you run the same command twice, it should build (or list or test) the same packages at the same versions, even if new versions have been published since the first invocation.

For example, if you run go build ., and a .go file in the current directory imports example.com/m/pkg which is not provided by any known module, the go command will add a requirement on the latest version of the module example.com/m to go.mod. Future runs of go build . will produce the same result.

While these updates are usually helpful, there are many situations where they're not desirable.

Selected issues

gopls (various issues)

gopls loads information about source files in a workspace using golang.org/x/tools/go/packages, which invokes go list. gopls may also run go list directly. In either case, gopls may trigger changes to go.mod and go.sum. This may be caused by user actions that seem unrelated to building anything, for example, opening a file. go.mod appears to change mysteriously on its own, and users don't realize gopls is triggering it.

It's not usually important that the information gopls loads is reproducible; files it operates on are frequently changing. However, it is important that when it resolves an unknown import path to a module, it doesn't need to do so repeatedly since this can add a lot of latency, especially on slow connections.

gopls could set -modfile and -sumfile to temporary copies of the original go.mod and go.sum. The original go.mod and go.sum would not be modified (until the user explicitly runs a command like go build). Resolved module requirements would stay in the temporary files so they would not need to be resolved again.

#25922 - clarify best practice for tool dependencies

Developers need a way to express module requirements that aren't implied by package imports. This is especially useful for tools invoked by go generate. Authors can add tool requirements to go.mod manually or with go get, but these requirements are erased by go mod tidy.

The current recommendation is to create a tools.go file, tag it with // +build tools, then import main packages of needed tools. tools.go will never be built because of the tag, but go mod tidy will read the imports and preserve the requirements. This feels like a hacky workaround rather than a best practice. It also pushes requirements which may not otherwise be needed on downstream modules.

A better solution would be to keep a separate go.tools.mod file with tool requirements, then point to that with -modfile=go.tools.mod when running commands that require tools.

#26640 - allow go.mod.local to contain replace/exclude lines

This is a feature request to keep some go.mod statements out of source control. It's frequently useful for module authors to check out dependencies and point to them with replace statements for local development and debugging. These statements shouldn't necessarily be exposed to users or other developers on the same project though.

Setting -modfile=go.local.mod and -sumfile=go.local.sum would solve this problem, at least partially. The two files could be copied from the regular go.mod and go.sum files and added to .gitignore. Note however, that these local files are used instead of the regular files, not in addition to, so some synchronization might be required.

#30515 - offer a consistent "global install" command

Developers want to be able to install tools from any directory, regardless of the requirements of the current module. go get tool@version may update the current go.mod, so users need to change to a temporary directory without a go.mod file to run commands like this. Tool authors need to be careful when writing installation instructions because of this.

The -g flag would address this issue. It would tell the go command to run as if it were outside any module. Tool authors could write go get -g tool@latest in their installation instructions: this would install the latest version of the tool, regardless of the current directory.

Note that "missing go.mod" is being reconsidered (#32027), so the actual semantics of -g may change: this issue is just about ignoring the current module.

#33710 - module mode removes concept of global docs

In module mode, go doc example.com/pkg prints documentation for the packages named on the command line at the same version they would be built with. Like go build, go doc may add or update requirements in go.mod. This may be undesirable, especially if you're using the documentation to decide whether you want to depend on a package that is not currently imported.

The -g flag would partially solve this. The current module would be ignored, and "global" documentation would be shown.

Note that go doc does not currently work in "missing go.mod" for packages outside std. #33710 would need to be fixed, but -g would provide a useful way to access that mode.

Other related issues

There are a large number of open issues about unexpected and unwanted go.mod changes. The flags suggested here won't solve all these problems, but they provide useful context.

  • #26977 - go mod why adds a go.mod line
  • #29452 - go list has too many (more than zero) side effects
  • #29869 - 'go list' should not resolve or record modules that are not relevant to the requested output fields
  • #31372 - 'mod verify' does not respect -mod=readonly
  • #31999 - x/tools/gopls: support go.mod files
  • #32027 - rethink "missing go.mod" mode
  • #33326 - go install: don’t fail when go.mod can’t be updated on a read-only system
  • #34450 - go mod download support -readonly
@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

-g - "global mode" - the go command would behave as if no go.mod file were present.

Per #32027 (comment), we'll probably want to make the go command less willing to resolve dependencies when no go.mod file is present.

If we do that, presumably the -g flag will allow the go command to resolve the transitive dependencies of the modules containing the packages listed on the command line. Should -g also allow the go command to resolve missing dependencies found in the import statements of those packages?

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

The original go.mod would still determine the module root directory,

but

These flags would be accepted when GO111MODULE is set to on and rejected when GO111MODULE is set to off.

If GO111MODULE is on but there is no go.mod file above the current working directory, what would we use as the module root?

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Per #32027 (comment), we'll probably want to make the go command less willing to resolve dependencies when no go.mod file is present.

If we do that, presumably the -g flag will allow the go command to resolve the transitive dependencies of the modules containing the packages listed on the command line. Should -g also allow the go command to resolve missing dependencies found in the import statements of those packages?

I think -g should just make the go command do whatever it would do without a go.mod file. So if go run x.go resolves transitively imports and succeeds, go run -g x.go would do the same within a module. But if we change go run x.go to fail, then go run -g x.go would also fail.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

If GO111MODULE is on but there is no go.mod file above the current working directory, what would we use as the module root?

I contradicted myself a bit here. Updated the text. -modfile and -sumfile require an actual go.mod to be present in order to set the module root directory.

@heschik

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

Regarding the file flags: I think having to specify both -modfile and -sumfile is cumbersome. I can't think of any compelling reason to want to share go.sum with any other module, since go mod tidy will throw away the unnecessary lines anyway. So I would suggest that at a minimum, -sumfile be derived from -modfile if it's not set. A more extreme option would be to specify a single prefix, say -modprefix, and add .mod and .sum to it as needed. That may be too strange, though.

This is also a good precedent to set in case there's ever a third file, since nobody will know to override its location before it exists.

@myitcv

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

@jayconrod regarding the "gopls (various issues)" motivation. I'm not entirely clear on what the issue here is and hence why -g/-modfile/other is a solution.

Is the problem that people are not used to cmd/go having side effects in module mode?

Because for me, I would expect that anything I do inside my editor, e.g. adding an import for a package whose module is not in my go.mod, could have side effects on the main module.

Could you give a bit more background?

@mvdan

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

#30515 - offer a consistent "global install" command

While not part of the original issue, the consensus for a while seemed to be that a "global" install should also obey replace directives. At least, this is what @ianthehat said in #30515 (comment). Has the team's position generally changed to not obeying replace directives at all?

I'm also a bit confused by the multiple flags, like @heschik. How about a -modroot=path flag? It would roughly be equivalent to cd path && go <args>.

@myitcv

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Possibly worth reiterating that replace directives come in two forms:

  • directory-based
  • non-directory-based

I think the consensus that @mvdan refers to above was on the latter being applied.

See @mvdan's comment following this one.

@mvdan

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Sorry, I should have clarified. There were initially some comments about applying non-directory replace directives, but the consensus seemed that we either apply all of them or none of them - to not add a third build mode. @ianthehat's comment that I linked seemed to lean towards applying all replace directives.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

Personally, I'm in favour of applying directory replacements only when the source is in a user-controlled directory (as opposed to in global mode where the source is in the module cache). But I feel strongly that installs in global mode should respect other replacements, FWIW.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

@mvdan, @rogpeppe: see #30515 (comment) and #30515 (comment).

Furthermore, given that the proposed semantics of the -g flag are to do whatever we would do if outside of a module, the question of whether or how to apply replace directives seems orthogonal (and a bit off-topic). #31173 is probably a more appropriate venue for that discussion.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

It would be an error to use this flag when no actual go.mod file is present.

We should probably make that even stronger: the module directive in the replacement go.mod file should specify the same module path as the actual go.mod file. (Otherwise, if we're building packages within the module, we will end up resolving what should be module-local imports by looking for an external module.)

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

regarding the "gopls (various issues)" motivation. I'm not entirely clear on what the issue here is and hence why -g/-modfile/other is a solution.

Is the problem that people are not used to cmd/go having side effects in module mode?

Because for me, I would expect that anything I do inside my editor, e.g. adding an import for a package whose module is not in my go.mod, could have side effects on the main module.

@myitcv, @ianthehat and @stamblerre have told me there's no canonical issue for this, but they've had to close a lot of issues as "working as intended", pointing to #29452, and they explain this frequently on Slack.

They mentioned one egregious example where someone in a clean workspace switched branches, then tried to switch back but were unable to because go.mod had uncommitted changes. Their editor (and gopls) was open in the background. It had detected changes in open files, run go list indirectly, and modified go.mod as a result.

-modfile would have made these changes to a temporary go.mod file instead of the go.mod in the main repo. #31999 is about supporting go.mod files in gopls, and while there aren't any details there yet, part of the plan is to provide easy ways to make changes and upgrades. It would be difficult to do that without -modfile.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Regarding the file flags: I think having to specify both -modfile and -sumfile is cumbersome. I can't think of any compelling reason to want to share go.sum with any other module, since go mod tidy will throw away the unnecessary lines anyway. So I would suggest that at a minimum, -sumfile be derived from -modfile if it's not set. A more extreme option would be to specify a single prefix, say -modprefix, and add .mod and .sum to it as needed. That may be too strange, though.

This is also a good precedent to set in case there's ever a third file, since nobody will know to override its location before it exists.

I like the simplicity of just saying what both files should be, but I couldn't actually come up with a scenario where you'd want to set -modfile without -sumfile.

How about this: there's no -sumfile flag. If -modfile is set to M, then the sum file is strings.TrimPrefix(M, ".mod") + ".sum".

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

I'm also a bit confused by the multiple flags, like @heschik. How about a -modroot=path flag? It would roughly be equivalent to cd path && go .

I don't think that solves the same set of problems. -modfile would still use the location of actual go.mod file to set the module root directory, not its argument. -modfile just redirects reads and writes, providing a way to control changes.

@ianthehat

This comment has been minimized.

Copy link

commented Sep 25, 2019

While not part of the original issue, the consensus for a while seemed to be that a "global" install should also obey replace directives. At least, this is what @ianthehat said in #30515 (comment). Has the team's position generally changed to not obeying replace directives at all?

After thinking through all the weird edge cases and trying out a bunch of things, I came to the conclusion that applying replace directives is a confusing and dangerous ball of scary, and the only sane thing to do is not to apply them, in fact the only sane thing to do is to never ever check replace directives in to your repository in the first place.
Part of the benefit of this proposal is it allows for a workflow that makes that a much easier goal, by allowing an alternate go.mod that has the replace directives in it but is not used by default.

I'm also a bit confused by the multiple flags, like @heschik. How about a -modroot=path flag? It would roughly be equivalent to cd path && go <args>.

That would not allow most of the useful patterns (having multiple .mod files in the same directory that you can choose between, or a cache directory with modified .mod files for a bunch of different modules etc)

It would also mean that you have to specify what happens in a bunch of interesting edge cases (thinks like relative paths, are they from the original module or the alternate root?)

I think that specifying the .sum file would/should be incredibly rare, leaving it as the original one would be fine for the majority of use cases.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

@mvdan @rogpeppe

On replacements:

  1. We should apply all of them or none of them. A third mode would cause confusion. (#30515 (comment)).
  2. In order to apply file path replacements, we'd need to check out a whole repo. That's very different from what go get does now (especially when a proxy is in use), and there are many ways it could fail. We also won't get reproducible builds if replacement paths point outside the repository.

Personally, I think the downsides of (2) outweigh the benefits, and it's better to have something very simple like -g.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

How about this: there's no -sumfile flag. If -modfile is set to M, then the sum file is strings.TrimPrefix(M, ".mod") + ".sum".

Maybe split the difference? If the original is go.mod and -modfile is M.mod, first check whether M.sum exists: if so, use (and update) it, and otherwise use (and update) the original go.sum.

As far as I can tell, that handles both the tools.go.mod case and the /tmp/some-gopath-dir/go.{mod,sum} case.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Maybe split the difference? If the original is go.mod and -modfile is M.mod, first check whether M.sum exists: if so, use (and update) it, and otherwise use (and update) the original go.sum.

As far as I can tell, that handles both the tools.go.mod case and the /tmp/some-gopath-dir/go.{mod,sum} case.

@bcmils Could work, but it seems a little subtle. If a module only depends on std and has no go.sum file, a naïve tool might copy go.mod to /tmp without creating an empty go.sum file. It would then unexpectedly modify the temp go.mod and the original go.sum.

@myitcv

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

@jayconrod re #34506 (comment)

-modfile would have made these changes to a temporary go.mod file instead of the go.mod in the main repo

But what about the situations where you do want gopls to have these side effects? i.e. adding an import for a package whose module is not in my go.mod.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

I think -g should just make the go command do whatever it would do without a go.mod file.

Without a go.mod file, go get example.com/some/really/old/tool (that is, some tool without its own go.mod file) should probably fail, rather than re-resolving the latest transitive imports of that tool and discarding the result.

On the other hand, I think it is probably reasonable to expect go get -g example.com/some/really/old/tool to succeed, even if it is consistently slow.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

But what about the situations where you do want gopls to have these side effects? i.e. adding an import for a package whose module is not in my go.mod.

@myitcv It would be up to gopls and the editor to provide a sensible way to do that. Perhaps it could show a warning that go.mod is incomplete and provide a quick fix to add the missing requirement. Or it could update go.mod when a file is saved if it adds new imports.

There are situations where modifying go.mod is not wanted, and -modfile provides gopls with a knob that controls when those modifications happen.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Without a go.mod file, go get example.com/some/really/old/tool (that is, some tool without its own go.mod file) should probably fail, rather than re-resolving the latest transitive imports of that tool and discarding the result.

On the other hand, I think it is probably reasonable to expect go get -g example.com/some/really/old/tool to succeed, even if it is consistently slow.

@bcmills There's a lot of nuance here. Assuming no go.mod is present, should go get example.com/tool when example.com/tool does have a go.mod file? What if the go.mod file is missing some requirements?

Should -g have this variation in behavior for go get only or for other commands, too?

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Assuming no go.mod is present, should go get example.com/tool when example.com/tool does have a go.mod file? What if the go.mod file is missing some requirements?

I'm not sure. Probably:

  • Without -g and outside of a module:

    go get should resolve the latest version of the requested packages, compute the resulting build list, try building the requested packages, and error out if any of the import statements it encounters cannot be resolved using that build list.

    That way, the number of latest lookups (and, potentially, fetches) is O(N) with the arguments passed to go get, and on successive runs everything else can be resolved using the local module cache.

  • Without -g and inside of a module:

    go get should resolve the latest version of the requested packages, add them to the main module's build list, and resolve and add any additional dependencies as needed during the build, recording the result in the main module's go.mod file.

    That still keeps the number of latest lookups down to O(N) with the arguments in the steady state, since any subsequent go get will use the versions recorded in the go.mod file (which will be in the local cache).

  • With -g and inside of a module:

    The user has explicitly told us not to record the result of version resolution.

    If the latest version of the module(s) containing the requested packages does not specify all needed dependencies, we have two possible options:

    1. We could reject the go get request and ask the user to retry within a module. Most likely they'll just set -modfile=$(mktemp) instead of -g and run the command again, in which case we've added no value.

    2. We could resolve the full import graph and throw away the results. But I think in most cases the user won't be running go get -g on the same module repeatedly, so that's probably fine. (In contrast, the user may reasonably run go run foo.go on the same source file repeatedly, so it's more important not to discard dependency information there.)

  • With -g and outside of a module:

    It's the same situation as inside of a module, but instead of -modfile=$(mktemp), they'll likely do pushd $(mktemp -d); go mod init ugh; go get […]; rm -r .; popd.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Should -g have this variation in behavior for go get only or for other commands, too?

Do we need to support the -g flag at all for other commands? I was assuming it would be a get-specific flag.

@myitcv

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

It would be up to gopls and the editor to provide a sensible way to do that. Perhaps it could show a warning that go.mod is incomplete and provide a quick fix to add the missing requirement. Or it could update go.mod when a file is saved if it adds new imports.

This creates too much of a disconnect with the other go commands to my mind. By way of analogy, this would be equivalent to go test failing because go.mod is incomplete, and then requiring the user to run some specific command to fix it before again running go test.

Hence @bcmills's comment makes sense to me:

Do we need to support the -g flag at all for other commands? I was assuming it would be a get-specific flag.

Updated: @bcmills was talking about -g and @jayconrod was referring to -modfile

@rodrigc

This comment has been minimized.

Copy link

commented Sep 27, 2019

@jayconrod will your proposed change still be compatible with the advice given at:

https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module

(I'm guessing yes, but just want to make sure).

Some projects have migrated to that approach such as this:

https://github.com/atlassian/smith/blob/master/tools.go

But there are other projects which still do go get in a Makefile inside the module directory to fetch tools.

@ianthehat

This comment has been minimized.

Copy link

commented Sep 27, 2019

The proposal has no effect on that approach, but...

That approach is a horrible hack with all sorts of horrendous caveats and problems. I would argue strongly that it was okay as a temporary measure in go1.11 but it is already bad advice today and would become much worse advice when the features in this proposal are available.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

That approach is a horrible hack with all sorts of horrendous caveats and problems.

[citation needed]

#33926 is the only issue I'm aware of describing a concrete problem with the // +build tools approach, and the concrete problem cited there (#33926 (comment), an incompatibility between gopls and one of its dependencies) was due to a breaking change made in one of the tool's non-semantically-versioned dependencies.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

Some notes from a few out-of-band conversations with @bcmills @ianthehat and @myitcv:

-modfile

  • In evaluating this feature request, it would be helpful to have a better understanding of how gopls would use it. Clearly, there are situations where it should and should not modify go.mod. Some extra logic will be required to manage and synchronize both the actual and temporary go.mod files.
    • @ianthehat @stamblerre Could you comment here with ideas on when this feature would be used and how that would work in gopls? What would the user experience look like, ideally? Is any of this a v1.0.0 blocker?
  • It seems like most of the time, we want a separate file that is used in addition to the actual go.mod, not instead of. gopls needs to manage additional requirements. For local replacements, we still want the original set of requirements. For tool dependencies, we want additional requirements.
    • Having -modfile always augment the actual go.mod seems like it loses flexibility though. What if you actually want something different?
    • An idea: we could add a statement (say include) that pulls in statements from another go.mod file.
    • A statement like this would only work in the main go.mod file though. Like replace and exclude, it would be ignored in remote modules. It would add a lot of complication to the proxy protocol and sumdb logic to support include in general.
    • Maybe it could be done as part of the flag? -modfile=+go.local.mod or -modfile=go.local.mod,go.mod would both mean "read both go.mod and go.local.mod and write changes to go.local.mod".

-g

  • Note that -modfile and -g are separate features that solve different issues, though they are both related to go.mod modifications. I probably should have filed two issues for these though.
  • It's hard to evaluate -g because we haven't decided what "missing go.mod" mode should do (#32027).
    • At the moment, @bcmills and I are thinking that it should be possible to build packages in std and ad-hoc packages comprising .go files listed on the command line that only import packages in std. Any build, run, list, test, doc command that requires resolving an import path to a module would fail outside of a module (I'll post a comment on that issue).
    • If we go with that solution, there's no point in those commands supporting -g, since they won't usually be able to do anything useful.
    • So -g may as well be a special flag for go get that is tailored to fix #30515.
  • We haven't been able to agree on a solution for #30515 though. The question is what to do with replace directives.
    • replace directives should be used for 1) temporarily forking a module to depend on a fix or feature that hasn't been released upstream, and 2) local development, especially of changes that affect multiple modules. In the context of #30515, only (1) is important.
    • Technically, we could apply replace directives that replace modules with other modules. However, we cannot safely apply replace directives that replace modules with directories, since paths may point outside the module or outside the current repository. See #30515 (comment) and the comments below. We don't want to introduce a "third mode" where some replace directives are applied but not others, since this adds complication for module authors (increased test surface) and in the go command (lots of opportunity for bugs).
    • An idea: go get -g could apply module replace directives in the module providing packages named on the command line. It would reject file replace directives with an error. So replace directives would never be partially applied.
    • gorelease could warn module authors if they are about to publish a version that includes file replace directives in the main go.mod. File replace directives could be kept in a separate go.local.mod, referenced with -modfile.
    • This seems like a lot of pieces to fit together, so perhaps initially for 1.14: go get -g rejects all replace directives with an error. We could enable module replace directives later on without breaking anyone.
@eliasnaur

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

@myitcv

But what about the situations where you do want gopls to have these side effects? i.e. adding an import for a package whose module is not in my go.mod.

@jayconrod

Some notes from a few out-of-band conversations with @bcmills @ianthehat and @myitcv:

-modfile

* In evaluating this feature request, it would be helpful to have a better understanding of how gopls would use it. Clearly, there are situations where it should and should not modify go.mod. Some extra logic will be required to manage and synchronize both the actual and temporary go.mod files.
  
  * @ianthehat @stamblerre Could you comment here with ideas on when this feature would be used and how that would work in gopls? What would the user experience look like, ideally? Is any of this a v1.0.0 blocker?

I don't think it's obvious gopls should ever modify my go.mod and go.sum. To me, gopls is an extension of my editor, and my editor should not change any files I didn't ask it to. To me, it's tedious to constantly have to keep an eye out for irrelevant go.mod/go.sum changes before checking in new code.

Of course, if I run go commands from inside my editor (either directly or indirectly through a "run", "build" or similar editor command), then my go.mod/go.sum files are subject to change. But in those cases I'm no longer editing, I'm using the editor as a convenient command line shell.

So in conclusion, I see gopls always using -modfile pointing to its own go.mod file as the right decision.

@myitcv

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

@eliasnaur so what about when go test modifies your go.{mod,sum} - is that also an error from your perspective? Because in that instance you haven't specifically asked it to modify your go.{mod,sum}, you've simply asked to run some tests. go mod edit is the specific command for editing go.mod. If go test did not operate in this way, having to run an additional command (which would require some manual effort) to add the relevant require directives would be tiresome to my mind because that's almost always what I want to do.

Per @jayconrod's request to @ianthehat and @stamblerre above, I think we need to establish what the workflow here would be from a user's perspective before doing something that to my mind goes against what the go tool is already doing.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

Another use case for this is go-fuzz. go-fuzz has to augment the source tree with its own dependencies during compilation, but there is ~zero value to the user in having the module containing those dependencies be present in their go.mod/go.sum. (cc @thepudds)

@zikaeroh

This comment has been minimized.

Copy link

commented Oct 7, 2019

To offer some input from a heavy gopls user: I'm very happy to let gopls modify my go.mod file as it pleases, since that means I can paste in import paths and have them actually resolve, even if I have to do cleanup duty later if go.mod contains something I didn't expect.

If it didn't have this ability, they'd break, and then more things end up breaking when I have to go run go in my terminal and gopls doesn't see that something has changed (leading to some bad state only a reload can fix, which is already far too common). I can see a future where gopls wouldn't have the ability to modify go.mod, but it'd really need to better handle watching for changes, and would be kinda disappointing to go from the workflow of "paste in an import and it just works" back to the old workflow of needing to make sure that everything is there before trying to use it.

@eliasnaur

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

@eliasnaur so what about when go test modifies your go.{mod,sum} - is that also an error from your perspective?

No. I meant to cover go test by:

Of course, if I run go commands from inside my editor (either directly or indirectly through a "run", "build" or similar editor command), then my go.mod/go.sum files are subject to change. But in those cases I'm no longer editing, I'm using the editor as a convenient command line shell.

In other words, go commands modify my go.* files, as they should. I'm only talking about gopls and editing in general, which shouldn't.

To offer some input from a heavy gopls user: I'm very happy to let gopls modify my go.mod file as it pleases, since that means I can paste in import paths and have them actually resolve, even if I have to do cleanup duty later if go.mod contains something I didn't expect.

I agree, and that's why I haven't complained about gopls before this proposal. With -modfile, gopls can keep a private go.mod file and operate exactly as if it had modified your go.mod file.

@myitcv

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

In other words, go commands modify my go.* files, as they should

My point was that for some people go test and other build commands (e.g. go list, go build etc) should not have these side effects: #29452. i.e. an explicit go mod edit commands should be required before running go test etc.

I'm not arguing for that position, just using it to point out that many people have normalised the fact that cmd/go build commands modify go.{mod,sum} in just the same way that I've normalised gopls modifying go.{mod,sum}.

With -modfile, gopls can keep a private go.mod file and operate exactly as if it had modified your go.mod file.

What would the workflow look like when I do want gopls to modify my go.{mod,sum}?

Having a second go.{mod,sum} introduces a second source of truth; the "real" one is used by cmd/go, the "fake" is used by gopls. How do we ensure they remain in sync? How do we point out conflicts?

@eliasnaur

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

In other words, go commands modify my go.* files, as they should

My point was that for some people go test and other build commands (e.g. go list, go build etc) should not have these side effects: #29452. i.e. an explicit go mod edit commands should be required before running go test etc.

I'm not arguing for that position, just using it to point out that many people have normalised the fact that cmd/go build commands modify go.{mod,sum} in just the same way that I've normalised gopls modifying go.{mod,sum}.

Sure, I've also normalized that behaviour, by monitoring my go.* files like a hawk :)

Note that #29452 seems to make a distinction between building commands such as go test, go install, go build and query commands such as go list, go mod. So even with a hypothetical fix to #29452, your go test would still do what you expect.

With -modfile, gopls can keep a private go.mod file and operate exactly as if it had modified your go.mod file.

What would the workflow look like when I do want gopls to modify my go.{mod,sum}?

May I ask when and why you want gopls to modify you go.* files? And for each of the cases you do want its modifications, why aren't your inevitable go test, go run, go install or even go mod tidy good enough checkpoinst for actually changing your go.* files?

The only legitimate case I can see is for adding imports that you're going to keep, but how would gopls know the keepers from your spelling mistakes? I can't count the number of times gopls has added irrelevant dependencies to my go.* files just because I code completed or misspelled a package.

Having a second go.{mod,sum} introduces a second source of truth; the "real" one is used by cmd/go, the "fake" is used by gopls. How do we ensure they remain in sync? How do we point out conflicts?

My go.* files are the source of truth, while gopls' own files are for caching queries and pleasing its underlying go commands. If conflicts occur, its conflicting (or all?) changes to its internal go.* files should be wiped out.

@myitcv

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Note that #29452 seems to make a distinction between building commands such as go test, go install, go build and query commands such as go list, go mod. So even with a hypothetical fix to #29452, your go test would still do what you expect.

It's worth noting that go list is a build command; it takes build flags. go mod edit is I think the only go mod subcommand that is literally concerned with mechanically editing go.mod. All the other subcommands will cause a full resolution of dependencies that might result in changes to go.{mod,sum}.

The reason I bring up the go test example is because in another world we could easily have decided that go test should fail if go.{mod,sum} didn't contain the relevant entries for go test to proceed. In such a scenario, a go mod edit or similar mechanical update would have been required as a separate step before running go test again. As it is, go test does as little as it needs to to go.{mod,sum} for the build configuration under test. This feels natural to me, because it avoids the painful error message "you need to run X before continuing".

May I ask when and why you want gopls to modify you go.* files?

Because I want the type checking and analysis that gopls carries out to proceed in much the same way that go test does without me needing to intervene. Similarly, when gopls gains the power to help with things like code generation or other commands, I don't want to be having to manually intervene to run go mod edit to fix my go.mod or click "accept" on a code suggestion to add something to my go.mod.

The only legitimate case I can see is for adding imports that you're going to keep, but how would gopls know the keepers from your spelling mistakes?

I notice you skilfully sidestepped my question on the workflow of when I do want gopls to update my go.{mod,sum} 😉 I'm guessing however you want to manually "copy" the "right" changes from gopls's copy to the real copy?

I can't count the number of times gopls has added irrelevant dependencies to my go.* files just because I code completed or misspelled a package.

This could happen in any scenario: even with go test for example. go mod tidy is your friend here, and I think Rebecca added support for a formatting of go.mod files (which amounts to a go mod tidy from memory). I don't think manually copying/accepting the "right" changes is necessary here, the tools can do the work for us.

If conflicts occur, its conflicting (or all?) changes to its internal go.* files should be wiped out.

I'm certainly not clear what this conflict resolution algorithm would look like: can you sketch it out ?

@eliasnaur

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

Note that #29452 seems to make a distinction between building commands such as go test, go install, go build and query commands such as go list, go mod. So even with a hypothetical fix to #29452, your go test would still do what you expect.

It's worth noting that go list is a build command; it takes build flags. go mod edit is I think the only go mod subcommand that is literally concerned with mechanically editing go.mod. All the other subcommands will cause a full resolution of dependencies that might result in changes to go.{mod,sum}.

I suppose that's what #29452 is about: go list is a build command, but feels like a query command.

However, in the context of gopls its use of go list is an implementation detail.

May I ask when and why you want gopls to modify you go.* files?

Because I want the type checking and analysis that gopls carries out to proceed in much the same way that go test does without me needing to intervene. Similarly, when gopls gains the power to help with things like code generation or other commands, I don't want to be having to manually intervene to run go mod edit to fix my go.mod or click "accept" on a code suggestion to add something to my go.mod.

That's a great example. I want to consent to go.mod changes because adding a dependency is not something I do lightly. From Rus Cox' Our Software Dependency Problem:

"A package, for this discussion, is code you download from the internet. Adding a package as a dependency outsources the work of developing that code—designing, writing, testing, debugging, and maintaining—to someone else on the internet, someone you often don’t know. By using that code, you are exposing your own program to all the failures and flaws in the dependency. Your program’s execution now literally depends on code downloaded from this stranger on the internet. Presented this way, it sounds incredibly unsafe. Why would anyone do this?"

In other words, completing or auto-generating code for me is ok to do more or less automatically, but or adding or changing my dependencies is not.

I notice you skilfully sidestepped my question on the workflow of when I do want gopls to update my go.{mod,sum} wink I'm guessing however you want to manually "copy" the "right" changes from gopls's copy to the real copy?

Whenever you consent to the changes. Whether that is a go build, go test on the command line, or clicking "run" or "accept" in a IDE. And perhaps there is a setting for "I accept all changes" if you really want to accept whatever gopls gives you during edits.

I can't count the number of times gopls has added irrelevant dependencies to my go.* files just because I code completed or misspelled a package.

This could happen in any scenario: even with go test for example. go mod tidy is your friend here, and I think Rebecca added support for a formatting of go.mod files (which amounts to a go mod tidy from memory). I don't think manually copying/accepting the "right" changes is necessary here, the tools can do the work for us.

Yes, but I consider go test and friends consenting to changes. It's not perfect (#29452) and I would personally have GOFLAGS=-mod=readonly if govim supported it.

If conflicts occur, its conflicting (or all?) changes to its internal go.* files should be wiped out.

I'm certainly not clear what this conflict resolution algorithm would look like: can you sketch it out ?

Keep an internal copy of go.* and use -modfile to point to them. Whenever the original go.* files changes, replace the copy.

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

I'm curious as to whether #34829 would satisfy the gopls use-case less invasively. How important is it to resolve dependencies in uncommitted code, vs. tolerating (and possibly suppressing) errors for unresolved dependencies?

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2019

Contemporaneous notes from a conversation with @ianthehat and @stamblerre:

  • When you add an import, either explicitly or by completion, gopls should update go.mod or offer to do so. The exact behavior needs evaluation.
  • If changes to go.mod don't happen automatically, they'd probably be suggested Quick Fixes on imports that aren't provided by any module in the build list.
  • If changes to go.mod happen automatically, they should happen when the file is saved, probably not as soon as the import is added (right after a completion).
  • A frequent problem: a user opens a file that transitively imports a package not provided by any module in the build list. gopls (via go list) adds a requirement for a new module. The user hasn't edited anything yet and is surprised that go.mod has changed.
  • How far could we get if go list -e -mod=readonly worked better (#34829)?
    • It doesn't let us type check, since we can't load packages that transitively depend on missing packages.
    • It doesn't let us suggest fixes (updates to go.mod). Everything is just broken.
    • -modfile would let us add the module requirement to a separate go.mod, then suggest a change with the difference. We could type check everything.
  • How should -modfile be passed to go list?
    • golang.org/x/tools/go/packages.Config.BuildFlags.
    • Drivers are expected to know all the flags for go list, including -modfile. Drivers for older versions of the Go command and other build systems should filter this flag out or find something equivalent.
@myitcv

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@eliasnaur

go list is a build command, but feels like a query command.

I can sympathise with that point of view: it's certainly not obvious.

However, in the context of gopls its use of go list is an implementation detail.

This is true but it's a critical implementation detail. Whilst go/packages is the abstraction, go list is the very means by which the concept of modules, packages etc are defined for cmd/go-based build systems.

(That's not to say however that we're bound only by what cmd/go offers today, indeed that's what we're discussing here.)

I would personally have GOFLAGS=-mod=readonly if govim supported it.

I think this is a great suggestion for an option in govim. Indeed I just did a bit of experimentation and it already works if GOFLAGS=-mod=readonly is passed to the environment of gopls by govim: you get no modifications to go.{mod,sum} as a result of using Vim/govim, and changes made via cmd/go are correctly picked up by gopls by virtue of govim simulating file watch changes (whilst we wait for full gopls file watching support). We could add this today with a bit of a hack on the govim side, pending support for a workspace config option in gopls at a later date. I've created govim/govim#555 to track this for govim.

Whenever you consent to the changes. Whether that is a go build, go test on the command line, or clicking "run" or "accept" in a IDE.

I'm not clear how go build or go test is any more "safe" than things happening via gopls/go/packages/go list - they will end up having exactly the same side effects (if we consider them being run at the same point in time) and you're still blind to them unless you check your go.{mod,sum}.

Keep an internal copy of go.* and use -modfile to point to them. Whenever the original go.* files changes, replace the copy.

As you mention above, you'd either look to "accept" the changes by:

  1. explicitly running go build, go test and friends
  2. prompting the user

For option 1, this would mean that there is a possibility the subsequent go build/go test will result in a different changes to the go.{mod,sum} than those made in the -modfile copy. Because none of these commands (with the exception of go get) specifies a version of a dependency. This doesn't seem ideal.

For option 2, presumably you'd prompt for every change that gets made to the -modfile go.mod to "sync" it back to the original if the user accepts? If so, this feels like a very noisy workflow to me, especially when I don't get any such prompts when using go test etc.

Per @jayconrod's request to Rebecca and Ian (#34506 (comment)) and subsequent follow up in #34506 (comment), I think it's worth looking at the scenarios when a go.{mod,sum} change can occur:

  • incomplete go.{mod,sum} - i.e. go mod tidy not run
  • new import added in main module and code action run to run goimports equivalent (this happens on save in govim if so configured)
  • ... others?

And also the use cases that motivated this discussion in the first place:

  • UC1 - people changing Git branches (as I understand it, this generally causes all sorts of problems)
  • UC2 - people just browsing projects, i.e. with no intention of making changes
  • UC3 - people who do not want to have gopls touch go.{mod,sum}
  • ... others?

UC3 is covered above and and in govim/govim#555.

UC1 requires a significantly broader solution to my understanding, because changing files like this "underneath" the editor has all sorts of problems.

UC2 is totally valid to my mind, and building on the suggestion discussed between Jay, Rebecca and Ian, it might make sense (assuming the user is not interested in UC3) to not make any changes to go.{mod,sum} until the first user-initiated change is made to a file in the main module. This however creates a weird UX, because all type check errors that might show would then disappear with the addition, say, of a space to a comment. Alternatively, if it's detected the go.{mod,sum} is not tidy, then the user could be prompted to fix this as the workspace is opened.


Just to note, I'm very much in support of fixing workflow issues like this. My reservations are around a lack of clarity on the UI/UX, use cases that we're looking to fix, whether we will end up actually fixing those use cases, and potential issues with conflicts if we were to use -modfile.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

scenarios when a go.{mod,sum} change can occur

As I mentioned above, this happens with go-fuzz. We inject a dependency and execute a build, which then changes go.mod in a way that the user doesn't care about at all.

@myitcv

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@josharian - sorry, I totally missed that. My comments were more focussed on use cases from a gopls/editor perspective. Although I'll admit I don't really know whether there is overlap or not with go-fuzz in that respect.

@eliasnaur

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

@josharian

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2019

Here's another use case for this flag.

As part of #34867, I want to use a small Go program to do some fairly trivial json parsing. At the moment that I need that Go program to run, the local go.mod file is in a bad state. (I'm running the Go program as part of a script to make the go.mod file functional.) As a result, I can't use go run. If I could do go run -modfile=/sometempfile, then I could temporarily get past the broken go.mod and run the file.

(Yes, there are other ways to accomplish this, like setting GO111MODULE=off, or copying the Go program into a temp dir, manufacturing a trivial go.mod for it, building it, and running it. But this flag also provides a nice, easy simple workaround.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.