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.

Sign up for GitHub

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

Jump to bottom

x/tools/packages: Populate Module field for package loads using Gopackagesdriver #62601

Open
rastenis opened this issue Sep 12, 2023 · 5 comments
Open

x/tools/packages: Populate Module field for package loads using Gopackagesdriver #62601

rastenis opened this issue Sep 12, 2023 · 5 comments
Labels
Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@rastenis
Copy link

rastenis commented Sep 12, 2023 •
edited

packages.Load uses either the goListDriver or an external driver (Gopackagesdriver) to retrieve information about packages. However, the goListDriver response returns data that is missing in the Gopackagesdriver response.

Currently, the Module field is not parsed from the external driver response, implemented as an UnmarshalJSON override in packages.go:

	*p = Package{
                  ID:              flat.ID,
                  Name:            flat.Name,
                  PkgPath:         flat.PkgPath,
                  Errors:          flat.Errors,
                  GoFiles:         flat.GoFiles,
                  CompiledGoFiles: flat.CompiledGoFiles,
                  OtherFiles:      flat.OtherFiles,
                  EmbedFiles:      flat.EmbedFiles,
                  EmbedPatterns:   flat.EmbedPatterns,
                  ExportFile:      flat.ExportFile,
                  // Module is not parsed
	}

However, it is parsed from the goListDriver response, in golist.go:

	pkg := &Package{
	        Name:            p.Name,
	        ID:              p.ImportPath,
	        GoFiles:         absJoin(p.Dir, p.GoFiles, p.CgoFiles),
	        CompiledGoFiles: absJoin(p.Dir, p.CompiledGoFiles),
	        OtherFiles:      absJoin(p.Dir, otherFiles(p)...),
	        EmbedFiles:      absJoin(p.Dir, p.EmbedFiles),
	        EmbedPatterns:   absJoin(p.Dir, p.EmbedPatterns),
	        IgnoredFiles:    absJoin(p.Dir, p.IgnoredGoFiles, p.IgnoredOtherFiles),
	        forTest:         p.ForTest,
	        depsErrors:      p.DepsErrors,
	        Module:          p.Module,  // Module is parsed
	}

The proposal is to add the Module field to the UnmarshalJSON override in packages.go, so that packages.Load returns Module data when using either driver. This also involves adding the field to the flatPackage struct.

The fields required in the Module field for our usecase is as follows:

  1. Module.Version
  2. Module.Dir
  3. Module.Path

Let me know if this sounds good, or if there are any alternatives to consider or suggestions about this approach.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2023
@gopherbot gopherbot added this to the Unreleased milestone Sep 12, 2023
@timothy-king
Copy link
Contributor

From MarshalJSON's doc:

// This method exists to enable support for additional build systems.  It is
// not intended for use by clients of the API and we may change the format.

Is there a build system or tool that would benefit from populating Module information in Package.MarshalJSON? It is kinda clear we could be more consistent here, but I am wondering if there is another motivation. (It is not obvious to me bazel could take advantage of this for example.)

Also can you be a bit more clear about what you are proposing would be serialized for Module? As in which fields? We do not serialize all of Package for example. Does this include Replace? A prototype/CL could also clarify this.

@JamyDev
Copy link

JamyDev commented Sep 22, 2023 •
edited

We've been working on creating stored indices using sourcegraph/scip-go. It encodes version information from the module data, which is available when using the standard Go import system, but with Go Packages Driver we're not able to populate version information.

Info needed, mostly Module.Version I think, @rastenis correct me please.

It uses packages.Load to get the packages in the project.

@timothy-king
Copy link
Contributor

Info needed, mostly Module.Version I think, @rastenis correct me please.

For a proposal to go through the proposal process https://github.com/golang/proposal#the-proposal-process, I would recommend updating the top comment to have a complete list of the fields from Modules you think should be included. I am guessing Module.Path and ModuleError are other requirements?

I presume when serializing a packages.Package, there will be a copy of the serialized *Packages.Module when available. This will be somewhat redundant as there can be many packages per module.

@rastenis
Copy link
Author

I have added the required fields above. For our direct use case, Module.Version and Module.Dir are required, however I also added Module.Path since it is referenced multiple times in the project we are modifying for our use cases, so I included it as well to be safe.

@adonovan
Copy link
Member

This proposal seems reasonable. It ought to be possible to proxy for a go/packages driver to produce anything 'go list' can produce--even if it doesn't correspond to an actual build system such as bazel, pants, buck, etc--so that it can be used to proxy a remote project, for example. So I would be in favor of adding a Module field, whose type is *packages.Module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Projects
Proposals
Status: Incoming
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@timothy-king @JamyDev @adonovan @gopherbot @rastenis