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

go/importer: implement support for importer.Lookup #13847

Closed
griesemer opened this issue Jan 7, 2016 · 17 comments
Closed

go/importer: implement support for importer.Lookup #13847

griesemer opened this issue Jan 7, 2016 · 17 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@griesemer
Copy link
Contributor

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

@griesemer griesemer self-assigned this Jan 7, 2016
@griesemer griesemer added this to the Go1.6Maybe milestone Jan 7, 2016
@griesemer
Copy link
Contributor Author

Started.

@griesemer
Copy link
Contributor Author

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 griesemer modified the milestones: Go1.7, Go1.6Maybe Jan 21, 2016
@griesemer
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

  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
Copy link

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
Copy link
Contributor Author

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.

@griesemer griesemer modified the milestones: Go1.8Early, Go1.7 May 9, 2016
@rsc
Copy link
Contributor

rsc commented Sep 26, 2016

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

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 26, 2016
@griesemer
Copy link
Contributor Author

Also indirectly related to #14738.

@creachadair
Copy link

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
Copy link
Contributor

bronze1man commented Jul 11, 2017

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
Copy link
Contributor Author

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

@griesemer griesemer self-assigned this Aug 16, 2017
@griesemer griesemer modified the milestones: Go1.10, Unplanned Aug 16, 2017
@gopherbot
Copy link

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

@rsc
Copy link
Contributor

rsc commented Oct 30, 2017

@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
Copy link
Contributor Author

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
Copy link
Contributor

alandonovan commented Oct 30, 2017

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
Copy link
Contributor

rsc commented Oct 30, 2017

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.

@golang golang locked and limited conversation to collaborators Oct 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants