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

Make proto-tools script creates unbuildable proto files #7332

Closed
4 tasks
AdityaSripal opened this issue Sep 16, 2020 · 10 comments · Fixed by #7893
Closed
4 tasks

Make proto-tools script creates unbuildable proto files #7332

AdityaSripal opened this issue Sep 16, 2020 · 10 comments · Fixed by #7893
Assignees
Labels
T:Bug tooling dev tooling within the sdk

Comments

@AdityaSripal
Copy link
Member

Summary of Bug

I reinstalled my proto-tools with the new Makefile script and while everything downloaded just fine, and running the proto commands works fine. The final result is unbuildable.

To see an example of the pb.go files this build generated, see this commit: c2b7b56

Trying to build fails on this commit as you can see in the github checks on that commit.

I had to checkout to a version of master before #7294 and reinstall my proto tooling there to get my branch working again.

Version

Commit: 56e3bc1

Operating System: Darwin 19.4.0

Steps to Reproduce

# on master
rm proto-tools-stamp
make proto-tools
make proto-gen
make build
# Result
➜  cosmos-sdk git:(master) ✗ make build
go install -mod=readonly ./...
# github.com/keybase/go-keychain
cgo-gcc-prolog:203:11: warning: 'SecTrustedApplicationCreateFromPath' is deprecated: first deprecated in macOS 10.15 - No longer supported [-Wdeprecated-declarations]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Security.framework/Headers/SecTrustedApplication.h:59:10: note: 'SecTrustedApplicationCreateFromPath' has been explicitly marked deprecated here
# github.com/cosmos/cosmos-sdk/types
types/coin.pb.go:300:8: invalid argument m.Amount (type Int) for len
types/coin.pb.go:301:11: invalid argument m.Amount (type Int) for len
types/coin.pb.go:302:7: second argument to copy should be slice or string; have struct { i *big.Int }
types/coin.pb.go:303:43: invalid argument m.Amount (type Int) for len
types/coin.pb.go:337:8: invalid argument m.Amount (type Dec) for len
types/coin.pb.go:338:11: invalid argument m.Amount (type Dec) for len
types/coin.pb.go:339:7: second argument to copy should be slice or string; have struct { i *big.Int }
types/coin.pb.go:340:43: invalid argument m.Amount (type Dec) for len
types/coin.pb.go:374:8: invalid argument m.Int (type Int) for len
types/coin.pb.go:375:11: invalid argument m.Int (type Int) for len
types/coin.pb.go:375:11: too many errors
make: *** [build] Error 2

I also noticed that in either case, make proto-format was not working for me: #7320 (comment).
proto-tools downloads clang-format version 10.0.1 for me. I don't know if that is the correct version or not.

It's possible something I did in my setup was wrong, but I removed all the executables proto-tools should download and reran the script and still got the same error, so not sure what's happening.

cc: @alessio @alexanderbez


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alessio
Copy link
Contributor

alessio commented Sep 16, 2020

Hold on, your build's job proto-check-breaking-docker leverages a docker-ized version of buf. Which I didn't touch at all.
And from the build log shown here, there's clearly a mismatch between such adocker-ized version of buf and the local version of buf we've been using for a long time (I didn't touch that either).

@aaronc @clevinson any advice here? Can anyone reproduce this issue somehow?

@tac0turtle
Copy link
Member

Hold on, your build's job proto-check-breaking-docker leverages a docker-ized version of buf. Which I didn't touch at all.
And from the build log shown here, there's clearly a mismatch between such adocker-ized version of buf and the local version of buf we've been using for a long time (I didn't touch that either).

The docker command stays up to date with the latest buf changes.

To mitigate this someone should create a docker file that builds proto stubs. It seems there are countless issues with the current design because of the use of so many tools. We did this on tendermint to mitigate this issue when the work on proto first started. It has saved the team time in producing the proto files

@tac0turtle
Copy link
Member

buf now has support for protoc buf protoc. It may be beenficial to migrate to it from normal protoc as they both do the same thing

@AdityaSripal
Copy link
Member Author

I had this issue locally as well, so I don't think docker is the problem?

@AdityaSripal
Copy link
Member Author

Has anyone else been able to reproduce on a Mac?

@colin-axner
Copy link
Contributor

I can reproduce this on Linux, see comment

@fedekunze
Copy link
Collaborator

fedekunze commented Oct 1, 2020

I'm experiencing this issue too on Mac... See #7425 (review)

@fedekunze fedekunze added the tooling dev tooling within the sdk label Oct 1, 2020
@fedekunze
Copy link
Collaborator

adding this to 0.40 milestone @clevinson @amaurymartiny

@fedekunze fedekunze added this to the v0.40 [Stargate] milestone Oct 1, 2020
@amaury1093
Copy link
Contributor

amaury1093 commented Oct 1, 2020

I think we all agree that we should 1/ remove make proto-tools and 2/ run make proto-gen using Docker: #7410 (comment)

I'm assigning this to @marbar3778, lmk if you prefer someone from the Regen team to take care of it.

@colin-axner
Copy link
Contributor

colin-axner commented Oct 29, 2020

I'm still facing this issue. How can I fix it?

Edit: I checked out the commit 5e16c21 and reinstalled using the old proto script as Aditya described in the top post of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Bug tooling dev tooling within the sdk
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants