Switch from x/net/context -> context by kolyshkin · Pull Request #36904 · moby/moby

kolyshkin · 2018-04-19T22:17:25Z

Since Go 1.7, context is a standard package. Since Go 1.9, all x/net/context is a few aliases to types in context.

Many vendored packages still use x/net/context, so vendor entry remains for now.

The commit is generated by this ugly script (I'm bad at sed so had to use awk in between):

#!/bin/bash

for f in $(git ls-files \*.go | grep -v ^vendor/); do
	printf .
	sed -i 's|"golang.org/x/net/context"|"context"|' $f
# fix the position of "context" line
	goimports -w $f
# remove the empty line before "context"
	awk '/^$/ {e=1; next;}
		/^\t"context"$/ {e=0;}
		{if (e) {print ""; e=0}; print;}' < $f > $f.new && mv $f.new $f
# fix the position of "context" line again
	goimports -w $f
done
echo

Note that there is a "standard" way of doing this change -- by running the go tool fix -r context, what it does is the same as the sed above. In addition, I had to fix the "context" line placement which was tricky in case there are empty lines in the imports list (thus the two goimports with an awk in between.

thaJeztah · 2018-04-19T22:22:48Z

We'll probably need something similar for swarmkit, containerd, libnetwork as well (assuming it works)

thaJeztah · 2018-04-19T22:23:50Z

Looks like you need to (re-)vendor something

22:19:13 ---> Making bundle: test-integration (in bundles/test-integration)
22:19:13 Building test suite binary ./integration-cli/test.main
22:19:13 # github.com/docker/docker/integration-cli
22:19:13 package github.com/docker/docker/integration-cli (test)
22:19:13 	imports github.com/docker/docker/client
22:19:13 	imports context/ctxhttp: cannot find package "context/ctxhttp" in any of:
22:19:13 	/go/src/github.com/docker/docker/vendor/context/ctxhttp (vendor tree)
22:19:13 	/usr/local/go/src/context/ctxhttp (from $GOROOT)
22:19:13 	/go/src/context/ctxhttp (from $GOPATH)

kolyshkin · 2018-04-19T22:31:53Z

We'll probably need something similar for swarmkit, containerd, libnetwork as well (assuming it works)

Looking into it...

Looks like you need to (re-)vendor something

Oops, the sed regex was too greedy. Fixed.

kolyshkin · 2018-04-19T22:46:32Z

It looks like as long as Go >= 1.9 is used, we can mix x/net/context and context freely.

kolyshkin · 2018-04-19T22:46:56Z

Nice, it looks like the switch revealed a couple of bugs in the code:

15:44:42 pkg/ioutils/readers_test.go:83::error: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak (vet)
15:44:42 daemon/cluster/swarm.go:507::error: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak (vet)

dnephin · 2018-04-19T23:23:46Z

The commit is generated by this ugly script

FYI, you can do this migration with go tool fix -r context <paths> (using go1.10)

kolyshkin · 2018-04-20T00:27:55Z

FYI, you can do this migration with go tool fix -r context (using go1.10)

Just found that out myself. The problems with the above tool are:

It fixes stuff in vendor/ too.
It doesn't fix the placement of the "context" line in imports list in case there are empty lines in it.

kolyshkin · 2018-04-20T00:30:58Z

CI failure on experimental:

16:12:47 --- FAIL: TestCreateServiceConfigFileMode (12.36s)
16:12:47 	daemon.go:269: [d7e8cb005d1a9] waiting for daemon to start
16:12:47 	daemon.go:301: [d7e8cb005d1a9] daemon started
16:12:47 	create_test.go:295: timeout hit after 10s: task count at 1 waiting for 0
16:12:47 	daemon.go:259: [d7e8cb005d1a9] exiting daemon

Hmm, it does not look related but does not look like a flaky test either

thaJeztah · 2018-04-20T00:36:40Z

I think I just saw that one fail on another PR as well

dnephin · 2018-04-20T15:41:02Z

The problems with the above tool are:

I guess it would need a git reset HEAD vendor && goimports -w ./...

thaJeztah · 2018-04-20T16:17:32Z

@kolyshkin needs a rebase 😅, also perhaps it's good to keep the fixes in a separate PR

kolyshkin · 2018-04-20T19:16:08Z

I guess it would need a git reset HEAD vendor && goimports -w ./...

Unfortunately not -- in case there is an empty line between "standard" and "prefixed" packages, it still won't have "context" in the line it should be, this is why I have to do goimports -w twice in my list, with an empty line removal in between.

codecov · 2018-04-20T19:25:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5c233cf). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36904   +/-   ##
=========================================
  Coverage          ?   35.06%           
=========================================
  Files             ?      614           
  Lines             ?    45694           
  Branches          ?        0           
=========================================
  Hits              ?    16024           
  Misses            ?    27565           
  Partials          ?     2105

kolyshkin · 2018-04-20T19:25:56Z

@kolyshkin needs a rebase

done

also perhaps it's good to keep the fixes in a separate PR

will do

kolyshkin · 2018-04-20T19:30:03Z

also perhaps it's good to keep the fixes in a separate PR

done (#36920), but this way CI here will fail as in #36904 (comment)

kolyshkin · 2018-04-20T20:10:59Z

rebased again

cpuguy83

If CI is happy I'm happy. LGTM!

cpuguy83 · 2018-04-23T16:52:32Z

06:28:53 daemon/cluster/swarm.go:507::error: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak (vet)
06:28:53 pkg/ioutils/readers_test.go:83::error: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak (vet)

kolyshkin · 2018-04-23T17:00:12Z

@cpuguy83 the fix for this is separated to #36920 as per request in #36904 (comment)

Since Go 1.7, context is a standard package. Since Go 1.9, everything that is provided by "x/net/context" is a couple of type aliases to types in "context". Many vendored packages still use x/net/context, so vendor entry remains for now. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2018-04-23T20:53:08Z

Rebased to master (with #36920 merged)

thaJeztah

LGTM!

kolyshkin requested review from cpuguy83, dnephin, tonistiigi and vdemeester as code owners April 19, 2018 22:17

GordonTheTurtle added the status/0-triage label Apr 19, 2018

kolyshkin mentioned this pull request Apr 19, 2018

Bump golang.org/x/net to go1.10 release commit #36894

Merged

kolyshkin force-pushed the context branch from 69cf031 to f689fbc Compare April 19, 2018 22:31

kolyshkin mentioned this pull request Apr 20, 2018

Flaky test: TestCreateServiceConfigFileMode #36907

Open

kolyshkin force-pushed the context branch from 8273b6e to 17edc79 Compare April 20, 2018 19:25

kolyshkin mentioned this pull request Apr 20, 2018

context.WithTimeout: do call the cancel func #36920

Merged

kolyshkin force-pushed the context branch from 17edc79 to 7ecd425 Compare April 20, 2018 19:29

kolyshkin force-pushed the context branch from 7ecd425 to f6323f5 Compare April 20, 2018 20:11

cpuguy83 approved these changes Apr 20, 2018

View reviewed changes

AkihiroSuda added the rebuild/janky label Apr 23, 2018

AkihiroSuda added rebuild/experimental labels Apr 23, 2018

AkihiroSuda approved these changes Apr 23, 2018

View reviewed changes

GordonTheTurtle removed rebuild/experimental labels Apr 23, 2018

thaJeztah added status/2-code-review and removed status/0-triage labels Apr 23, 2018

kolyshkin changed the title ~~[WIP] Switch from x/net/context -> context~~ Switch from x/net/context -> context Apr 23, 2018

kolyshkin force-pushed the context branch from f6323f5 to 7d62e40 Compare April 23, 2018 20:53

kolyshkin mentioned this pull request Apr 24, 2018

Switch from x/net/context -> context containerd/containerd#2305

Merged

thaJeztah approved these changes Apr 24, 2018

View reviewed changes

thaJeztah merged commit 53683bd into moby:master Apr 24, 2018

vdemeester mentioned this pull request Apr 24, 2018

client.Client package should provide testable/mockable version for unit (not e2e) testing #30454

Closed

kolyshkin mentioned this pull request Apr 24, 2018

Switch from x/net/context to context moby/libnetwork#2140

Merged

kolyshkin mentioned this pull request May 4, 2018

Switch from x/net/context to context docker/cli#1038

Merged

kolyshkin mentioned this pull request Sep 18, 2018

Use gometalinter; switch from x/net/context -> context moby/swarmkit#2750

Merged

retpolanne mentioned this pull request Dec 3, 2018

Switch from x/net/context -> context docker-archive/classicswarm#2913

Closed

Conversation

kolyshkin commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Apr 19, 2018

Uh oh!

thaJeztah commented Apr 19, 2018

Uh oh!

kolyshkin commented Apr 19, 2018

Uh oh!

kolyshkin commented Apr 19, 2018

Uh oh!

kolyshkin commented Apr 19, 2018

Uh oh!

dnephin commented Apr 19, 2018

Uh oh!

kolyshkin commented Apr 20, 2018

Uh oh!

kolyshkin commented Apr 20, 2018

Uh oh!

thaJeztah commented Apr 20, 2018

Uh oh!

dnephin commented Apr 20, 2018

Uh oh!

thaJeztah commented Apr 20, 2018

Uh oh!

kolyshkin commented Apr 20, 2018

Uh oh!

codecov bot commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kolyshkin commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Apr 20, 2018

Uh oh!

kolyshkin commented Apr 20, 2018

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 23, 2018

Uh oh!

kolyshkin commented Apr 23, 2018

Uh oh!

kolyshkin commented Apr 23, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kolyshkin commented Apr 19, 2018 •

edited

Loading

codecov bot commented Apr 20, 2018 •

edited

Loading

kolyshkin commented Apr 20, 2018 •

edited

Loading