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/gcexportdata: support export using indexed format #28260

Closed
alandonovan opened this issue Oct 17, 2018 · 12 comments
Closed

x/tools/go/gcexportdata: support export using indexed format #28260

alandonovan opened this issue Oct 17, 2018 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@alandonovan
Copy link
Contributor

alandonovan commented Oct 17, 2018

We need to port the reader of indexed import data ($GOROOT/src/cmd/compile/internal/gc/iexport.go) to produce go/types data structures, analogous to what we do for the $$B format, which is deprecated.

@stamblerre

@gopherbot gopherbot added this to the Unreleased milestone Oct 17, 2018
@alandonovan alandonovan changed the title x/tools/go/gcexportdata: support export reading from indexed files x/tools/go/gcexportdata: support reading from indexed files Oct 17, 2018
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 19, 2018
@stamblerre
Copy link
Contributor

It seems like gcexportdata.Read is already reading indexed export data as of golang/tools@9a70f1f.

@alandonovan or @griesemer: is there anything else that needs to be done here?

@alandonovan alandonovan changed the title x/tools/go/gcexportdata: support reading from indexed files x/tools/go/gcexportdata: support export using indexed format Nov 20, 2018
@alandonovan
Copy link
Contributor Author

I corrected the issue title to mean "write", not "read".

@stamblerre
Copy link
Contributor

Thanks - I'll start working on this now.

@stamblerre stamblerre self-assigned this Nov 28, 2018
@gopherbot
Copy link

Change https://golang.org/cl/156901 mentions this issue: go/internal/gcimporter: write export data for go/types

gopherbot pushed a commit to golang/tools that referenced this issue Jan 30, 2019
Add an iexport.go (and corresponding iexport_test.go) file, which is an
adapted version of $GOROOT/src/cmd/compile/internal/gc/iexport.go. This
code writes exportdata for a *go/types.Package.

A majority of this code is directly copied from iexport.go, with a
change of types, while some of it had to be modified slightly.

Updates golang/go#28260

Change-Id: Ic7e8e99f0c6b886839280b410afffb037da8a79b
Reviewed-on: https://go-review.googlesource.com/c/156901
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@griesemer
Copy link
Contributor

@stamblerre Is this finished, i.e., can we close this issue?

@stamblerre
Copy link
Contributor

I think the final step here would be to have the gcexportdata.Write function actually write the indexed export data format (https://github.com/golang/tools/blob/master/go/gcexportdata/gcexportdata.go#L102). I will send out a CL to that effect ASAP.

@gopherbot
Copy link

Change https://golang.org/cl/198742 mentions this issue: go/gcexportdata: use IExportData when writing gcexportdata

gopherbot pushed a commit to golang/tools that referenced this issue Oct 3, 2019
Updates golang/go#28260

Change-Id: Id0ef98e1599fbef4dd67ba23c20ddb8bc4d33228
Reviewed-on: https://go-review.googlesource.com/c/tools/+/198742
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Oct 4, 2019
Updates golang/go#28260

Change-Id: Id0ef98e1599fbef4dd67ba23c20ddb8bc4d33228
Reviewed-on: https://go-review.googlesource.com/c/tools/+/198742
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@jba
Copy link
Contributor

jba commented Oct 14, 2019

https://golang.org/cl/198742 introduced a compatible change, but a breaking one for some programs. It used to be that Read returned the same package that was written if path was garbage. Now it returns something else. For example, this program (which the playground won't currently run) writes the export data for "cloud.google.com/go/bigquery", then reads it back in with path set to nonsense (the filename). It prints "bufio".

I think Read should return an error if it can't find path in the export data, or at least it should return the equivalent of imports[path]. That is also a compatible breaking change, but one that would have caught my bug much earlier.

@stamblerre
Copy link
Contributor

@jba: I just mailed CL 201097, with which you should be able to import the package with the filename again. Does it fix the problem for you?

@gopherbot
Copy link

Change https://golang.org/cl/201097 mentions this issue: go/internal/gcimporter: use empty string for the top-level package path

@jba
Copy link
Contributor

jba commented Oct 22, 2019

I worked around it a different way. (I saved the path with the export data.)

It sounds like you're keeping the previous behavior, where Read returns the written package for certain values of path. That is useful behavior, but it doesn't match the spec of Read.

Should we update Read's doc to describe it? Maybe "If path is empty, Read returns the same package that was passed to Write"? In which case I still think you should return an error if path is neither empty nor a path found in the file.

gopherbot pushed a commit to golang/tools that referenced this issue Oct 22, 2019
I made a mistake with the initial port of iexport.go in that I left the
original package path of the top-level package in the export data.
The package path for the top-level package should have been empty so
that it can be changed when the package is loaded.

Updates golang/go#28260

Change-Id: I781e63317a54eaf59385f25d18609e73ff97d572
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201097
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@stamblerre
Copy link
Contributor

Ended up adding documentation to IExportData (https://go-review.googlesource.com/c/tools/+/201097/6/go/internal/gcimporter/iexport.go), since it's really a behavior of the export data writer. The data that gets read should always have the import path that is provided by the caller, which is the way it worked in the past. It was my misunderstanding that changed the behavior, though it was never documented that the top-level package's path should be dropped. I think adding that documentation should be sufficient.

@golang golang locked and limited conversation to collaborators Oct 21, 2020
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. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants