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

Go: consider redesigning third-party metadata to only be what's used #14071

Closed
Eric-Arellano opened this issue Jan 4, 2022 · 4 comments
Closed

Comments

@Eric-Arellano
Copy link
Contributor

Problem

Our current approach for Go is to run go list ... to see the entire universe of third-party packages in the project. We use it to generate hundreds of go_third_party_package targets. From there, we use dependency inference to map particular import statements to those packages. Many of these packages will never get used.

list_argv = (
"list",
# This rule can't modify `go.mod` and `go.sum` as it would require mutating the workspace.
# Instead, we expect them to be well-formed already.
#
# It would be convenient to set `-mod=mod` to allow edits, and then compare the resulting
# files to the input so that we could print a diff for the user to know how to update. But
# `-mod=mod` results in more packages being downloaded and added to `go.mod` than is
# actually necessary.
# TODO: nice error when `go.mod` and `go.sum` would need to change. Right now, it's a
# message from Go and won't be intuitive for Pants users what to do.
"-mod=readonly",
# There may be some packages in the transitive closure that cannot be built, but we should
# not blow up Pants.
#
# For example, a package that sets the special value `package documentation` and has no
# source files would naively error due to `build constraints exclude all Go files`, even
# though we should not error on that package.
"-e",
"-json",
# This matches all packages. `all` only matches first-party packages and complains that
# there are no `.go` files.
"...",
)

The major downside of this approach is that it requires go.mod and go.sum to have far more entries than are actually necessary for the project, otherwise go list will error (due to us setting -mod=readonly). Even if we have Pants start to generate lockfiles for users, this is still a big downside: your go.mod/go.sum will have more entries than necessary, and the Go ecosystem tolerates but does not like that. In particular, go mod tidy will cause those files to no longer work - many tools want you to run go mod tidy, including VSCode out-of-the-box.

Possible solution

I wonder what it would look like if we assumed the user had just run go mod tidy - can we get Go to give us back the metadata we need without resolving unnecessary metadata? What commands specifically would we run? TBD.

We would need to figure out how to handle if go mod tidy had not been run, i.e. that go.mod/go.sum are stale.

See #13352 for prior research here.

@ryanking
Copy link
Contributor

ryanking commented Jan 5, 2022

Assuming that go.mod/sum are up to date, what's wrong with reading go.sum to get a list of dependencies?

@Eric-Arellano
Copy link
Contributor Author

what's wrong with reading go.sum to get a list of dependencies?

We need more metadata than that, such as knowing what dependencies a certain third-party Go package has on other packages. To build third-party packages, we compile it like any other code - we need to recursively compile dependencies first.

But possibly we could use go.sum to do something like run go list $pkg instead of go list ....

@Eric-Arellano
Copy link
Contributor Author

It looks like our current approach of go list ... does not play nicely with Go 1.17+. Because of its change to expect indirect dependencies declared in the go.mod, it's much more finicky to get go.mod working.

A user shared their go.mod/go.sum with me and go list -mod=mod ... would not actually update the go.mod, but then running go list -mod=readonly ... right after would error and suggest running go mod tidy. Frustrating. The fix was to use Go 1.16 for now, but that's not acceptable long term.

@Eric-Arellano
Copy link
Contributor Author

This was fixed by @tdyas in #14157. Yay, thank you Tom!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants