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.

Sign up for GitHub

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/lif, x/net/route: as long as std imports these packages, they can't import golang.org/x/sys/unix because of standard library policy #54259

Closed
dmitshur opened this issue Aug 4, 2022 · 8 comments
Closed

x/net/lif, x/net/route: as long as std imports these packages, they can't import golang.org/x/sys/unix because of standard library policy #54259

dmitshur opened this issue Aug 4, 2022 · 8 comments
Assignees
@ianlancetaylor
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Release Team
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Aug 4, 2022 •
edited

Updating to latest golang.org/x dependencies causes golang.org/x/sys/unix to be vendored into std, which goes against the policy set in #32102:

--- FAIL: TestScript (0.02s)
    --- FAIL: TestScript/prevent_sys_unix_import (1.68s)
        script_test.go:270: 
            # (2022-08-04T14:23:36Z)
            # Policy decision: we shouldn't vendor golang.org/x/sys/unix in std
            # See https://golang.org/issue/32102 (1.671s)

It's happening because the golang.org/x/net/lif package (vendored into std) has been updated to import and depend on golang.org/x/sys/unix in CL 413275 and CL 414994:

src $ go mod why 'golang.org/x/sys/unix'
# golang.org/x/sys/unix
net
golang.org/x/net/lif
golang.org/x/sys/unix

To make progress on #36905, I will likely need to send CLs to revert those golang.org/x/net/lif simplification CLs.

Package net seems to only use golang.org/x/net/lif in one function call interfaceAddrTable, so a path that can be investigated in the future is whether removing that dependency can be simpler overall.

CC @tklauser, @ianlancetaylor, @bcmills.
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 4, 2022
@dmitshur dmitshur added this to the Unreleased milestone Aug 4, 2022
@dmitshur dmitshur changed the title x/sys/lif: as long as net imports this package, it can't import golang.org/x/sys/unix because of standard library policy x/net/lif: as long as net imports this package, it can't import golang.org/x/sys/unix because of standard library policy Aug 4, 2022
@dmitshur dmitshur mentioned this issue Aug 4, 2022
Open
@dmitshur
Copy link
Contributor Author

dmitshur commented Aug 4, 2022 •
edited

Next up:

$ go mod why 'golang.org/x/sys/unix'
# golang.org/x/sys/unix
net
golang.org/x/net/route
golang.org/x/sys/unix

Reverting CL 419180 is enough to resolve this and avoid x/sys/unix getting vendored into std.

@dmitshur dmitshur changed the title x/net/lif: as long as net imports this package, it can't import golang.org/x/sys/unix because of standard library policy x/net/lif, x/net/route: as long as net imports this package, it can't import golang.org/x/sys/unix because of standard library policy Aug 4, 2022
@dmitshur dmitshur changed the title x/net/lif, x/net/route: as long as net imports this package, it can't import golang.org/x/sys/unix because of standard library policy x/net/lif, x/net/route: as long as std imports these packages, they can't import golang.org/x/sys/unix because of standard library policy Aug 4, 2022
@gopherbot
Copy link

Change https://go.dev/cl/421334 mentions this issue: all: update vendored golang.org/x dependencies for Go 1.20 development

gopherbot pushed a commit that referenced this issue Aug 4, 2022
@dmitshur @gopherbot
all: update vendored golang.org/x dependencies for Go 1.20 development
85d87b9 
Go 1.20 development is just beginning. This is a time to update all
golang.org/x/... module versions that contribute packages to the std
and cmd modules in the standard library to latest master versions.

This CL holds back some of the available updates to the x/net module
due to go.dev/issue/54259. It'll be updated in a later separate pass.
x/tools is also held back a bit to avoid pulling in too new of x/net.

For #36905.
For #53812.
Updates #54259.

Change-Id: Iaefe6a343a02cc5ceb85c15125882d64dd372627
Reviewed-on: https://go-review.googlesource.com/c/go/+/421334
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur dmitshur added this to In Progress in Go Release Team Aug 4, 2022
@ianlancetaylor
Copy link
Contributor

It may be possible to use syscall rather than x/sys/unix for these constants.

@gopherbot
Copy link

Change https://go.dev/cl/421419 mentions this issue: lif: import syscall rather than golang.org/x/sys/unix

gopherbot pushed a commit to golang/net that referenced this issue Aug 5, 2022
@ianlancetaylor @gopherbot
lif: import syscall rather than golang.org/x/sys/unix
7431dee 
It happens that everything we need is already defined in syscall.
This avoids problems when this package is vendored into the
standard library.

For golang/go#54259

Change-Id: I999eba5d089a1dfb341e27ebf3651ace0de26947
Reviewed-on: https://go-review.googlesource.com/c/net/+/421419
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/421425 mentions this issue: route: import syscall rather than golang.org/x/sys/unix

gopherbot pushed a commit to golang/net that referenced this issue Aug 5, 2022
@ianlancetaylor @gopherbot
route: import syscall rather than golang.org/x/sys/unix
a33c5aa 
It happens that everything we need is already defined in syscall.
This avoids problems when this package is vendored into the
standard library.

For golang/go#54259

Change-Id: I86bfe44f20c9db2ecfdb8dbb2ef798391da2bfa6
Reviewed-on: https://go-review.googlesource.com/c/net/+/421425
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@dmitshur
Copy link
Contributor Author

dmitshur commented Aug 5, 2022

Thanks Ian for finding a resolution to this issue that's better than reverting those earlier x/net CLs. I'm glad I left x/net out of CL 421334 to create an opportunity to take time to think about this (without the pressure of #53812).

I've sent CL 421461 to vendor the latest x/net in the main repo, and will close this now because I don't think there's anything left to do.

I've considered adding a test to x/net (also suggested by @prattmic), but such a test would either need to hard-code which x/net packages are vendored in std, or extract that dynamically somehow which is probably better suited for #48523. If this happens more often we can revisit it. (If someone has an idea for a better test, please feel free to file an issue.)

@dmitshur dmitshur closed this as completed Aug 5, 2022
Go Release Team automation moved this from In Progress to Done Aug 5, 2022
@gopherbot
Copy link

Change https://go.dev/cl/421461 mentions this issue: all: update vendored golang.org/x/{net,tools} for Go 1.20 development

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 5, 2022
@tklauser
Copy link
Member

tklauser commented Aug 5, 2022

Sorry, I wasn't aware of the policy #32102 when I sent these CLs. Thanks Ian for fixing these!

gopherbot pushed a commit that referenced this issue Aug 5, 2022
@dmitshur @gopherbot
all: update vendored golang.org/x/{net,tools} for Go 1.20 development
27f1246 
CL 421334 updated most of golang.org/x dependencies at the start of
the Go 1.20 development cycle. This CL updates x/net and x/tools as
well, now that go.dev/issue/54259 is resolved.

For #36905.
Updates #54259.

Change-Id: Ie422b71cba060a4774076eebf3b499cda1150367
Reviewed-on: https://go-review.googlesource.com/c/go/+/421461
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@dmitshur @jproberts
all: update vendored golang.org/x dependencies for Go 1.20 development
87791b1 
Go 1.20 development is just beginning. This is a time to update all
golang.org/x/... module versions that contribute packages to the std
and cmd modules in the standard library to latest master versions.

This CL holds back some of the available updates to the x/net module
due to go.dev/issue/54259. It'll be updated in a later separate pass.
x/tools is also held back a bit to avoid pulling in too new of x/net.

For golang#36905.
For golang#53812.
Updates golang#54259.

Change-Id: Iaefe6a343a02cc5ceb85c15125882d64dd372627
Reviewed-on: https://go-review.googlesource.com/c/go/+/421334
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@dmitshur @jproberts
all: update vendored golang.org/x/{net,tools} for Go 1.20 development
54c3929 
CL 421334 updated most of golang.org/x dependencies at the start of
the Go 1.20 development cycle. This CL updates x/net and x/tools as
well, now that go.dev/issue/54259 is resolved.

For golang#36905.
Updates golang#54259.

Change-Id: Ie422b71cba060a4774076eebf3b499cda1150367
Reviewed-on: https://go-review.googlesource.com/c/go/+/421461
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @gopherbot
lif: import syscall rather than golang.org/x/sys/unix
8d3b1af 
It happens that everything we need is already defined in syscall.
This avoids problems when this package is vendored into the
standard library.

For golang/go#54259

Change-Id: I999eba5d089a1dfb341e27ebf3651ace0de26947
Reviewed-on: https://go-review.googlesource.com/c/net/+/421419
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @gopherbot
route: import syscall rather than golang.org/x/sys/unix
0d1c92d 
It happens that everything we need is already defined in syscall.
This avoids problems when this package is vendored into the
standard library.

For golang/go#54259

Change-Id: I86bfe44f20c9db2ecfdb8dbb2ef798391da2bfa6
Reviewed-on: https://go-review.googlesource.com/c/net/+/421425
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@ianlancetaylor ianlancetaylor

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Release
Archived in project
Go Release Team
Done
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@tklauser @dmitshur @ianlancetaylor @gopherbot