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

Jump to bottom

x/net/webdav: replace "golang.org/x/net/context" with "context" #21364

Closed
dmitshur opened this issue Aug 9, 2017 · 5 comments
Closed

x/net/webdav: replace "golang.org/x/net/context" with "context" #21364

dmitshur opened this issue Aug 9, 2017 · 5 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Aug 9, 2017

This is a long term tracking issue for the task of replacing "golang.org/x/net/context" import with "context". Doing so is desirable because otherwise, modern Go packages end up having no choice but to import both of those 2 packages, which is suboptimal (but currently a lesser evil than the alternatives).

This is somewhat similar to #21358, but a complication is that this changes the definition of the webdav.FileSystem interface. That interface is quite popular (as far as I know) and has many implementations outside.

The conversation about this was started in the CL that added context.Context as first parameter to all webdav.FileSystem methods:

https://go-review.googlesource.com/c/32421#message-528154c59254446fadf063500da3911231523950

The latest comment was from @nigeltao saying that:

Having 1.7 (and 1.8) importing both context packages is a little annoying, but I think that it's the lesser of two evils, compared to duplicating a lot of code.

In Go 1.9, we might get type aliases back, and it will be even less annoying. If so, longer term, when we don't have to support anything prior to 1.9, I think we will be able to just replace all of the "golang.org/x/net/context"s with "context"s in this package, without breaking any third party code that uses "golang.org/x/net/context" with webdav.

An update on that. Go 1.9 final is close to release. Type aliases have been added indeed, and the implementation of golang.org/x/net/context package in Go 1.9+ is just:

// +build go1.9

package context

import "context" // standard library's context, as of Go 1.7

// A Context carries a deadline, a cancelation signal, and other values across
// API boundaries.
//
// Context's methods may be called by multiple goroutines simultaneously.
type Context = context.Context

// A CancelFunc tells an operation to abandon its work.
// A CancelFunc does not wait for the work to stop.
// After the first call, subsequent calls to a CancelFunc do nothing.
type CancelFunc = context.CancelFunc

(Source.)

So, if my understanding is correct, we are now waiting on "when we don't have to support anything prior to 1.9", which is probably going to be around Go 1.11 or later becoming stable. Is my understanding right @nigeltao?
@gopherbot gopherbot added this to the Unreleased milestone Aug 9, 2017
@nigeltao
Copy link
Contributor

nigeltao commented Sep 1, 2017

We won't drop Go 1.8 support until at least after Go 1.11 is released, but I don't think we need to wait until Go 1.11 to do something now.

For example, we could follow the "< 1.9" vs ">= 1.9" split in the golang.org/x/image/draw package, based on build tags:

$ cd golang.org/x/image/draw
$ grep build go1_*
go1_8.go:// +build !go1.9,!go1.8.typealias
go1_9.go:// +build go1.9 go1.8.typealias

A year or two from now, we can delete go1_8.go and rename or merge go1_9.go, both in x/image/draw and in x/net/webdav.

@dmitshur
Copy link
Contributor Author

dmitshur commented Sep 1, 2017 •
edited

I suggested using build tags on November 2016:

With build tags, you can use "x/net/context" with Go 1.6, but "context" Go 1.7. It would require duplicating a lot of code across the Go versions (file.go, prop.go, etc.). Do you think that it's not worth doing that?

You responded at the time with:

Having 1.7 (and 1.8) importing both context packages is a little annoying, but I think that it's the lesser of two evils, compared to duplicating a lot of code.

You're suggesting build tags as a viable solution now. Is it because type aliases enable us to do something we couldn't do previously? (I'm just trying to understand what factors have changed since then.)

If using build tags is an acceptable solution, I'm happy to work on a CL to resolve this issue (unless someone else wants to take it).

@nigeltao
Copy link
Contributor

nigeltao commented Sep 1, 2017

Ah, I forgot that the type in question (context.Context) is defined in a different package, so it's unlike the x/image/draw package where is Op type is defined in the same package can be easily defined/aliased before/after Go 1.9.

Yeah, build tags don't seem as viable for x/net/webdav, even with type aliases, due to all the code duplication it would require.

I agree that we're not going to do anything for now, and are waiting on "when we don't have to support anything prior to 1.9".

@gopherbot
Copy link

Change https://golang.org/cl/148438 mentions this issue: webdav: remove Go 1.6 support, use std context

@bradfitz
Copy link
Contributor

bradfitz commented Nov 8, 2018

I agree that we're not going to do anything for now, and are waiting on "when we don't have to support anything prior to 1.9".

We are officially in that state as of Nov 1st. I'm actively deleting any Go 1.8 and earlier code.

dmitshur added a commit to shurcooL/webdavfs that referenced this issue Nov 14, 2018
@dmitshur
webdavfs: Replace "golang.org/x/net/context" with "context".
44bbca9 
The issue golang/go#21364 has been resolved upstream,
so we can also switch to importing package context here.

Follows golang/net@04ba8c8.
@golang golang locked and limited conversation to collaborators Nov 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@bradfitz @dmitshur @ianlancetaylor @nigeltao @gopherbot