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/exp/cmd/apidiff: incorrectly reports breaking change on numeric const using scientific e-notation #44796

Closed
noahdietz opened this issue Mar 4, 2021 · 8 comments

Comments

@noahdietz
Copy link

noahdietz commented Mar 4, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16 darwin/amd64

Also happens on go version go1.16 linux/amd64.

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOOS="darwin"

Also happens on linux/amd64.

What did you do?

go install golang.org/x/exp/cmd/apidiff@latest
git clone https://github.com/googleapis/google-cloud-go gocloud
cd gocloud/pubsub
apidiff -w pkg.master cloud.google.com/go/pubsub
apidiff -incompatible pkg.master cloud.google.com/go/pubsub 

What did you expect to see?

Nothing, no breaking changes.

What did you see instead?

- MaxPublishRequestBytes: value changed from 0.000582077 to 10000000

The LoC in question is here.

In short, it reads:

const (
  MaxPublishRequestBytes = 1e7
)

We are trying to (re)enable apidiff in cloud.google.com/go CI, but can't enable it until this is fixed. We can always workaround this specific error though, I guess.

@gopherbot gopherbot added this to the Unreleased milestone Mar 4, 2021
@ianlancetaylor
Copy link
Contributor

CC @jba

@noahdietz
Copy link
Author

I think it might be on the export side of things - because the "new" side of the diff outputs the proper value, while the "old" side (I think loaded from the exported data) is reported as the mangled value. It could be that the constant value is incorrectly written at export, or incorrectly read at compat check. Since apidiff just invokes gcexportdata APIs, maybe there is an issue in there?

@noahdietz
Copy link
Author

noahdietz commented Mar 7, 2021

When I use a local install of golang.org/x/exp/cmd/apidiff @ HEAD with no changes, there is no diff - it works properly. But when I install again via go install golang.org/x/exp/cmd/apidiff@latest, rewrite the base export data, and compare, it fails with the error above.

This is also with a local install of tools/go/gcexportdata @ HEAD wired up with replace in exp. I think there are some dependency updates I picked up that fixed the issue?

@noahdietz
Copy link
Author

Updating the golang.org/x/tools dep to v0.1.0 still does not work. Going to check individual commits to golang.org/x/tools since the v0.1.0 release that also changed the gcexportdata pkg to see if I can find the commit that fixes it. v0.1.0 was cut on 1/19/21, so there are only a handful of commits to gcexportdata to check...

@noahdietz
Copy link
Author

noahdietz commented Mar 7, 2021

I checked out golang/tools@a1db63c locally (still wired with replace) and then apidiff worked properly. I have no idea how that would fix things.

The pseudo version for that x/tools commit is v0.1.1-0.20210217070731-a1db63cc08e0.

Worth noting that some other deps in exp were updated as a result:

	golang.org/x/mod v0.4.1
	golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c
	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1

@jba
Copy link
Contributor

jba commented Mar 8, 2021

It was fixed by commit b79f76f, CL b79f76f
https://go-review.googlesource.com/c/tools/+/292350. Anything past that should fix it.

@jba jba closed this as completed Mar 8, 2021
@noahdietz
Copy link
Author

Ok, good to know the exact fix, but that commit hasn't been released. Is it possible to cut a release of the root module in tools? Then we can update the version used in exp to capture the fix and go install golang.org/x/exp/cmd/apidiff@latest will work.

@noahdietz
Copy link
Author

@ianthehat Hi would it be possible to cut a release of the x/tools root module? The commit jba mentioned that has a fix for apidiff that would unblock its use in cloud.google.com/go CI. If there is some regular release schedule, we can wait on that too! Thanks.

@golang golang locked and limited conversation to collaborators Mar 8, 2022
@rsc rsc unassigned jba Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants