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

Jump to bottom

x/tools/go/gcexportdata: improved API to replace gcimporter15 #15651

Closed
adonovan opened this issue May 11, 2016 · 9 comments
Closed

x/tools/go/gcexportdata: improved API to replace gcimporter15 #15651

adonovan opened this issue May 11, 2016 · 9 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@adonovan
Copy link
Member

Currently (tip prior to go 1.7) the go/importer package provides an implementation of the go/types.Importer interface that loads types.Packages and types.Objects from gc exportdata. However, the standard library provides only very restricted access to this functionality (importer.For). For example, there is no way to control how the importer finds .a files nor any way to access to the position information recorded in the export data, and the API provides no access to the functions that convert a types.Package to a []byte and back again. So, many clients need to use the golang.org/x/tools/go/gcimporter15 package, whose implementation is essentially identical, but whose API is not artificially restricted by the importer.For bottleneck. Maintaining the two packages is a nuisance and there is a risk of version skew.

I propose that the Go 1.8 standard library include a new package that is essentially equivalent to gcimporter15, and that the latter package be deprecated.

The API of the new standard package would look like this:

package gcexportdata // import "go/types/gcexportdata"

// Export a package.
func  Export(fset *go/token.FileSet, pkg *go/types.Package) []byte

// Import a package.
func  Import(fset *go/token.FileSet, imports map[string]*go/types.Package, data []byte, path string) (*go/types.Package, error)

// Find an archive file for the specified import, following GOPATH conventions.
func  FindArchive(path string, srcDir string) (filename string, id string)

// Read from r (an archive file) up until the start of the export data.
func  Advance(r *bufio.Reader) error

It might be better for Import and Export to take a single config struct as a parameter so that options can be added later without breaking compatibility, since the export data format will surely change as the compiler evolves.
@adonovan adonovan mentioned this issue May 11, 2016
Closed
@griesemer
Copy link
Contributor

griesemer commented May 11, 2016 •
edited

I think we should not add yet another package. We should integrate the needed functionality into go/importer, even if that means deprecating some of it's API. It's less confusing.

Also, I suggest that we take this slow - experience has shown that we've been very bad at nailing down the exact API; and once we had something, we had to change it soon for a new use case. Marking as 1.8Early.

@griesemer griesemer modified the milestones: Go1.8, Go1.8Early May 11, 2016
@adonovan
Copy link
Member Author

The idea behind a subpackage is to suggest that this is a more advanced, complete API, but adding the same API to the existing go/importer package sounds good to me. I don't think we need to deprecate the old API. It's restricted but works well enough for the simplest use-cases.

@rsc rsc added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 26, 2016
@rsc
Copy link
Contributor

rsc commented Sep 26, 2016

What is the status here? Is this important for Go 1.8?

@griesemer
Copy link
Contributor

See comment on #14738.

@alandonovan
Copy link
Contributor

Recently the issue of version skew has made me question whether importers belong in the standard library at all. Consider the moment at which a large Go customer switches from, say, Go 1.7 to Go 1.8. The compiler starts producing object files and export data in the latest format, but any other tools used by that customer will still be using the Go 1.7 importer and will instantly break. It is impossible to upgrade the whole ecosystem atomically; Google's tooling ecosystem involves many teams and complex release processes.

The solution to the version skew problem is to put the importer in a separate repo and to release it each time the file format evolves. That way, the tip version of the importer in the weeks before the Go 1.8 release is capable of reading Go 1.7 and Go 1.8 files. So long as the ecosystem is using this version of the library by the time of the release, everything just works. This is what we are already doing by developing the importer in x/tools.

The API improvements are still a good idea, though.

@griesemer
Copy link
Contributor

griesemer commented Sep 27, 2016 •
edited

We do need an importer in the std library otherwise go/types cannot function (unless we make it just an internal part of go/types, which is a possibility). The tip importer is backward-compatible and can read older formats. But @alandonovan is also correct that an older importer won't be able to read a newer format. This is an issue for infrastructure that cannot move to the latest release yet must process (for some reason or another) object files and export data built with the latest compiler (as is the case for source code analysis tools).

@griesemer
Copy link
Contributor

griesemer commented Oct 6, 2016 •
edited

@alandonovan Based on our conversations, can we close this now? We are still planning to fix the go/importer implementation and API (issues #16088, #13847, and #11415).

@alandonovan
Copy link
Contributor

alandonovan commented Oct 6, 2016 •
edited

I no longer think we should change the standard library, but we should provide in x/tools a package that is functionally equivalent to gcimporter15 but with the slightly nicer API described above. All this requires is: adding the new thin wrapper package, x/tools/go/gcexportdata, deprecating x/tools/go/gcimporter15, and, eventually, deleting gcimporter15 by moving it under x/tools/go/gcexportdata/internal.

@alandonovan alandonovan changed the title go/types/gcexportdata: new package for loading types.Objects from gc export data x/tools/go/gcexportdata: improved API to replace gcimporter15 Oct 6, 2016
@griesemer griesemer modified the milestones: Unreleased, Go1.8Early Oct 6, 2016
@griesemer griesemer removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 6, 2016
@gopherbot
Copy link

CL https://golang.org/cl/30612 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@rsc @adonovan @alandonovan @griesemer @gopherbot