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

proposal: x/tools/go/packages: add Dir and Target (and perhaps other) fields to Package struct #38445

Open
matloob opened this issue Apr 14, 2020 · 15 comments
Labels
Projects
Milestone

Comments

@matloob
Copy link
Contributor

@matloob matloob commented Apr 14, 2020

This is proposal 1 of 3 coming out of the discussion of issue #38234.

We propose adding the following fields to the Package struct, which might not necessarily be present for all build systems:

  • Dir, which would be the directory associated with the package, if any. For the go build system, this would be the directory the package is contained in. This might not be as clearly defined for Bazel, but might be the directory the BUILD file is in.
  • Target, which is the install path of the .a file, for libraries, and executable file for binaries,

and perhaps other such fields, pending the discussion. If these fields are added, we'd need to also add Need fields. We'd also need to make it clear to users that these fields might not be set, even if they are requested via needs fields.

@gopherbot gopherbot added this to the Proposal milestone Apr 14, 2020
@matloob matloob changed the title proposal: go/packages: add Dir and Target (and perhaps other) fields to Package struct proposal: x/tools/go/packages: add Dir and Target (and perhaps other) fields to Package struct Apr 14, 2020
@dominikh
Copy link
Member

@dominikh dominikh commented Apr 18, 2020

I would really like to see ForTest exposed, but that one is only defined for Go's own build system.

@myitcv
Copy link
Member

@myitcv myitcv commented Apr 18, 2020

Agreed on ForTest. It's exposed internally and used in gopls, which confirms to my mind it's useful. If necessary, this could be a build system-specific field.

@matloob
Copy link
Contributor Author

@matloob matloob commented Apr 20, 2020

I think if the proposal to add the field with build system specific information as accepted, ForTest would best belong there. Otherwise, I could see an argument to put it directly on Package. I just want to be careful about the number of fields we add to Package-- I'm a bit worried it would grow too large.

@dominikh
Copy link
Member

@dominikh dominikh commented Apr 23, 2020

I'm a bit worried it would grow too large

If proposal 3 doesn't get accepted, then I would argue that go/packages should support the important features of the most common build system, which is go list.

@matloob
Copy link
Contributor Author

@matloob matloob commented Apr 23, 2020

If proposal 3 doesn't get accepted, then I would argue that go/packages should support the important features of the most common build system, which is go list.

That's reasonable. I think we should probably do one or the other.

@dominikh
Copy link
Member

@dominikh dominikh commented May 10, 2020

Another field I would deem useful is Match:

Match []string // command-line patterns matching this package

This would allow tools to emulate Go's behavior of warning about patterns that don't match any packages, a la go: warning: "./foobar/..." matched no packages.

Whether me needing yet another field pushes us more towards proposal 3 I don't know.

@mvdan
Copy link
Member

@mvdan mvdan commented May 10, 2020

Another one would be a package's build ID, once/if #37281 is implemented. We could call the field BuildID, but if we want something more generic, we could also go for CacheKey.

@matloob
Copy link
Contributor Author

@matloob matloob commented May 11, 2020

Hm, I'm thinking that BuildID is so low-level that it's reasonable to ask users to run go list -json themselves.

Match is reasonable: it was a pretty early request when we were developing go/packages.

@dominikh
Copy link
Member

@dominikh dominikh commented May 11, 2020

Hm, I'm thinking that BuildID is so low-level that it's reasonable to ask users to run go list -json themselves.

The issue, as so often, is performance: we want to use go/packages to be build-system agnostic, but we also want low-level details when available, without incurring the cost of a second go list invocation. In particular, our desire to get the BuildID is as an optimization to avoid computing our own hash of build inputs; but computing the hash is still much faster than calling go list twice (once via go/packages, once directly).

@heschi
Copy link
Contributor

@heschi heschi commented May 11, 2020

I thought Match wasn't implementable in Bazel?

@matloob
Copy link
Contributor Author

@matloob matloob commented May 11, 2020

It isn't. If this proposal is accepted, we'll relax our requirement that all of Package's fields correspond to concepts that exist in every build system. It'll be important to set the expectation for users that the fields won't be filled for every build system.

We'll have to be careful about this to make sure that we don't end up with too many tools that don't work on Blaze/Bazel.

@matloob
Copy link
Contributor Author

@matloob matloob commented May 13, 2020

I'm hoping that fixing this (adding some build-system-specific fields to Package) will provide users enough information so that #38448 isn't needed. So I'd like to hear if there are any other build-system-specific fields users need.

I think we should have a plan for what we're going to do with both proposals before proceeding.

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 21, 2020

@matloob ping - has this advanced in the past few months? Or are you waiting for more input from users?

@jpap
Copy link
Contributor

@jpap jpap commented Nov 24, 2020

I'd love to see a Dir field here also -- "go/build".Packages has it, and it would be very useful here too.

I am currently using a kludgey workaround:

  • Request NeedFiles
  • Perform the Load
  • Pick off the first GoFiles[0], and extract the path, relying on the fact that GoFiles represent the absolute path to each file within the package.

At first I did try to retrieve the Module with NeedModule, but that of course doesn't work with GOPATH packages.

@dominikh
Copy link
Member

@dominikh dominikh commented May 29, 2021

I'd like to echo mvdan's ping and vote to move forward with adding the following fields:

  • Dir
  • Target
  • ForTest
  • Match
  • BuildID

The fields which are not viable across build systems can be documented and handled as such by tools. For example, in my specific case, Match would be used for improved UI, and BuildID as an optimization, both of which can be optional.

We might need a new field, however, that stores which build system specific fields were populated, as an empty Match is a valid value and difficult to differentiate from "Match wasn't populated".

I don't think that #38448 is viable due to the bad performance of tying all information to a single Need bit.

Furthermore, it has been 3 years since the introduction of go/packages, and exactly one public driver exists, go list. I think it is fair to say that the number of expected Go build systems is extremely finite, and we won't run the risk of having to add dozens of new fields and bits for other build systems.

I'd also like to assume that there will be enough overlap between the fields of different build systems, the same way we already assume for the current fields in type Package. That is, these additional fields might not stay specific to go list forever.

Finally, go/packages already has several features specific to go list:

  • Config.Tests
  • Config.Overlay (assuming that this cannot be efficiently implemented in all conceivable build systems)
  • Package.Module

I think it is safe to say that go/packages has already adopted a "go list first" attitude. Adding several more highly useful fields won't change that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants