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

proposal: x/tools/go/gcexportdata: drop experimental bundle API #69573

Open
adonovan opened this issue Sep 21, 2024 · 8 comments
Open

proposal: x/tools/go/gcexportdata: drop experimental bundle API #69573

adonovan opened this issue Sep 21, 2024 · 8 comments

Comments

@adonovan
Copy link
Member

adonovan commented Sep 21, 2024

The gcexportdata package has two functions, WriteBundle and ReadBundle, that have always been marked as experimental and are not used within x/tools, nor, to my knowledge, anywhere else. The only match in GitHub was a single call from apidiff in the obsolete golang/exp repo (and forks thereof), which I have since updated.

package gcexportdata

func ReadBundle(in io.Reader, fset *token.FileSet, imports map[string]*types.Package) ([]*types.Package, error)
func WriteBundle(out io.Writer, fset *token.FileSet, pkgs []*types.Package) error

We propose to drop bundle functionality by making both functions return an error unconditionally.

Related:

@gopherbot gopherbot added this to the Proposal milestone Sep 21, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/614815 mentions this issue: go/gcexportdata: drop experimental Bundle support

@ianlancetaylor
Copy link
Member

CC @jba for apidiff

@jba
Copy link
Contributor

jba commented Sep 23, 2024

Why do you describe golang.org/x/exp as obsolete? exp means "experimental,", not "expired."

apidiff has a few users, notably the Go client libraries for GCP. I don't think we should break it.

I'd be happy to rewrite Read/WriteBundle, if you could explain how to get the same effect.

@adonovan
Copy link
Member Author

Why do you describe golang.org/x/exp as obsolete? exp means "experimental,", not "expired."

I misremembered. In any case, it's easy enough for me to fix this one instance. My point was more that no-one is really using this feature.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Oct 16, 2024
@rsc
Copy link
Contributor

rsc commented Dec 4, 2024

Given that it is experimental and we don't think anyone is using it, this seems fine.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Dec 4, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/634600 mentions this issue: cmd/apidiff: stop using gcexportdata.{Read,Write}Bundle

gopherbot pushed a commit to golang/exp that referenced this issue Dec 10, 2024
This CL changes apidiff to stop using the bundle read/write
operations, which were always experimental and are in the
process of being deprecated/dropped (#69573).
Instead, we use the ordinary gcexportdata.Write operation
to save each package into a section of a zip file named
after the package, in topological order.

(This could perhaps be simplified further by simply
retaining the export data files originally produced
by the compiler without even parsing them;
see go/packages.NeedExportFile.)

Also:
- plumb token.FileSet down from main, addressing a TODO.
- remove unnecessary packages.LoadMode bits.

Updates golang/go#69573

Change-Id: I6347097b480853d7bf21047595282f2123ee50b1
Reviewed-on: https://go-review.googlesource.com/c/exp/+/634600
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
@aclements
Copy link
Member

Based on the discussion above, this proposal seems like a likely accept.

The proposal is to make the unused and experimental functions WriteBundle and ReadBundle in golang.org/x/tools/go/gcexportdata always return an error.

@aclements aclements moved this from Active to Likely Accept in Proposals Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Likely Accept
Development

No branches or pull requests

6 participants