Fix installation of gocosmos #7410

amaury1093 · 2020-09-29T13:46:25Z

Description

the command go get github.com/regen-network/cosmos-proto/protoc-gen-gocosmos needs to run in the root folder, to be aware of the replace we added in go.mod.

I then ran make proto-gen with the following versions:

cosmos-sdk/contrib/devtools/proto-tools-installer.sh

Lines 10 to 11 in ddaa3c5

    
           PROTOC_VERSION=3.13.0 
        
           PROTOC_GRPC_GATEWAY_VERSION=1.14.7

➜  cosmos-sdk git:(am-proto-gen) protoc --version
libprotoc 3.13.0
➜  cosmos-sdk git:(am-proto-gen) protoc-gen-grpc-gateway --version
Version 1.14.7, commit 37837bc5882204897dfb5b17ee231fc645c9102d, built at 2020-08-12T08:49:14Z

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

amaury1093 · 2020-09-29T13:47:10Z

Could someone with the correct version also run make proto-gen, and make sure there's no diff? 🙏 (except maybe staking.pb.go)

types/abci.pb.go

alexanderbez · 2020-09-29T13:59:59Z

Confirmed I have no diff!

codecov · 2020-09-29T17:12:30Z

Codecov Report

Merging #7410 into master will decrease coverage by 0.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7410      +/-   ##
==========================================
- Coverage   55.07%   54.81%   -0.26%     
==========================================
  Files         588      587       -1     
  Lines       36760    36512     -248     
==========================================
- Hits        20245    20014     -231     
+ Misses      14417    14406      -11     
+ Partials     2098     2092       -6

contrib/devtools/proto-tools-installer.sh

alessio · 2020-09-30T17:12:52Z

@robert-zaremba dixit:

This doesn't solve anything. It just prints the message, and gogo/protobuf is already checked in go.mod, so this is noop.

Those lines remove pushd/popd calls, which are seemingly unnecessary. I agree that the comment should be removed though.

…o am-proto-gen

robert-zaremba · 2020-09-30T17:16:13Z

So, the whole command (f_install_protoc_gen_gocosmos) doesn't make sense because it doesn't fix any dependency

github.com/regen-network/cosmos-proto/protoc-gen-gocosmos is a binary dependency for protoc. github.com/regen-network/cosmos-proto/ version is already fixed in go.mod.
we should avoid running go get at all kind of setup scritps. We should use go get only when we add a new dependency or when we want to upgrade one.
Go modules were designed specifically for this use-cases - to encapsulate all dependencies on go packages.

Go modules can be used to run binaries. Just use go run package/to/binary and we will run a binary (eg go run github.com/regen-network/cosmos-proto/protoc-gen-gocosmos).

Howe however here we have more delicate problem. Instead of us running the binary, it's protoc, which doesn't understand Go modules.

The ideal solution would be to:

force go module to install binary in a local folder. Maybe with go vendor? Or maybe we can tweak GOBIN to make sure we will not collide with system wide installations.
Then we can tweak PATH before running protoc
```
 PATH="./bin:$PATH" protoc ...
```

alessio · 2020-09-30T17:23:48Z

Maybe with go vendor?

No thanks. Using go vendor just to go install stuff does not sound super clever.

Or maybe we can tweak GOBIN to make sure we will not collide with system wide installations.

No thanks. GOBIN breaks cross-compilation and should not be used.

The root of all evils here still lies with the absence of a global go install command that does not touch modules go.mod files.

force go module to install binary in a local folder.

This is perhaps the best idea.

I'm still in favour of installing and using all these tools from some sort of devtools container in order to guaranteee reproducibility and ease of installation. @marbar3778 any ideas?

robert-zaremba · 2020-09-30T17:30:41Z

force go module to install binary in a local folder.

This is perhaps the best idea.

I'm not aware about any other way to do it without setting GOBIN.

robert-zaremba · 2020-09-30T17:31:01Z

BTW: there are more people who has same problem: golang/go#32504

Ultimately, what I would love to see is that if my coworker git-cloned my project that depended on other main packages such as github.com/golang/protobuf/protoc-gen-go, they would be able to get the precise version that I intended to use protoc-gen-go with.

robert-zaremba · 2020-09-30T17:34:16Z

So, if we don't have any other solution I'm also for removing this script at all and encapsulate all protobuf dependencies in a docker container.

(same @amaurymartiny , @aaronc , @anilcse , @ethanfrey ....)

robert-zaremba · 2020-09-30T17:34:47Z

(sorry, I missed the button ;) )

alessio · 2020-09-30T17:40:26Z

I'm not aware about any other way to do it without setting GOBIN.

GOBIN is unnecessary because the binary default installation directory is $GOPATH/bin. If you set GOBIN explicitly though, things change. I've commented on this subject multiple times, we've removed the mention of GOBIN from docs and instructions long time ago.

BTW: there are more people who has same problem: golang/go#32504

Yes. It's known limitation of go modules.

So, if we don't have any other solution I'm also for removing this script at all and encapsulate all protobuf dependencies in a docker container.

Yep - @marbar3778 was working on that IIRC?

robert-zaremba · 2020-09-30T17:43:34Z

GOBIN is unnecessary because the binary default installation directory is $GOPATH/bin. If you set GOBIN explicitly though, things change.

Exactly, this is what I meant: to pin GOBIN to a local directory, eg: GOBIN=./bin

ethanfrey · 2020-09-30T18:15:22Z

So, if we don't have any other solution I'm also for removing this script at all and encapsulate all protobuf dependencies in a docker container.

💯 for this

The Dockerfile can/should be in the cosmos-sdk/devtools directory but trying to auto-gen code without locked versions between dev environments is chaos.

alexanderbez · 2020-09-30T21:13:50Z

dkr++

fedekunze

Works for me. Only diff is staking

amaury1093 · 2020-10-01T10:32:23Z

Are we okay to merge this temporary fix while waiting for the Docker solution (tracked here: #7332) ?

cc @robert-zaremba @alessio

contrib/devtools/proto-tools-installer.sh

talked offline

Run make proto-gen with correct version

eb97e11

amaury1093 requested a review from tac0turtle September 29, 2020 13:46

amaury1093 requested review from aaronc, alessio and alexanderbez as code owners September 29, 2020 13:46

amaury1093 requested a review from anilcse September 29, 2020 13:46

amaury1093 marked this pull request as draft September 29, 2020 13:49

tac0turtle reviewed Sep 29, 2020

View reviewed changes

types/abci.pb.go Outdated Show resolved Hide resolved

amaury1093 added 3 commits September 29, 2020 19:02

Run with correct gocosmos

a8f2ca8

Install gocosmos with go.mod

ea346b3

Merge branch 'master' into am-proto-gen

9f72ab7

amaury1093 changed the title ~~Run make proto-gen with correct version~~ Fix installation of gocosmos Sep 29, 2020

amaury1093 marked this pull request as ready for review September 29, 2020 17:10

anilcse reviewed Sep 29, 2020

View reviewed changes

contrib/devtools/proto-tools-installer.sh Outdated Show resolved Hide resolved

Add check go.mod

da7aa55

anilcse approved these changes Sep 29, 2020

View reviewed changes

robert-zaremba previously requested changes Sep 30, 2020

View reviewed changes

contrib/devtools/proto-tools-installer.sh Outdated Show resolved Hide resolved

Merge branch 'master' into am-proto-gen

c0ffc8d

amaury1093 added 2 commits September 30, 2020 19:14

Remove comment

f9cdb4f

Merge branch 'am-proto-gen' of ssh://github.com/cosmos/cosmos-sdk int…

125727c

…o am-proto-gen

robert-zaremba closed this Sep 30, 2020

robert-zaremba reopened this Sep 30, 2020

This was referenced Oct 1, 2020

ibc: applications restructure #7425

Merged

Make proto-tools script creates unbuildable proto files #7332

Closed

fedekunze approved these changes Oct 1, 2020

View reviewed changes

Merge branch 'master' into am-proto-gen

55d8e2d

amaury1093 commented Oct 1, 2020

View reviewed changes

contrib/devtools/proto-tools-installer.sh Outdated Show resolved Hide resolved

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 1, 2020

amaury1093 and others added 2 commits October 1, 2020 14:19

Update contrib/devtools/proto-tools-installer.sh

b8e942b

Merge branch 'master' into am-proto-gen

33d7b6b

alessio approved these changes Oct 1, 2020

View reviewed changes

mergify bot merged commit 22b47f4 into master Oct 1, 2020

mergify bot deleted the am-proto-gen branch October 1, 2020 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix installation of gocosmos #7410

Fix installation of gocosmos #7410

amaury1093 commented Sep 29, 2020 •

edited

Loading

amaury1093 commented Sep 29, 2020 •

edited

Loading

alexanderbez commented Sep 29, 2020

codecov bot commented Sep 29, 2020 •

edited

Loading

alessio commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020

alessio commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020 •

edited

Loading

robert-zaremba commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020 •

edited

Loading

alessio commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020

ethanfrey commented Sep 30, 2020 •

edited

Loading

alexanderbez commented Sep 30, 2020

fedekunze left a comment

amaury1093 commented Oct 1, 2020

Fix installation of gocosmos #7410

Fix installation of gocosmos #7410

Conversation

amaury1093 commented Sep 29, 2020 • edited Loading

Description

amaury1093 commented Sep 29, 2020 • edited Loading

alexanderbez commented Sep 29, 2020

codecov bot commented Sep 29, 2020 • edited Loading

Codecov Report

alessio commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020

alessio commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020 • edited Loading

robert-zaremba commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020 • edited Loading

alessio commented Sep 30, 2020

robert-zaremba commented Sep 30, 2020

ethanfrey commented Sep 30, 2020 • edited Loading

alexanderbez commented Sep 30, 2020

fedekunze left a comment

Choose a reason for hiding this comment

amaury1093 commented Oct 1, 2020

amaury1093 commented Sep 29, 2020 •

edited

Loading

amaury1093 commented Sep 29, 2020 •

edited

Loading

codecov bot commented Sep 29, 2020 •

edited

Loading

robert-zaremba commented Sep 30, 2020 •

edited

Loading

robert-zaremba commented Sep 30, 2020 •

edited

Loading

ethanfrey commented Sep 30, 2020 •

edited

Loading