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

x/tools: clean up dependencies #29981

Closed
3 of 8 tasks
matloob opened this issue Jan 29, 2019 · 34 comments
Closed
3 of 8 tasks

x/tools: clean up dependencies #29981

matloob opened this issue Jan 29, 2019 · 34 comments
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@matloob
Copy link
Contributor

matloob commented Jan 29, 2019

The following directories in x/tools have dependencies outside tools:

  • cmd/tip (a whole bunch of stuff)
  • cmd/godoc (appengine, cloud) also redigo through internal/memcache
  • cmd/html2article (x/net/html)
  • cmd/present (transitively through playground)
  • playground/... (appengine, websocket)
  • blog (transitively through present)
  • godoc (golang.org/x/net/ctxhttp) also redigo through internal/memcache) (edit 2019-02-20: internal/memcache dependency is gone)
  • internal/memcache (github.com/gomodule/redigo)

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.

@matloob matloob self-assigned this Jan 29, 2019
@gopherbot gopherbot added this to the Unreleased milestone Jan 29, 2019
@matloob matloob assigned dmitshur and unassigned matloob Jan 29, 2019
@dmitshur
Copy link
Contributor

dmitshur commented Jan 29, 2019

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:

  • cmd/present (appengine, x/net/websocket transitively through playground and playground/socket) - in appengine mode, it’s used to serve talks.golang.org
  • playground (appengine) - used by x/blog, x/tour and x/talks (also cmd/godoc, but that’s moving to x/website); can consider moving it to golang.org/x/playground/handler
  • playground/socket (x/net/websocket) - used by x/tour (in local mode) and x/tools/cmd/present (both in local mode, and in appengine mode)

Others:

  • cmd/html2article (x/net/html) - still need to decide what to do with it

  • blog (only stdlib imports) - no longer an issue, it only imports stdlib

  • godoc/redirect (x/net/ctxhttp) - can trivially stop using x/net/ctxhttp by manually implementing (a subset of) what ctxhttp.Get does; see below:

godoc/redirect diff
diff --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 dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 29, 2019
@bradfitz
Copy link
Contributor

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?

@dmitshur
Copy link
Contributor

dmitshur commented Jan 30, 2019

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:

  1. libraries for working with Go code (go/types, astutil, imports, etc.),
  2. commands that work with Go code (cmd/goimports, cmd/gorename, etc.),
  3. tools that support the Go project in production (cmd/godoc in appengine mode that powers golang.org, godoc/short in appengine mode that imports cloud.google.com/go/datastore to implement the golang.org/s URL shortener, cmd/tip which powers tip.golang.org, x/tools/playground package in appengine mode that imports google.golang.org/appengine, etc.)

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.

/cc @hyangah @matloob

@matloob
Copy link
Contributor Author

matloob commented Jan 30, 2019

This bug is making the assumption that repo-level dependencies are a problem.

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.

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.

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.

Before we clean things up, can we at least agree on the problem?

I'm probably doing a poor job of explaining this, so maybe we should meet offline to discuss, and share our conclusions here after?

@bradfitz
Copy link
Contributor

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.
...
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.

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?

@matloob
Copy link
Contributor Author

matloob commented Jan 31, 2019

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?

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.

@matloob
Copy link
Contributor Author

matloob commented Jan 31, 2019

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.

@ianthehat
Copy link

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.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

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.

@ianthehat
Copy link

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.
2: Actual tools, main packages, which should be free to pull in just about anything you like.
3: Random stuff that ended up here because it was the easiest place to drop it

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 would argue that the contents of group 2 all needs to be versioned separately from group 1 anyway, and thus should be in a different module. That means either a multi-module repository, or moving them out to a separate repositor(y|ies).

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.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

Agree on cleaning (3).

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.

I was about to suggest the same but thought it'd be viewed as too crazy. Glad to hear you say it too.

@hyangah
Copy link
Contributor

hyangah commented Feb 1, 2019

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 cmd and library (e.g. /cmd/godoc and /godoc) need to be rethought. It's desirable to release the godoc binary and related godoc library in one module, but the directory structure prevents it.

@matloob
Copy link
Contributor Author

matloob commented Feb 1, 2019

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.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

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

@gopherbot
Copy link

Change https://golang.org/cl/160817 mentions this issue: cmd/tip: delete

gopherbot pushed a commit to golang/tools that referenced this issue Feb 1, 2019
The cmd/tip program moved to x/build (along with its history) in:

  https://go.googlesource.com/build/+/63986c177d1ff5d2629840f6b00c445f1cb932bf

(There's no associated Gerrit CL; the merge commit of x/build's old
HEAD + a git-filter-branch of x/tools's cmd/tip was pushed directly to
Gerrit's git server, without creating Gerrit CLs for review for each
commit in its history)

Updates golang/go#29981

Change-Id: I16b9b1b0079e3d7b6851cb3e7322a878ece73e23
Reviewed-on: https://go-review.googlesource.com/c/160817
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

Of the things that remain,

cmd/godoc (appengine, cloud)

This is moving to x/website anyway.

godoc (golang.org/x/net/ctxhttp)

Moving, but I think a dependency to a peer x/foo repo is fine, no?

cmd/html2article (x/net/html)

I'm fine with that dependency.

cmd/present (transitively through playground)

Seems like a useful tool to keep in x/tools. I'm fine with the indirect websocket dep in x/net.

playground/... (appengine, websocket)

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:

appengine.go:   "google.golang.org/appengine" // IsDevAppServer, NewContext
appengine.go:   "google.golang.org/appengine/log"
appengine.go:   "google.golang.org/appengine/urlfetch"
appengine.go:   onAppengine = !appengine.IsDevAppServer()
  • IsDevAppServer is trivial to define locally
  • NewContext is no longer necessary
  • log is no longer special. just log.Printf to stderr is fine.
  • urlfetch is no longer required. regular http.Client works.

blog (transitively through present)

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.

@matloob
Copy link
Contributor Author

matloob commented Feb 1, 2019

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.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

The dependency on x/net pulls in x/crypto, x/sys, and x/text

The x/crypto one is solely because of the http2.golang.org demo site and an http/2 debugging tool, h2i:

bradfitz@gdev:~/src/golang.org/x/net$ git grep x/crypto
http2/h2demo/Dockerfile.0:# Repo golang.org/x/crypto at 1875d0a (2018-01-27)
http2/h2demo/Dockerfile.0:RUN go get -d golang.org/x/crypto/acme `#and 2 other pkgs` &&\
http2/h2demo/Dockerfile.0:    (cd /go/src/golang.org/x/crypto && (git cat-file -t $REV 2>/dev/null || git fetch -q origin $REV) && git reset --hard $REV)
http2/h2demo/Dockerfile.0:      golang.org/x/crypto/acme \
http2/h2demo/Dockerfile.0:      golang.org/x/crypto/acme/autocert \
http2/h2demo/h2demo.go: "golang.org/x/crypto/acme/autocert"
http2/h2i/h2i.go:       "golang.org/x/crypto/ssh/terminal"

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.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

Actually I'm not even seeing the x/sys dependency. Where's that?

@dmitshur
Copy link
Contributor

dmitshur commented Feb 1, 2019

I don't see it as needed in x/tools now either. Here's what I get now:

require (
	cloud.google.com/go v0.35.1
	github.com/gomodule/redigo v2.0.0+incompatible
	golang.org/x/crypto v0.0.0-20190131182504-b8fe1690c613
	golang.org/x/net v0.0.0-20190125091013-d26f9f9a57f3
	google.golang.org/appengine v1.4.0
)

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

go mod why -m says:

$ go mod why -m golang.org/x/sys
# golang.org/x/sys
golang.org/x/website/cmd/golangorg
cloud.google.com/go/datastore
google.golang.org/grpc
google.golang.org/grpc/internal/channelz
golang.org/x/sys/unix

@gopherbot
Copy link

Change https://golang.org/cl/160837 mentions this issue: playground: use stdlib instead of appengine packages

@dmitshur
Copy link
Contributor

dmitshur commented Feb 1, 2019

I will investigate this and update the original issue with a plan for how this can be resolved.

I've edited in the results of that investigation in that comment. See #29981 (comment).

App Engine stuff is behind a build tag at least?

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.

But if we changed this to use modern App Engine or Cloud Functions, then we don't even need the appengine imports.

Sent CL 160837 for that.

blog (transitively through present)

It turned out the golang.org/x/tools/blog package imports (even indirectly) only standard library packages (and packages in x/tools), so nothing needs to be done to it for the purposes of this issue.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2019

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.

@dmitshur
Copy link
Contributor

dmitshur commented Feb 1, 2019

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.

@matloob
Copy link
Contributor Author

matloob commented Feb 4, 2019

About x/sys: I don't see it anymore. I'm not sure why I was seeing it earlier...

@gopherbot
Copy link

Change https://golang.org/cl/162400 mentions this issue: cmd/godoc: remove golang.org serving code

@gopherbot
Copy link

Change https://golang.org/cl/162401 mentions this issue: godoc/dl, godoc/proxy, godoc/short, internal/memcache: delete

gopherbot pushed a commit to golang/tools that referenced this issue Feb 20, 2019
The code to serve the golang.org website has been moved to
the golang.org/x/website sub-repository. x/website has become
the canonical source of the golang.org website as of CL 162157,
and so this code can be removed from here now.

This has the benefit of removing some external dependencies
that were only used by the website in production mode, and
in turn enabling x/tools to be a smaller tools-related module.

In future changes, the golang.org/x/tools/cmd/godoc command
will be reduced in scope to be a tool for serving Go package
documentation only, not the rest of the golang.org website.

Run go mod tidy (using Go 1.12 RC 1).

Updates golang/go#29206
Updates golang/go#29981

Change-Id: I61fd25627d0506901b04688dea8d8c9da9fe8f04
Reviewed-on: https://go-review.googlesource.com/c/162400
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Channing Kimble-Brown <channing@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 20, 2019
These packages existed only to power cmd/godoc for the purpose of
serving the golang.org website. That functionality has moved into
x/website as part of golang/go#29206. x/website has become the
canonical source of golang.org in CL 162157, the golang.org-serving
code was removed from cmd/godoc in CL 162400, and these packages can
be deleted too now.

This removes the last dependency on the cloud.google.com/go module,
which results in a significant reduction of the number of indirect
dependencies in x/tools (this is due to issue golang/go#29935, which
affects the current version of the cloud.google.com/go module).

Run go mod tidy (using Go 1.12 RC 1).

Updates golang/go#29206
Updates golang/go#29981

Change-Id: If07e3ccae8538b3ebd51af64b6af5be5463f4906
Reviewed-on: https://go-review.googlesource.com/c/162401
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Channing Kimble-Brown <channing@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@matloob
Copy link
Contributor Author

matloob commented Feb 20, 2019

Update: thanks to @dmitshur's two changes above, we're now down to direct dependencies on the appengine and net modules. The appengine dep will be removed it @dmitshur's cl: golang.org/cl/160837, which switches to using the new appengine api.

gopherbot pushed a commit to golang/tools that referenced this issue Mar 7, 2019
With modern versions of App Engine, it's no longer needed to use the
google.golang.org/appengine/... packages.

Package log from standard library can be used instead of the
google.golang.org/appengine/log package. Packages net/http and
context from standard library can be used instead of
google.golang.org/appengine/urlfetch.

This simplifies the code and reduces the number of dependences.

Start using the golangorgenv package from previous commit to
make the decision of whether to enforce sharing restrictions,
rather than relying on the appengine build tag. The appengine
build tag is no longer set in App Engine Standard with Go 1.11+
runtime. An alternative solution would be detect App Engine by
doing something like:

	// GAE_ENV environment variable is set to "standard" in App Engine environment, Go 1.11 runtime.
	// See https://cloud.google.com/appengine/docs/standard/go111/runtime#environment_variables.
	var onAppengine = os.Getenv("GAE_ENV") == "standard"

But we choose to upgrade to explicit app-scoped environment variable
configuration as part of this change. It provides better security
properties, and the value of adding an intermediate transitional step
is not high enough to justify doing it.

When getting the value of "X-AppEngine-Country" header, use its
canonical format "X-Appengine-Country" to avoid an allocation.
This does not change behavior.

Run go mod tidy (using Go 1.12).

Updates golang/go#29981
Updates golang/go#30486

Change-Id: I82a59e0f28623e06762b7ebdf3930b5ee243acda
Reviewed-on: https://go-review.googlesource.com/c/tools/+/160837
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@dmitshur
Copy link
Contributor

dmitshur commented Mar 8, 2019

The above has been done, and the go.mod of x/tools is down to a single module requirement now.

I think the next important piece of this puzzle is resolving #30685.

@gopherbot
Copy link

Change https://golang.org/cl/169998 mentions this issue: [release-branch.go1.12] playground: use stdlib instead of appengine packages

@gopherbot
Copy link

Change https://golang.org/cl/170000 mentions this issue: [release-branch.go1.12] cmd/godoc: remove golang.org serving code

@gopherbot
Copy link

Change https://golang.org/cl/170001 mentions this issue: [release-branch.go1.12] godoc/dl, godoc/proxy, godoc/short, internal/memcache: delete

@dmitshur
Copy link
Contributor

Issue #30685 has been resolved by now, and I think this issue is resolved too. @matloob Please let me know if there are any actionable tasks left here. If not, we can close this.

@matloob
Copy link
Contributor Author

matloob commented Jul 1, 2019

I think we're ready to close this!

@dmitshur dmitshur closed this as completed Jul 1, 2019
@golang golang locked and limited conversation to collaborators Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants