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 a mechanism to access build-system specific information about a package #38448

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

Comments

@matloob
Copy link
Contributor

@matloob matloob commented Apr 14, 2020

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

We propose evaluating the possibility of adding a field to the Package struct containing build-system specific information. It needs to be discussed what type this field would have. One possibility is to just provide go build specific information in a field, which is nil if another build system is being used. Another is to make an interface{} field whose concrete type depends on the build system, but it's unclear where the definitions of those concrete types would go: at the least, we'd want the type corresponding to go build to be defined somewhere in x/tools and versioned with go/packages.

If this is fully sketched out and agreed upon, and added to go/packages, the field would also need a Need bit, and users of the package given the expectation that they can't always guarantee the build system chosen by go/packages.

@mvdan
Copy link
Member

@mvdan mvdan commented Apr 15, 2020

the field would also need a Need bit

I just realised what's possibly a major disadvantage of this proposal versus the other two. "build-system specific information" could be a lot of information, and it could take a lot of work to retrieve it all. A single Need bit might end up requiring a lot of work that the user might not need.

Another option, however, would be to simply say - the specific information only contains what go/packages received from the build system, following the queries it had to do because of the other Need bits. That is, with just NeedName | NeedSpecificInfo, the specific info might be mostly empty.

@matloob
Copy link
Contributor Author

@matloob matloob commented Apr 15, 2020

Overall this issue is a bit more hazy than the other two, and unlike the other two I'm not completely convinced it's a good idea. It really depends on the details. We'd need a more specific instance of this proposal to examine so we can determine whether it's worth doing.

Another option, however, would be to simply say - the specific information only contains what go/packages received from the build system, following the queries it had to do because of the other Need bits. That is, with just NeedName | NeedSpecificInfo, the specific info might be mostly empty.

That sounds reasonable. Another option is for the build system specific info to return a subset of data that's always cheap to fetch.

@dominikh
Copy link
Member

@dominikh dominikh commented Apr 23, 2020

I was originally in favour of this idea, but I've discovered the same issue that mvdan has. A boolean flag wouldn't be fine-grained enough, and we can't add many per-build-system flags to control what data is returned by the build system.

Only returning the data we already have, anyway, might work, but it could also quickly lead to users asking for "just one more bit of information" to be included. It would also leak part of the implementation, and prevent us from changing what information is acquired. For example, if we found a more efficient way of answering a query, it might contain less information than before.

The only benefit this proposal has over proposal 1 is that it's "cleaner", with no information specific to any single build system leaking into the agnostic API. But technically speaking, it is already too late for that, because we have packages.Config.Tests.

Maybe we should just have a field that reports the detected build system and call it a day? We could even add a new kind of query that only detects the build system and does nothing else. Users with very special requirements could then interact with the build system directly.

@matloob
Copy link
Contributor Author

@matloob matloob commented Apr 23, 2020

I think we should proceed by getting a list of the specific fields we need. Then we can more easily make a reasoned decision about whether there's a useful subset of those we can add directly to Package as per proposal 1 without bloating it. If we can, we're done and can close this issue! Otherwise we need to make a harder decision about whether we want to go forward with this proposal.

I'm also worried about the "just one more bit of information" problem. If reporting the detected build system is "enough" of a substitute, I'm okay with that... But I wonder if that just means we have to go back to the proposal of building a wrapper for go list?

@dominikh
Copy link
Member

@dominikh dominikh commented Apr 23, 2020

But I wonder if that just means we have to go back to the proposal of building a wrapper for go list?

Yes.

@matloob
Copy link
Contributor Author

@matloob matloob commented Apr 24, 2020

But I wonder if that just means we have to go back to the proposal of building a wrapper for go list?

Yes.

It might be unavoidable, but I'd really like to avoid it if possible.

@matloob
Copy link
Contributor Author

@matloob matloob commented May 13, 2020

I'm hoping that fixing #38445 (adding some build-system-specific fields to Package) will provide users enough information so that this proposal isn't needed. So I'd like to hear (preferably there) whether there are any important fields that are missing.

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

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
4 participants