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/exp: reconsider using single module for all unrelated experimental packages of various maturity levels #37175

Closed
dmitshur opened this issue Feb 11, 2020 · 8 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Feb 11, 2020

The golang.org/x/exp repository is described in its README as:

exp

This subrepository holds experimental and deprecated (in the old
directory) packages.

The idea for this subrepository originated as the pkg/exp directory
of the main repository, but its presence there made it unavailable
to users of the binary downloads of the Go installation. The
subrepository has therefore been created to make it possible to go get these packages.

Warning: Packages here are experimental and unreliable. Some may
one day be promoted to the main repository or other subrepository,
or they may be modified arbitrarily or even disappear altogether.

In short, code in this subrepository is not subject to the Go 1
compatibility promise. (No subrepo is, but the promise is even more
likely to be violated by go.exp than the others.)

Caveat emptor.

It currently contains various unrelated experimental packages that are not related to each other. Some of them are more experimental and less stable, while others are more stable and less experimental.

Back when GOPATH mode was the only build mode available, there was not a high cost to having many unrelated packages in one repository.

In module mode, having many unrelated packages in one module can contribute to increasing the size of the module graph.

For example, the cloud.google.com/go module requires golang.org/x/exp because it uses cmd/apidiff, which means all transitive module requirements of all packages in golang.org/x/exp are added to anyone's module that uses at least one Google Cloud Client library from cloud.google.com/go.

The module mirror (https://proxy.golang.org) helps protect users from individual dependencies being unavailable at the origin server, but at this time there are still some users that haven't started using it for various reasons. I expect that number will continue to go down over time. See various user feedback from a recent outage affecting one of golang.org/x/exp dependencies:

It can be argued that perhaps the cloud.google.com/go module should not be requiring golang.org/x/exp (that can still be investigated in a separate issue at https://github.com/googleapis/google-cloud-go/issues), but maybe there can be a better solution in golang.org/x/exp that would make it possible to keep separate packages and their dependencies more isolated.

One possible idea is to move the shiny project into a nested module at golang.org/x/exp/shiny, which would remove its dependencies from the main module. It's unclear how scaleable such a solution is or how applicable it would be in the general case, but it would likely help people who want to import cmd/apidiff.

Opening this issue to discuss and look for ways to improve the current situation if possible.

Other related issues:

/cc @bcmills @jayconrod @matloob @jba @dsymonds @robpike

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. modules labels Feb 11, 2020
@dmitshur dmitshur added this to the Unreleased milestone Feb 11, 2020
@dsymonds
Copy link
Contributor

It seems like cmd/apidiff should at least be moved to x/tools given its stability these days.

But separate go.mod files throughout x/exp seems good too, especially since that repository is prone to having dependencies outside the standard library and other x/ repos.

@jayconrod
Copy link
Contributor

I'd argue that packages in golang.org/x/exp should be promoted to their own repos or to other repos like x/tools if people depend on them being stable. cmd/apidiff may fit that description.

@bcmills is working on a change that would reduce the cost of a large module graph (both in load time and stability). Placeholder issue is #36460. So this may not be a justification for splitting modules in the future.

In general, multi-module repos complicate development in a lot of ways, and I think we should avoid that if we can.

@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

Nothing should depend on x/exp. If lots of things need cmd/apidiff, it sounds like it should move.

In general, x/ repos should not depend on non-golang repos. x/exp is an exception because it's not supposed to be important for people (see previous paragraph).

It would be fine to put a go.mod in x/exp/shiny today to contain the dependencies and not pollute the rest of x/exp that people should not be depending on.

In the longer term we need to work out the plan for x/ repos generally. I'm OK with a band-aid in x/exp because, again, people should not be depending on it.

x/tools is a bit of a dumping ground but maybe is the right home for cmd/apidiff at this point. It at least tends dependencies better.

The right fix for today is to put a go.mod in x/exp/shiny.

In the longer term, lazy loading of go.mods will make this situation not bother looking at shiny, because it doesn't matter for the build at hand.

@bcmills
Copy link
Contributor

bcmills commented Feb 12, 2020

To speak specifically to the point about lazy module loading: note that the module dependencies that provide all packages reachable from x/exp are a strict subset of the dependencies found in the module graph:

~/src/golang.org/x/exp$ go list -m all | sort -u
dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9
github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802
github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550
golang.org/x/exp
golang.org/x/image v0.0.0-20190802002840-cff245a6509b
golang.org/x/mobile v0.0.0-20190719004257-d2bd2a29d028
golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee
golang.org/x/net v0.0.0-20190620200207-3b0461eec859
golang.org/x/sync v0.0.0-20190423024810-112230192c58
golang.org/x/sys v0.0.0-20190412213103-97732733099d
golang.org/x/text v0.3.0
golang.org/x/tools v0.0.0-20200207183749-b753a1ba74fa
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898

~/src/golang.org/x/exp$ go list -f '{{with .Module}}{{.Path}} {{.Version}}{{end}}' all | sort -u
github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802
golang.org/x/exp
golang.org/x/image v0.0.0-20190802002840-cff245a6509b
golang.org/x/mobile v0.0.0-20190719004257-d2bd2a29d028
golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee
golang.org/x/tools v0.0.0-20200207183749-b753a1ba74fa
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898

@nigeltao
Copy link
Contributor

nigeltao commented Feb 4, 2022

The right fix for today is to put a go.mod in x/exp/shiny.

I'm not familiar with all the intricacies of go.mod, but as the original shiny author (and shiny was created at a time before go.mod or even before github.com/golang IIRC), I'd also be fine if moving shiny out of x/exp would help (e.g. to github.com/golang/shiny similar to github.com/golang/snappy being a stand-alone project).

@gopherbot
Copy link

Change https://golang.org/cl/383197 mentions this issue: shiny: add go.mod file

gopherbot pushed a commit to golang/exp that referenced this issue Feb 5, 2022
The shiny package is not related to other exp packages.
Put it in a separate module.

This package uses a modified copy of github.com/BurntSushi/xgb.
Move that out of the vendor directory, so that current go commands work.

For golang/go#37175

Change-Id: Ib3bad0823d2651ff90a9651e405ed7bcd9de94b6
Reviewed-on: https://go-review.googlesource.com/c/exp/+/383197
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Trust: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/exp that referenced this issue Feb 18, 2022
Clean up dependencies after moving shiny, event, and jsonrpc2 to
separate modules.

For golang/go#37175

Change-Id: Ie72d0eb4b99a625f41615d6ce3bb5c8935eb11c4
Reviewed-on: https://go-review.googlesource.com/c/exp/+/386795
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/386795 mentions this issue: go.mod: run go mod tidy

@dmitshur
Copy link
Contributor Author

With recent changes, x/exp is down to requiring just 2 golang.org/x modules directly, and 2 more indirectly. There are a number of nested modules that contain distinct parts of x/exp and their dependencies.

I don't think there's anything more left to do in this issue, so closing.

@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 Feb 19, 2022
@golang golang locked and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants