golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.05k stars 17.68k forks source link

x/tools/go/packages: tags proposal / design inquery #26697

Closed kardianos closed 6 years ago

kardianos commented 6 years ago

Regarding package golang.org/x/tools/go/packages.

I'm looking forward to Go modules. When go1.11 lands I hope to convert some work projects over from govendor to go modules. At work we have a policy to review all dependencies. We currently facilitate this by reviewing the vendor directory. One main difference between the govendor vendor tree and the Go modules vendor tree is that govendor doesn't copy un-imported packages. Furthermore, it also has a custom package loader that allows flexibly choosing tags, GOOS, and GOARCH. In order to make this govendor -> Go modules go smoothly, I plan to make a tool that trims out the unused packages from the vendor folder.

To trim out the unused packages from the vendor folder, I plan to:

There are two problems with using the new packages package for this purpose:

  1. It requires a specific GOOS and GOARCH. I would Ideally like to analyze the full set of packages regardless of GOOS and GOARCH (though I'd like to potentially black-list some GOOS similar to tags below).
  2. I don't see any method to specify build tags. I'm unsure if this can be passed into Env or Flags fields, but I would like to see this more flexible. govendor by default includes all tags, but allows excluding tags, GOOS, GOARCH, test.

I will avoid speculating on a design that would allow these features until I hear back to see if these would even be in scope. If not in scope I'll write a version that supports what I need directly.

/cc @suzmue

suzmue commented 6 years ago

/cc @matloob @ianthehat

matloob commented 6 years ago

(a) is difficult to do: module resolution depends on the imports in your build, which in turn depends on GOOS and GOARCH. I'm not sure how we could design around that. It seems like the only option is to run through the GOOS x GOARCH product and re-Load the packages in each configuration... (b) We were planning to add tags as an option, but decided that it would be cleaner to just let them be passed in through the Flags field. Perhaps we should add a comment to Flags to indicate where we'd pass in the flags?

kardianos commented 6 years ago

It seems like the only option is to run through the GOOS x GOARCH product and re-Load the packages in each configuration...

It really depends on what you are trying to do. If you are attempting to calculate the maximum dependency graph (as I am here with exceptions), then you can just read all relevant files to do so. However, most tools (go, others) that this probably shells out to (I'm assuming) won't accept that.

Between this assumption of GOOS and GOARCH and the tags being loaded into the Flags, upon further investigation the design of packages is really a papering over of other build tools with some useful hooks. If this is accurate then what I'm trying to do will be out of scope (which is fine).


On a site note: If this is intended to be a general purpose package loader tools such as guru or other code refactoring tools should use, I would air a few concerns:

I could be mis-understanding the intended user of this package too (I thought it would be used by general purpose tools like goimports and guru).

If our interests align, I could certainly work on a part of this, but I don't want to step on toes either.

ianthehat commented 6 years ago

To really answer your question I would need to know slightly more about your goals, and their granularity.

If you are using go in module mode, then it ignores the vendor folder, so any kind of checks on the vendor folder are not going to be much use to you. Assuming you know that and you are just using the vendor folder as a way of materializing information about your build, there are a few options available to you:

It is not really feasible for us to process all build configurations, because the number of possible combinations is too large to do them all, but trying to do it for all configurations at once can result in a cyclic graph (it is valid to have a dependency edge flip direction depending on tagged out files, and I have seen an example of someone doing it). It would also not be possible to run any of the higher modes because the source list would not type check, so it would have to be a mode dependent option, something we have been trying to avoid.

kardianos commented 6 years ago

@ianthehat (real quick background: I'm the author of govendor, I've made some minor contributions to vgo and generally understand the code there, I understand what flags are available, and I understand that -sync does a maximum graph resolution while go build inspects GOARCH and GOOS).

To really answer your question I would need to know slightly more about your goals, and their granularity.

I spelled out my exact goals. I'm not sure what more to tell you.

because pruning is at the module level not the package level.

Yes, I specifically called this out. I'm looking for package level pruning with custom tag/GOOS/GOARCH exclusions.

If you just want to check the module dependencies, you can do that with go list -m all or go mod -graph

This doesn't work because I'm not looking for the pure maximal graph, as I'd like to prune based on tags/GOOS/GOARCH.

walk that information for a specific build configuration.

That's a non-starter. It really is. If this is going to be used by guru (goimports might be fine), this design concerns me. But it also isn't my call :).

It is not really feasible for us to process all build configurations,

Right. That's not a good idea. (So don't do that?) You know how go mod -sync walks the source tree differently then go build? I'm looking for a variant of how go mod -sync walks the tree. You are implementing packages like how go build walks the source tree.

It would also not be possible to run any of the higher modes because the source list would not type check, so it would have to be a mode dependent option, something we have been trying to avoid.

So again, I'll don't think you can avoid the difference; it is inherit complexity. guru will need all related symbols so it can truly do a implements or rename function properly. But a build tool or a type checker must resolve to a concrete GOOS and GOARCH. The difference isn't large in resolution details, but it is fundamental and needed.

I'll close the issue as I'm not interested in a package resolver that requires a GOOS and GOARCH.

Thanks for your time.

kardianos commented 6 years ago

I stand corrected. Existing tools today (guru, gorename) only use a single GOOS x GOARCH. yikes. I learn something new each day. :/

ianthehat commented 6 years ago

I used govendor on multiple projects in the past (thanks btw!) and recognized your name from the import path :) I was just trying to be very explicit mostly for anyone else reading along. There seems to be some confusion about the granularity of repository > module > package right now, so I am being careful in my responses to be very clear, mostly because these comments live forever and turn up in searches.

As you say, "a type checker must resolve to a concrete GOOS and GOARCH"; all existing code inspection tools (godef, guru etc) and refactoring tools (gorename, eg etc) that I know of require type information to work, so they all run in a single configuration. In theory one could write a version of the types package that could cope, but in practice the extra complexity in the API (allowing multiple edges where the language only allows one) would be a hard sell to people that mostly just don't care about multiple configurations. We also report and use the export data, which is inherently single configuration, and because go list reports the processed cgo files, that also needs to be done in a specific configuration.

For inspection tools, it's never really a problem, editing/debugging in the default configuration is what most people expect if they even think about it at all. It is normally possible to modify the configuration if really needed.

For refactoring tools its a much more serious problem, because refactoring in a single configuration causes breaking changes to the other configurations. I don't know of any attempts to fix this at the moment, but we do have some ideas about the right way to write these kinds of tools, it involves a higher level API that probably sits on top of go/packages and probably runs it multiple times making safe incremental changes each time.

Expanded package graphs are a rare enough use case that they probably need custom code, you are probably right to not use the go/packages API. We are trying to keep this API as simple as possible for the common go source inspection tools that people tend to write, we explicitly chose to leave out some power use cases from this API to keep the simple common case easy. If you can see a good way to add it, we would welcome suggestions, the design is far from set in stone, it is explicitly in the x/tools repository so that we are able to make changes based on people really trying to use it.

We may add further more powerful APIs to a different package in the future, and would be happy to collect any thoughts you have about what those APIs need to expose, and good ways to expose it.