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

proposal: cmd/go: deprecate a major module version #40357

Open
peterbourgon opened this issue Jul 22, 2020 · 24 comments
Open

proposal: cmd/go: deprecate a major module version #40357

peterbourgon opened this issue Jul 22, 2020 · 24 comments
Labels
Projects
Milestone

Comments

@peterbourgon
Copy link
Member

@peterbourgon peterbourgon commented Jul 22, 2020

Proposal: A Mechanism For Deprecating Modules

Authors: @peterbourgon, @adg

This proposal is a replacement for part of #38762. The other part is replaced by #40323.

Problem

Currently, there is no standard way for package authors to signal to consumers that a major version of a package is deprecated and no longer supported. Package consumers can easily inadvertently select an unmaintained major version (see #40323) and miss important new features or bug fixes.

Proposal

We propose adding a Deprecated: comment for modules that is recognized by go tooling to flag that a module version as deprecated. This is parallel to the Deprecated: comments that can be used to flag a package or identifier as deprecated.

When selecting a deprecated module version, the go tool will print a note to the user alerting them to the deprecation, and printing the deprecation text provided by the module author. Authors can use this notification to direct package consumers to more recent major versions, or an entirely new module that replaces the deprecated one.

As with package-level deprecation comments, module deprecation comments can span multiple lines.

Example

The author of module github.com/peterbourgon/ff has released a new major version v2 (current version v2.0.1), and wants to deprecate the old major version tree v1 (latest version v1.7.4).

In the v1 branch, they add a module declaration to the go.mod file, above the module declaration:

// Deprecated: The latest supported version is github.com/peterbourgon/ff/v2.
module github.com/peterbourgon/ff

They commit this change and tag it as a new patch release v1.7.5.

There are several ways this deprecation message will or will not be displayed to package consumers.

If a consumer is fetching the module at major version v1 for the first time, the command succeeds but a deprecation warning is printed as part of go get's output:

$ go get gthub.com/peterbourgon/ff
go: deprecated: github.com/peterbourgon/ff v1.7.5 👈
go: deprecated: The latest supported version is github.com/peterbourgon/ff/v2. 👈

If a consumer is currently using the module at version e.g. v1.7.4, no warnings will be printed when that module version is fetched. However, if the consumer tries to upgrade the dependency, they will see the deprecation warning. Notably, the upgrade will still succeed.

$ go get -u github.com/peterbourgon/ff
go: github.com/peterbourgon/ff upgrade => v1.7.5
go: deprecated: github.com/peterbourgon/ff v1.7.5 👈
go: deprecated: The latest supported version is github.com/peterbourgon/ff/v2. 👈

If a consumer is currently using the module at major version 1 (e.g. v1.7.4), and runs a command that lists the latest version available in that major version tree (i.e. v1.7.5), the warning is printed.

$ go list -m -u all
example.com/my-module
github.com/BurntSushi/toml v0.3.1
github.com/mitchellh/go-wordwrap v1.0.0
github.com/peterbourgon/ff v1.7.4 [v1.7.5]
go: deprecated: github.com/peterbourgon/ff v1.7.5 👈
go: deprecated: The latest supported version is github.com/peterbourgon/ff/v2. 👈

Integrations

pkg.go.dev

Module deprecation warnings should be prominently displayed on pkg.go.dev. It is worth exploring the linkification of module paths when displaying module deprecation notices.

Editor integration

If and when editors upgrade dependencies, these deprecation notices should be surfaced to the user as appropriate.

@cespare
Copy link
Contributor

@cespare cespare commented Jul 22, 2020

If both #40323 and this proposal were accepted:

  • Am I correct that you propose alerts under the same set of operations?
  • If a module version were deprecated and there's a newer major module version available, do you propose to print both warnings?
@peterbourgon
Copy link
Member Author

@peterbourgon peterbourgon commented Jul 23, 2020

Am I correct that you propose alerts under the same set of operations?

To the best of my knowledge, yes: when a module consumer first adds a version of a module to their project which meets the respective criteria.

If a module version were deprecated and there's a newer major module version available, do you propose to print both warnings?

Yes.

@bcmills
Copy link
Member

@bcmills bcmills commented Jul 23, 2020

To me, this has the same issue as proposal #40323: a one-time warning when the dependency is added is easy to miss.

Module deprecation seems like a good thing to surface in gorelease, gopls, and pkg.go.dev. go get and go mod tidy seem far too late in the development cycle, and too ephemeral anyway.

CC @jayconrod @matloob

@peterbourgon
Copy link
Member Author

@peterbourgon peterbourgon commented Jul 23, 2020

To me, this has the same issue as proposal #40323: a one-time warning when the dependency is added is easy to miss . . . go get and go mod tidy seem far too late in the development cycle, and too ephemeral anyway.

I don't really understand these points. When I add a new module dependency to my project, and then run whichever go command will cause it to be fetched — either directly in the terminal, or indirectly through my editor integration — I always watch to see the results of the fetch. Don't you? And isn't that moment, when your project first gains knowledge of a new dependency, precisely the moment when you'd want to be alerted to information like this?

Maybe we have different workflows, that's possible. I'd love to understand yours.

@bcmills
Copy link
Member

@bcmills bcmills commented Jul 23, 2020

When I add a new module dependency to my project, and then run whichever go command will cause it to be fetched — either directly in the terminal, or indirectly through my editor integration — I always watch to see the results of the fetch. Don't you?

If I make a change to a file and then run go test on the package containing that file, I generally watch for the results of the test — not for other changes to module versions that happened to precede the actual test run. (To examine the resulting changes to module requirements, I use git diff after the code is actually working.)

@peterbourgon
Copy link
Member Author

@peterbourgon peterbourgon commented Jul 23, 2020

If I make a change to a file

We're not talking about just making a change, we're talking about the first time you add a new dependency to a project, which is much less routine.

I generally watch for the results of the test — not for other changes to module versions that happened to precede the actual test run.

OK, I think it's clear that the changes in this proposal are likely to be missed by your workflow. I'm not convinced that (a) your workflow is typical, or (b) that in any case this should impact the proposal.

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Jul 23, 2020

OK, I think it's clear that the changes in this proposal are likely to be missed by your workflow. I'm not convinced that (a) your workflow is typical, or (b) that in any case this should impact the proposal.

FWIW my workflow is similar. When making changes to a project that might be using new dependencies, the test and/or build output can be verbose and I fear that these warnings would be lost in the noise. The warnings wouldn't be repeated AIUI, so I'm not convinced that this feature is as useful as it could be.

As usual, warnings are often ignored. I think I'd prefer to have a feature that made it an error to add a new direct dependency that's deprecated (either opt-in, or as default behaviour with a way to opt out of it).

@myitcv
Copy link
Member

@myitcv myitcv commented Jul 23, 2020

I always watch to see the results of the fetch. Don't you?

@peterbourgon can you provide more detail on your workflow?

I ask because from my perspective and experience of using gopls, the majority of people will end up adding requirements to their go.mod indirectly via gopls (largely via the goimports-esque workflow). My gut feeling is that the majority of people (based on the assumption that the majority of Go developers today use gopls) will not use go get directly when it comes to adding/upgrading a dependency.

That's not to say we shouldn't support a notification when invoking go get. However I share @bcmills' and @rogpeppe's concerns that this would, in most cases, be a notification that is missed by virtue of it getting buried in other output or because cmd/go isn't the end tool being used.

Module deprecation seems like a good thing to surface in gorelease, gopls, and pkg.go.dev. go get and go mod tidy seem far too late in the development cycle, and too ephemeral anyway.

Agreed. That said, getting UX for gopls "right" so as not to be annoying will be tricky (although we should do something).

cc @stamblerre for gopls

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Jul 23, 2020

I ask because from my perspective and experience of using gopls, the majority of people will end up adding requirements to their go.mod indirectly via gopls (largely via the goimports-esque workflow). My gut feeling is that the majority of people (based on the assumption that the majority of Go developers today use gopls) will not use go get directly when it comes to adding/upgrading a dependency.

I was quite literally just typing this out! 🙂

My workflow when it comes to new libs is to copy out of pkg.go.dev or my browser's URL bar directly into my editor, and then gopls will eventually make a change to my go.mod file by virtue that the tooling manages updates it. Then, I'll go back in the terminal some time later to go mod tidy and clean things up. I'm much less often at the terminal running go get unless I'm there to explicitly upgrade something or am doing something where I need go mod edit or gohack or something.

I'll also bring up that I think some users (at least with gopls in VS Code) may be running tests in the editor, be it by running the handy "run tests with coverage" commands or a code lens to run a specific test, in which case they may never see go test's output unless it fails. (I don't fall into this category, except when looking for coverage or debugging a test; I usually run tests in a separate terminal.)

@peterbourgon
Copy link
Member Author

@peterbourgon peterbourgon commented Jul 23, 2020

@peterbourgon can you provide more detail on your workflow?

I ask because from my perspective and experience of using gopls, the majority of people will end up adding requirements to their go.mod indirectly via gopls (largely via the goimports-esque workflow). My gut feeling is that the majority of people (based on the assumption that the majority of Go developers today use gopls) will not use go get directly when it comes to adding/upgrading a dependency.

Most of the time my workflow is like this, too. I'd say about 30% of the time I manually go get a new module in the terminal. In neither case do I ignore or only partially read the output of the next go tool command which ends up actually performing the version resolution and the fetch.

To the broader point, editor integration is important, and I would indeed hope/expect the warning message to be parsed and surfaced in a more visible way in the editor by gopls. I'm not sure if that's a little warning dialog, like when you need to recompile your tools; or a prominent message postfixed to the output pane; or what, exactly. But it seemed like an over-reach for the proposals to dictate how that should work. Once the message is in the output, tooling can surface it however makes sense.

@peterbourgon
Copy link
Member Author

@peterbourgon peterbourgon commented Jul 23, 2020

To zoom out a bit, I think we'd be happy to amend the proposal(s) to describe additional places or ways to emit the warning. The mechanisms listed were simply the most obvious to us at time.

We agree that editor integration and pkg.go.dev are important. I'm not sure about gorelease — presumably you meant goreleaser — because I want to be notified before I start writing code, not after. But maybe I'm not understanding the idea there.

@myitcv
Copy link
Member

@myitcv myitcv commented Jul 23, 2020

Thanks.

In neither case do I ignore or only partially read the output of the next go tool command which ends up actually performing the version resolution and the fetch.

I suspect you are rather more diligent therefore than most, myself included! If a command succeeds, I don't care much for the output unless that command's principal job is to show me output (e.g. ls). Hence I think @rogpeppe is on point with this comment #40357 (comment), specifically the suggestion in the second paragraph.

If gopls performs the addition for you (e.g. accepting a suggestion to add a requirement to go.mod) where do you review the output?

Once the message is in the output, tooling can surface it however makes sense.

I would be surprised if a gopls implementation involved parsing the output of a cmd/go command (not least because another cmd/go invocation, not triggered by gopls, might be the one that sees the "edge" and hence the user would miss this in their editor). Rather, as @bcmills alluded to in a reply above, the current set of warnings to show to the user would be determined from the current state of go.mod. The tricky part I referred to above is how to track when a user does not action such a notification and instead dismisses it with "I don't care".

I'm not sure about gorelease

I suspect @bcmills did indeed mean gorelease. i.e. before releasing a new version of a module you would have a final warning that your module (indirectly) depends on a deprecated module. So I think that reference was to a dependency author's workflow.

@peterbourgon
Copy link
Member Author

@peterbourgon peterbourgon commented Jul 23, 2020

I suspect @bcmills did indeed mean gorelease.

Interesting, I've never heard of this tool before. Seems like a kind of release linter? If so I agree that it makes sense to surface there.

The current set of warnings to show to the user would be determined from the current state of go.mod . . .

I didn't get that from @bcmills comments, but with the point clarified, it seems reasonable to me, too. My intuition is that a dialog generated when the deprecated module is first added to the file would be best, if gopls can edge-trigger on that condition. And/or squiggly-underlining the deprecated modules in the require block of the go.mod at a warning (yellow) level?

@Merovius
Copy link

@Merovius Merovius commented Jul 23, 2020

Just a thought: How about adding a // deprecated comment to go.mod, analogous to // indirect? Orthogonal to the question of if and when to output a warning. go.mod is where it would pop up in reviews and where I even occasionally check if the set of dependencies looks reasonable to me.

(In the question of "when to show a warning", I kinda agree with everyone here, I feel that when initially adding it would be the semantically correct point in time, but I also would almost certainly miss it if that was the only place. So I don't really have an opinion, but as long as the warning is exposed to me in go.mod I wouldn't care either way)

@myitcv
Copy link
Member

@myitcv myitcv commented Jul 23, 2020

Interesting, I've never heard of this tool before

@jayconrod announced it some time ago. We've discussed it a number of times on the golang-tools calls in various contexts.

quiggly-underlining the deprecated modules in the require block of the go.mod at a warning (yellow) level?

This is exactly what I was referring to. The challenge comes in storing the state of a dismissed warning. Should that also be dismissed for other authors of the package (c.f. @Merovius's comment) or is that something more personal?

I appreciate this is detail that one might consider beyond the scope of the proposal as it stands, but to my mind in this instance we need to work backwards from the UI/UX of various tools where this detail is surfaced and then derive the scheme of changes to go.mod or wherever that most make sense.

@peterbourgon
Copy link
Member Author

@peterbourgon peterbourgon commented Jul 23, 2020

The challenge comes in storing the state of a dismissed warning. Should that also be dismissed for other authors of the package (c.f. @Merovius's comment) or is that something more personal?

In my mind, the dialog on first add is naturally dismiss-able, but the squiggly should be perpetual.

Just a thought: How about adding a // deprecated comment to go.mod?

Interesting. Where would that logic live? I guess in the go tool?

@Merovius
Copy link

@Merovius Merovius commented Jul 23, 2020

Interesting. Where would that logic live? I guess in the go tool?

I'd assume so - my understanding is, that the best way to programmatically edit go.mod is to call into the go tool. I'm not quite sure which go tool invocation would add it, probably go mod tidy at least and go get likely when adding/updating a module line. Personally, I'd be fine with basically any subset that's deemed reasonable.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jul 23, 2020

It seems like deprecation notices should be shown in the same situations as retraction notices #24031, namely with go list -m -u and go get on the relevant module. It seems like the difference is that this proposal suggests showing a deprecation notice only once, the first time the go.mod containing the deprecation notice is downloaded into the cache, while retraction notices would be shown every time.

I'd advocate for showing both notices in the same situations: when the user checks for upgrades, and when the user performs an upgrade, regardless of what's in the cache.

(gorelease, pkg.go.dev, and gopls should all show these notices as well, but I think the UX for that can be worked out later.)

@rsc rsc changed the title Proposal: A Mechanism For Deprecating Modules proposal: cmd/go: deprecate a major module version Jul 23, 2020
@peterbourgon
Copy link
Member Author

@peterbourgon peterbourgon commented Jul 24, 2020

@Merovius

I'm not quite sure which go tool invocation would add it, probably go mod tidy at least and go get likely when adding/updating a module line. Personally, I'd be fine with basically any subset that's deemed reasonable.

Perhaps also plain go {build, install} if you manually add the requirement, and that's the first go tool invocation which causes the dependency to be fetched?

@Merovius
Copy link

@Merovius Merovius commented Jul 24, 2020

@peterbourgon Sure. The only reason I was conservative in my suggestion was that I know it is controversial that go build may hit the network or edit go.mod. I don't know of a reason not to have the deprecation notice in there, so as far as I'm concerned, any command that edits go.mod and has the info should probably add it, if it isn't there :)

@rsc rsc added this to Incoming in Proposals Aug 4, 2020
@adg
Copy link
Contributor

@adg adg commented Aug 4, 2020

Thanks everyone for your feedback so far. It has definitely helped me to better understand the various ways in which people manage their module dependencies.

@rogpeppe wrote:

As usual, warnings are often ignored. I think I'd prefer to have a feature that made it an error to add a new direct dependency that's deprecated (either opt-in, or as default behaviour with a way to opt out of it).

It is possible to make this an error, if the package maintainer wants it to be. They'd do this by breaking the build of (or perhaps retracting?) the package at the version that is deprecated. I think it's desirable that we leave this judgment—of whether it's worth breaking new users—to the upstream package maintainer.

Note that this proposal doesn't preclude gopls from doing something useful here. This proposal is about adding a deprecation mechanism for modules that gopls may (and likely will) do something useful with. If we assume that we do want module deprecations, then IMO the first step of incorporating this new feature is to put it in the go tool, and only then broaden the scope to gopls and other tools once we have confidently taken that first step.

@Merovius wrote:

Just a thought: How about adding a // deprecated comment to go.mod, analogous to // indirect? Orthogonal to the question of if and when to output a warning. go.mod is where it would pop up in reviews and where I even occasionally check if the set of dependencies looks reasonable to me.

I quite like this idea. In my workflow I always scrutinize changes to go.mod when I commit new changes to a repository. I hope others are the same. If I saw a // deprecated after the module requirement, it would certainly give me cause to investigate further.

In that case, perhaps there needs to be a mechanism to look up a deprecation message for a given module. Right now it's not straightforward to examine a go.mod for an arbitrary Go module version, at least not with the go tool alone.

@jayconrod wrote:

It seems like deprecation notices should be shown in the same situations as retraction notices #24031
[...]
I'd advocate for showing both notices in the same situations: when the user checks for upgrades, and when the user performs an upgrade, regardless of what's in the cache.

This seems reasonable to me, and from an implementation POV may be the most elegant approach (check for both retractions and deprecations at the same time). If we mostly agree both that 1) retractions should be surfaced at all and 2) that deprecations are a feature we want to provide, then we should all be on board with surfacing them to the user in the same way.

@rsc rsc moved this from Incoming to Active in Proposals Aug 12, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Aug 12, 2020

@bcmills @jayconrod @matloob, what are your opinions about whether we should move forward with this? I can't quite tell from the discussion above.

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 12, 2020

I'm in favor of the // Deprecated: comment convention.

I'm also in favor of @Merovius's suggestion to have the go command surface deprecated dependencies by adding // deprecated comments to the require lines of the consumer's go.mod file (such as during go mod tidy or go get -u).

I'm ambivalent on printing warnings to the console. I certainly don't think that should be the only means of surfacing deprecation, but it seems unobjectionable if accompanied by something more durable (such as the aforementioned consumer-side // deprecated comments).

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 14, 2020

I think we should move forward with this.

The // Deprecated: comment convention seems like the most reasonable way to express this. To be clear, this would deprecate an entire module, not individual versions of that module (I mean this in the sense that major versions are technically different modules).

The comment would be extracted from the @latest version of the module. So the deprecation could be changed or removed by releasing a new version.

go list -m -u and go get will surface the warning, together with retraction warnings.

Commands that update go.mod will add // deprecated comments to requirements in go.mod if they are aware of deprecations. But note that if #40728 is implemented, then most commands will not update go.mod by default, and even if they did, most commands won't go out to resolve the @latest version. That includes go mod tidy unless new dependencies are added. In practice, go get would be the only command that would do this for existing dependencies.

I'm not entirely in favor of the // deprecated comments by the way, but I don't feel strongly. They seem like an annotation for the dependency, not the module: the edge in the graph rather than the vertex. That said, they are highly visible without being in the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.