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/tools/go/packages: add module information to the Package struct #35921

Closed
a8m opened this issue Dec 1, 2019 · 12 comments
Closed

x/tools/go/packages: add module information to the Package struct #35921

a8m opened this issue Dec 1, 2019 · 12 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@a8m
Copy link
Contributor

a8m commented Dec 1, 2019

Currently, packages.Load returns a packages.Package object that contains useful information from the go list API. However, sometimes the module information for the requested package is needed as well.

My proposal is basically to add the modinfo.ModulePublic info from go list to this package.

Similar to go list, if the package is not part of a module or it's provided by the Overlay API, Package.Module will be nil.

Let me know what do you think about this and I'll submit my CL for the proposal.

@gopherbot gopherbot added this to the Proposal milestone Dec 1, 2019
@zikaeroh
Copy link
Contributor

zikaeroh commented Dec 1, 2019

This would probably make #35563 much simpler to implement.

@rsc rsc changed the title proposal: x/tools/go/packages add modinfo.ModulePublic to packages.Package proposal: x/tools/go/packages: add modinfo.ModulePublic to packages.Package Dec 4, 2019
@rsc rsc added this to Incoming in Proposals (old) Dec 4, 2019
@rsc
Copy link
Contributor

rsc commented Apr 8, 2020

/cc @matloob @ianthehat

@rsc rsc moved this from Incoming to Active in Proposals (old) Apr 8, 2020
@matloob
Copy link
Contributor

matloob commented Apr 8, 2020

For now the x/tools/go/packages package is meant to be build-system agnostic. Since modules don't appear in all Go build systems, that means that if we're keeping to being build-system agnostic.

That might not be the right decision, but I think we should revisit that decision before deciding on this.

Issue #38234 is somewhat relevant to this by the way.

@rsc
Copy link
Contributor

rsc commented Apr 15, 2020

Being build-system agnostic shouldn't mean ignoring core Go features, though.
If you're using Bazel those fields would be empty.

@matloob matloob changed the title proposal: x/tools/go/packages: add modinfo.ModulePublic to packages.Package proposal: x/tools/go/packages: add module information to the Package struct Apr 22, 2020
@matloob
Copy link
Contributor

matloob commented Apr 22, 2020

I'm merging this issue with #38446.

The main differences are that we still need to discuss exactly what struct we're putting in Package (module public is unexported, and whichever struct we pick should probably be defined in x/mod), and that we'd need a Need bit for the field.

@rsc
Copy link
Contributor

rsc commented Apr 22, 2020

Here's the Module struct from go help list:

When listing modules, the -f flag still specifies a format template
applied to a Go struct, but now a Module struct:

    type Module struct {
        Path      string       // module path
        Version   string       // module version
        Versions  []string     // available module versions (with -versions)
        Replace   *Module      // replaced by this module
        Time      *time.Time   // time version was created
        Update    *Module      // available update, if any (with -u)
        Main      bool         // is this the main module?
        Indirect  bool         // is this module only an indirect dependency of main module?
        Dir       string       // directory holding files for this module, if any
        GoMod     string       // path to go.mod file used when loading this module, if any
        GoVersion string       // go version used in module
        Error     *ModuleError // error loading module
    }

    type ModuleError struct {
        Err string // the error itself
    }

Is that what we want in go/packages? Is there anything we don't want in go/packages?

@matloob
Copy link
Contributor

matloob commented Apr 24, 2020

That sounds reasonable to me? I don't see anything that we'd obviously not want. Though I'd like to hear what @mvdan and @dominikh think.

@dominikh
Copy link
Member

SGTM. Will go/packages acquire Versions and Update? If not, we may want to skip those.

@matloob
Copy link
Contributor

matloob commented Apr 27, 2020

Good point. Those could potentially be passed through as build flags, but that doesn't sound like something we'd want to encourage.

@rsc
Copy link
Contributor

rsc commented Apr 29, 2020

It does sound like we should leave out Versions and Update, making the struct we're talking about adding:

type Module struct {
    Path      string       // module path
    Version   string       // module version
    Replace   *Module      // replaced by this module
    Time      *time.Time   // time version was created
    Main      bool         // is this the main module?
    Indirect  bool         // is this module only an indirect dependency of main module?
    Dir       string       // directory holding files for this module, if any
    GoMod     string       // path to go.mod file used when loading this module, if any
    GoVersion string       // go version used in module
    Error     *ModuleError // error loading module
}

type ModuleError struct {
    Err string // the error itself
}

I'm not sure about ModuleError either, but we can see how it goes.

It sounds like people agree with the idea of adding the info. We can fine-tune the exact fields separately but the above is a start.

Based on the discussion above, this sounds like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Apr 29, 2020
@rsc
Copy link
Contributor

rsc commented May 6, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) May 6, 2020
@rsc rsc modified the milestones: Proposal, Unreleased May 6, 2020
@rsc rsc changed the title proposal: x/tools/go/packages: add module information to the Package struct x/tools/go/packages: add module information to the Package struct May 6, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label May 6, 2020
@gopherbot
Copy link

Change https://golang.org/cl/234219 mentions this issue: go/packages: add a Module field to the Package struct

@golang golang locked and limited conversation to collaborators May 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
No open projects
Development

No branches or pull requests

6 participants