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

net: remove dependency on fmt, unicode via golang.org/x/net/dns/dnsmessage #40070

Closed
rsc opened this issue Jul 6, 2020 · 4 comments
Closed

net: remove dependency on fmt, unicode via golang.org/x/net/dns/dnsmessage #40070

rsc opened this issue Jul 6, 2020 · 4 comments
Assignees
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Jul 6, 2020

net should not depend on fmt and unicode.

golang.org/x/net/dns/dnsmessage contains one line using fmt:

return nil, off, fmt.Errorf("invalid resource type: %d", hdr.Type)

But dnsmessage is vendored into the standard library and imported by net,
and this use is making net depend on fmt, and in turn on unicode.

This one line should be changed, to remove the use of fmt.

go/build.TestDependencies claims to check that net does not
depend on unicode, which should have detected this, but that
test does not look inside vendored packages.

I have a fix that I will send out for that problem as well.

@rsc rsc added this to the Go1.15 milestone Jul 6, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 6, 2020

Change https://golang.org/cl/241078 mentions this issue: go/build: rewrite TestDependencies to be cleaner, more correct

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 6, 2020

Change https://golang.org/cl/241085 mentions this issue: dns/dnsmessage: remove use of fmt that crept in

gopherbot pushed a commit to golang/net that referenced this issue Jul 7, 2020
This package cannot use fmt, because standard package net imports it.
(Most of the package is already clean, including Type.String.)

For golang/go#40070.

Change-Id: I9be92e98d9f5dcfb26852d38004e05f07df5e17a
Reviewed-on: https://go-review.googlesource.com/c/net/+/241085
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Jul 7, 2020
TestDependencies defines the dependency policy
(what can depend on what) for the standard library.

The standard library has outgrown the idea of writing
the policy as a plain map literal. Also, the checker was
ignoring vendored packages, which makes it miss real
problems.

This commit adds a little language for describing
partial orders and rewrites the policy in that language.

It also changes the checker to look inside vendored
packages and adds those to the policy as well.

This turned up one important problem: net is depending
on fmt, unicode via golang.org/x/net/dns/dnsmessage,
filed as #40070.

This is a test-only change, so it should be appropriate
even for the release freeze, especially since it identified
a real bug.

Change-Id: I9b79f30761f167b8587204c959baa973583e39f2
Reviewed-on: https://go-review.googlesource.com/c/go/+/241078
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 8, 2020

Is there anything else to do here? As far as I can tell this issue can be closed.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jul 14, 2020

I've confirmed the package net does not import fmt nor unicode, not even indirectly. It was resolved via CL 241257, and CL 241078 added a regression test (see this line).

Closing as this issue is fixed. Please comment if I missed something.

@dmitshur dmitshur closed this Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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