go/importer: implement support for importer.Lookup

[griesemer]

Missing functionality. Required for clients that store packages outside the implied file system structure.

[griesemer]

Started.

[griesemer]

We may have to leave this one for 1.7. The existing API based on the importer.Lookup type for flexibility is unfortunately inadequate, and at the same time unimplemented (a non-nil Lookup function was not supported). That is, it's not possible to have any users yet.

This opens the door to a backward-incompatible change. At the same time we don't want to design in a vacuum (as we did on short-notice before 1.5 in this case). Probably better to leave this for early 1.7 when we also get the new import data which should clear things up.

[griesemer]

We have x/tools/gcimporter15 as a stop gap for now.

Due to the additional srdDir argument required for lookup because of vendoring, this API may have to change unfortunately.

[creachadair]

We had to work around the lack of support for custom lookup functions for http://kythe.io/. I did this by the gross but expedient hack of depending on the internal package, but that prevents building with the go tool. Obviously we'd prefer to get rid of the hack.

What can I offer by way of assistance to get this working?

[griesemer]

1. The API - as it was designed for 1.5 - didn't take into account vendoring (at that point considered an experiment introduced late). As a consequence, the current API (go/importer.Lookup function) may have to change. Since it cannot (Go 1 backward-compatibility promise) we need to come up with a new API. We didn't hurry this for 1.6 because we got to it too late and didn't want to prototype yet another possibly incomplete API.

2. The export data format is going to change for 1.7 (the new format is already implemented but for inlined exported functions). It shouldn't affect the API, but it might.

In terms of help: It would be good to have examples of concrete use cases, ideally with suggested minimal API. This would help clarify the final solution.

For what it's worth, we have x/tools/go/gcimporter15 which provides (I hope) all the individual pieces to import from custom locations and it is based on the 1.5/1.6 std repo go/types. Does that help?

[creachadair]

Thank you for the information about gcimporter15. If that works as I think, that might even let me get rid of the hacky dependency I took.

For Kythe, our goal is to be able to capture the files involved in building a Go package and to (later) re-run the type checker from those. An ideal API for our use case would be something like a function that takes a compiled package (say, a .a or .gcgox file) and returns whatever the go/types package needs to use it for type resolution—presumably a package name and some other internal state. We don't care at all about the internal format of this information as long as there's a way to get go/types to use it.

Having the API move around is fine, as long as it's a public API.

[griesemer]

This is not going to happen for 1.7 since it does require additional API work. We have x/tools/go/gcimporter15 as a stop-gap, it is compatible with the latest import/export work.

[rsc]

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

[griesemer]

Also indirectly related to #14738.

[creachadair]

From our perspective as outside consumers, if gcimporter15 (or a similar workaround) keeps working we can live with that. Our use case is still the same as described above.

[bronze1man]

I just found a work around to solve this problem.(I write them here in case I forgot them again)

• copy /usr/local/go/src/go/internal/gcimporter to your own GOPATH. (like src/github.com/xxx/gcimporter18)
• Delete some code in Import function to pass in filename and id directly.(like func Import(packages map[string]*types.Package, id, filename string) (pkg *types.Package, err error))
• Delete // import "go/internal/gcimporter"
• call them in another package gcimporter18.Import(map[string]*types.Package{},"xxx/myPackage","pkg/android_arm/xxx/myPackage.a")

The side effect is that one copyed package only support one version of golang .
You need copy them from go1.7 and go1.8 to support both of them.

[griesemer]

Note to self: See comments in @13222 when addressing this issue.

[gopherbot]

Change https://golang.org/cl/74354 mentions this issue: go/importer: support lookup in importer.For

[rsc]

@alandonovan and @griesemer, I need importer.For to accept a non-nil lookup func in order to pass cmd/vet proper details about built packages from cmd/go. I did a basic implementation at C: 74354 that works well enough for my use in CL 74355. Does that seem like a reasonable approach?

In #13222 Alan wrote:

Either it should return the name of a .a file for a given package, or it should return a reader for the specific exportdata section within the file.

I don't understand this either-or. If returning the name of a .a file is OK, then why does the alternative have to be opening the .a file and positioning the reader into the middle of the .a file? I made lookup in my CL position the reader at the beginning of the .a file. gcimporter already knows how to find the export data inside a .a file, so I don't see any reason to duplicate that logic. (If needed gcimporter could also sniff the first few bytes to see if the input is a .a file or raw binary export data.)

One thing that's kind of missing from the Importer API is exposing the mapping step from import path in source code to canonical import path identifier, such as for example turning "foo" into "x/vendor/foo" when compiling "x" (and "x/vendor/foo" exists). I say kind of because I was able to make it work by wrapping the low-level gcimporter with a lookup for opening the expanded imports, and then I have my own Import method that handles the translation. I think this is a bit of an existence proof that the current Lookup API is good enough, even if it's not what we might do if starting over.

[griesemer]

Your CL 74354 seems reasonable given that you do the translation to canonical import path identifier externally. This is basically where I stopped in the implementation.

Regarding Alan's comment: He might be thinking about export data that is stored in alternative forms, e.g. in a data base of sorts (e.g., which may be created nightly, by a crawl through a large repo, such as our Google-internal ones) where you may not want to the rest of the .a file around it (or at least before it).

[alandonovan]

He might be thinking about export data that is stored in alternative forms, e.g. in a data base of sorts (e.g., which may be created nightly, by a crawl through a large repo, such as our Google-internal ones) where you may not want to the rest of the .a file around it (or at least before it).

Yes, that's exactly what I had in mind.

[rsc]

Right, I think it's fine to say that lookup can return an io.ReadCloser positioned at either the archive or the export data (within the archive or for something else). It just doesn't seem worth forcing all implementations to jump into the middle of the archive if that's the form they have easy access to.