Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upx/tools: clean up dependencies #29981
Comments
matloob
self-assigned this
Jan 29, 2019
gopherbot
added this to the Unreleased milestone
Jan 29, 2019
matloob
assigned
dmitshur
and unassigned
matloob
Jan 29, 2019
This comment has been minimized.
This comment has been minimized.
I will investigate this and update the original issue with a plan for how this can be resolved. Edit: The following is the latest version of my investigation. golang.org website group:
cmd/present (used for serving talks.golang.org) group:
Others:
godoc/redirect diffdiff --git a/godoc/redirect/redirect.go b/godoc/redirect/redirect.go
index b4599f6a..cf4972f7 100644
--- a/godoc/redirect/redirect.go
+++ b/godoc/redirect/redirect.go
@@ -18,8 +18,6 @@ import (
"strings"
"sync"
"time"
-
- "golang.org/x/net/context/ctxhttp"
)
// Register registers HTTP handlers that redirect old godoc paths to their new
@@ -250,7 +248,11 @@ func isGerritCL(ctx context.Context, id int) (bool, error) {
// https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-change.
ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()
- resp, err := ctxhttp.Get(ctx, nil, fmt.Sprintf("https://go-review.googlesource.com/changes/%d", id))
+ req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("https://go-review.googlesource.com/changes/%d", id), nil)
+ if err != nil {
+ return false, err
+ }
+ resp, err := http.DefaultClient.Do(req.WithContext(ctx))
if err != nil {
return false, err
} |
dmitshur
added
the
NeedsInvestigation
label
Jan 29, 2019
This comment has been minimized.
This comment has been minimized.
This bug is making the assumption that repo-level dependencies are a problem. Implicit in this bug report is that you assume that repo == module, and that having multiple modules in a repo is bad / not an option. Before we clean things up, can we at least agree on the problem? |
This comment has been minimized.
This comment has been minimized.
The following is my understanding of the situation. I understand that repositories that contain more than one module (multi-module repositories) incur a certain complexity cost. Repositories containing a single module are a simpler solution, one that is preferable to a more complex one when both options are viable. There are some situations where it is necessary and justifiable to use multi-module repositories. Whether to make a repository multi-module or not is a decision that needs to be carefully thought through before committing to it, especially if picking the more complex solution. The importance of making an appropriate decision is amplified for golang.org/x subrepos, since many people look at the subrepos for inspiration and best practices to follow. Historically, x/tools had been used as a default repository for placing many tools related to the Go project, and ended up housing packages roughly fitting into the following 3 distinct categories:
By now, many of the production use cases have been or are in the process of being moved out into separate repositories (x/website, x/blog, x/playground, x/build, etc.). This issue outlines some of the remaining packages that support our production uses, and in turn pull in many external dependencies like appengine and cloud. As I understand, the goal of this issue is to investigate how viable it is to move the relatively few remaining pieces such that the x/tools repository can end up containing packages from categories 1 and 2 only. Then we can evaluate whether it makes sense to move those packages to the subrepositories where they fit better (e.g., x/tools/blog -> x/blog subrepo), and in turn make it possible for x/tools repository to consider the option of becoming a single Go library+command module. It may still turn out after doing this investigation that it is justifiable for x/tools to become a multi-module repository, but I think it'd be shame if that decision is forced on it simply because there are currently some remaining packages that happen to import external dependencies for production uses. |
This comment has been minimized.
This comment has been minimized.
I think module-level dependencies are a problem, not necessarily repo-level dependencies. Maybe we already agree on this? But here's my thinking: I don't think that module-level dependencies are always a problem, but I think it's important to be careful about which dependencies modules take on. Tools "proper" (categories 1 and 2 in @dmitshur's post) is intended to be used by many different tools created by the community. When a project or organization is taking on a dependency on tools, they need to download and perhaps vet/audit or and add to their organization module proxy all the modules. I don't think organizations should need to pull in cloud or appengine or even x/sys to use the code in tools.
Multi-module repositories are more complex than single-module repositories. There are some cases where multi-module repositories' complexity isn't a big issue: for instance a single repo that contains several independent top level directories which are modules isn't too hard to work with. On the other hand if tools itself was a module, and the repo contained another module, say, at cmd/tip, there's a lot more complexity. Users need to remember that dependencies from the cmd/tip module to the tools module would use the code in the module cache, not that in the tools repo checked out and immediately surrounding the code. And if dependency on the "outer" module is at a version before the "inner" go.mod was added, there could be two paths that satisfy the same import, and users need to reason which one will be used. Moving code between modules or changing module boundaries have even more complexities. We might be able to work around these issues, but to me it seems like it's worth doing the (relatively minimal) cleanup work to avoid those issues.
I'm probably doing a poor job of explaining this, so maybe we should meet offline to discuss, and share our conclusions here after? |
This comment has been minimized.
This comment has been minimized.
Thanks. That's a reasonable argument for how this would bite people. Does that argument still apply to putting go.mod files in leaf package main directories, like in cmd/html2article that don't have child directories with non-main packages? And what's the policy going forward for which x/foo => x/bar deps are allowed? In std we have go/build/deps_test.go. Are we looking to have a similar policy? |
This comment has been minimized.
This comment has been minimized.
It applies if those leaf package mains depend on other packages in the same repo: for instance, golang.org/x/tools/cmd/gorename is a leaf package that has dependences into tools (primarily on golang.org/x/tools/refactor/rename, which contains most of its logic). On the other hand, cmd/html2article has no dependencies into any other packages in tools: its only dependencies are on golang.org/x/net/html and golang.org/x/net/html/atom. So I think it might be safe to isolate in its own module, especially since it's stable and unlikely to take on imports on tools in the future. |
This comment has been minimized.
This comment has been minimized.
I wonder if @ianthehat has any opinions on multi-module repos in the case of a leaf package which has no dependencies on any modules in the rest of the repo. |
This comment has been minimized.
This comment has been minimized.
ianthehat
commented
Jan 31, 2019
Every single case of multi-module repositories I have seen so far has led to confusion of some form for some people, at this point I want to see a really strong reason to use them rather than searching for reasons not to. |
This comment has been minimized.
This comment has been minimized.
So in the end state that is being targeted here, what are the rules of when you're allow to depend on things? It'd be slightly sad if tools in x/tools could only depend on packages in x/tools. If all the tools move to their own modules, then x/tools is no longer tools but rather x/ast-util-packages-for-other-tools-to-depend-on-from-elsewhere. I'm fine breaking things up and creating new repos(==modules), but I want to understand where we're going first. |
This comment has been minimized.
This comment has been minimized.
ianthehat
commented
Feb 1, 2019
The trouble is there are three things conflated in this repository. 1: Packages that are essentially one step above the stdlib, things like /go/packages /go/analysis /go/ssa /imports etc which you should be able to depend on without pulling in the world. Everything in group 3 should just go somewhere else, the difficulties are in deciding what to do about the effect that group 2 has on the users of group 1. I actually think we should create a single new repository for the important go code handling libraries and move them out of x/tools so x/tools can go back to being the binaries. |
This comment has been minimized.
This comment has been minimized.
Agree on cleaning (3).
I was about to suggest the same but thought it'd be viewed as too crazy. Glad to hear you say it too. |
This comment has been minimized.
This comment has been minimized.
Agree with what @ianthehat says above. I just feel that the name of the repo 'tools' is more suitable for (2), and even (3) than (1). Also, if, if, if we decide to move the binaries in (2) and (3) to somewhere else that allows multi modules, the current package organization pattern that breaks |
This comment has been minimized.
This comment has been minimized.
Wow, @ianthehat that's bold! But it's probably the right choice... Though I don't know what the right modularization strategy for doing so is. I also wonder if we can do so without breaking everyone's imports of the libraries It seems like we all agree with (3), then. Doing that would remove most of the tools repo's dependencies. |
This comment has been minimized.
This comment has been minimized.
I just did the cmd/tip git-filter-branch+merge foo to move cmd/tip into x/build and sent @matloob and @dmitshur a tree to browse before I push: https://github.com/bradfitz/build/commits/merge-cmd-tip-from-tools/cmd/tip |
This comment has been minimized.
This comment has been minimized.
gopherbot
commented
Feb 1, 2019
Change https://golang.org/cl/160817 mentions this issue: |
pushed a commit
to golang/tools
that referenced
this issue
Feb 1, 2019
This comment has been minimized.
This comment has been minimized.
Of the things that remain,
This is moving to x/website anyway.
Moving, but I think a dependency to a peer x/foo repo is fine, no?
I'm fine with that dependency.
Seems like a useful tool to keep in x/tools. I'm fine with the indirect websocket dep in x/net.
App Engine stuff is behind a build tag at least? And websocket is in x/net, which I'm fine with. But if we changed this to use modern App Engine or Cloud Functions, then we don't even need the appengine imports. Of the appengine uses today:
I'm fine moving this binary, given that we already have x/blog anyway with code in the top level of x/blog that depends on x/tools/blog. Makes more sense to keep all its code together. |
This comment has been minimized.
This comment has been minimized.
The dependency on x/net pulls in x/crypto, x/sys, and x/text. It's a little unfortunate to have those dependencies, but we're already in a much better situation. I think we should remove the appengine dependencies, even though they're behind a build tag, especially since we can use the new appengine stuff now. |
This comment has been minimized.
This comment has been minimized.
The x/crypto one is solely because of the http2.golang.org demo site and an http/2 debugging tool, h2i:
But those living together with the http2 code makes sense. Perhaps http2 should be its own module somewhere? http2 depends on x/net/idna and x/net/http/httpguts. As for dependencies on x/sys, I'm not really worried about that. That's a tiny package and is effectively the syscall v2 package. Eventually everything will use that instead of syscall. |
This comment has been minimized.
This comment has been minimized.
Actually I'm not even seeing the x/sys dependency. Where's that? |
This comment has been minimized.
This comment has been minimized.
I don't see it as needed in
It may have disappeared due to a change in one of the indirect modules, but I'm not sure. Compare with the x/website go.mod which was created a week ago or so: https://github.com/golang/website/blob/d8ebd27297fa7c71a81f38716cc35c565263fa0d/go.mod#L10
|
This comment has been minimized.
This comment has been minimized.
gopherbot
commented
Feb 1, 2019
Change https://golang.org/cl/160837 mentions this issue: |
This comment has been minimized.
This comment has been minimized.
I've edited in the results of that investigation in that comment. See #29981 (comment).
Hiding something behind a build tag doesn't stop its dependencies from showing up in go.mod file. As far as I know, they show up there in order for all build configurations to be fully reproducible.
Sent CL 160837 for that.
It turned out the |
This comment has been minimized.
This comment has been minimized.
It's still weird that the blog code is split between two repos. If the split were on code vs content, sure, but the code is split. The x/tools/blog doesn't seem to be used from anywhere except x/blog too. |
This comment has been minimized.
This comment has been minimized.
I’m in favor of moving x/tools/blog to the x/blog subrepo, and I agree it belongs there. My point was just that it’s independent of the reducing-dependency reasons discussed in this issue. |
This comment has been minimized.
This comment has been minimized.
About x/sys: I don't see it anymore. I'm not sure why I was seeing it earlier... |
dmitshur
added
the
modules
label
Feb 6, 2019
bcmills
referenced this issue
Feb 12, 2019
Open
x/*: define module boundaries for all the repos under golang.org/x/ #28136
This comment has been minimized.
This comment has been minimized.
gopherbot
commented
Feb 13, 2019
Change https://golang.org/cl/162400 mentions this issue: |
This comment has been minimized.
This comment has been minimized.
gopherbot
commented
Feb 13, 2019
Change https://golang.org/cl/162401 mentions this issue: |
matloob commentedJan 29, 2019
•
edited by rsc
The following directories in x/tools have dependencies outside tools:
We should be able to remove the dependencies fairly easily, especially now that the website repo exists. Tools is a relatively low level repo and shouldn't have many external dependencies, especially on appengine and cloud.