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/gorelease: should not report removal of internal package as incompatible #37756

Closed
johanbrandhorst opened this issue Mar 9, 2020 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@johanbrandhorst
Copy link
Member

johanbrandhorst commented Mar 9, 2020

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

$ go version
go version go1.14 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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/REDACTED/.cache/go-build"
GOENV="/home/REDACTED/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY="REDACTED"
GONOSUMDB="REDACTED"
GOOS="linux"
GOPATH="/home/REDACTED/go"
GOPRIVATE="REDACTED"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build189907686=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I was using gorelease on a library which moved a package from internal/ to public API. gorelease was reporting that this was an incompatible change, since a package was removed, but I think this is wrong, since the package wasn't part of the public API beforehand.

Apologies if this is a misunderstanding and this does break compatibility.

For more information the error is here: https://app.circleci.com/jobs/github/grpc-ecosystem/grpc-gateway/5755, and the code change is here: grpc-ecosystem/grpc-gateway#1163.

What did you expect to see?

I expected gorelease not to report an error.

What did you see instead?

I saw an error.

@dmitshur
Copy link
Contributor

dmitshur commented Mar 9, 2020

/cc @jayconrod

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 9, 2020
@bcmills
Copy link
Contributor

bcmills commented Mar 9, 2020

What was the exact command you ran? (CircleCI links are generally not an appropriate way to report a bug against a tool other than CircleCI itself.)

Note that the tool owned by the Go team is golang.org/exp/cmd/gorelease (no r), not goreleaser.

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 9, 2020
@johanbrandhorst
Copy link
Member Author

What was the exact command you ran? (CircleCI links are generally not an appropriate way to report a bug against a tool other than CircleCI itself.)

Note that the tool owned by the Go team is golang.org/exp/cmd/gorelease (no r), not goreleaser.

Sorry for the confusion, I've updated the description. The command run is simply gorelease, on a branch. This is the translation of the Circle link to a terminal command:

$ gorelease
github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger
--------------------------------------------------------------------
Compatible changes:
- package added

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
---------------------------------------------------------
Compatible changes:
- package added

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
---------------------------------------------------------
Incompatible changes:
- package removed

Inferred base version: v1.14.1
Cannot suggest a release version.
Incompatible changes were detected.

@bcmills

This comment has been minimized.

@bcmills
Copy link
Contributor

bcmills commented Mar 9, 2020

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
---------------------------------------------------------
Compatible changes:
- package added

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
---------------------------------------------------------
Incompatible changes:
- package removed

Ok, that's a bug, but it doesn't appear to have anything to do with internal: for some reason, we're reporting the same package as both added and removed.

@bcmills bcmills removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 9, 2020
@johanbrandhorst
Copy link
Member Author

johanbrandhorst commented Mar 9, 2020

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
---------------------------------------------------------
Compatible changes:
- package added

github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
---------------------------------------------------------
Incompatible changes:
- package removed

Ok, that's a bug, but it doesn't appear to have anything to do with internal: for some reason, we're reporting the same package as both added and removed.

To add some context (if you didn't want to look at the pull request), the actual file changes are just a matter of moving a folder from github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/internal/genswagger to github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger. So the package genswagger was moved from being internal into being part of the public API, i.e. we added the package github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger and removed the package github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/internal/genswagger.

The error message doesn't make this very clear.

In case it makes any difference, the github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/internal folder was completely removed as a result of this action.

@gopherbot
Copy link

Change https://golang.org/cl/224118 mentions this issue: cmd/gorelease: sort packages returned by packages.Load.

@golang golang locked and limited conversation to collaborators Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants