feat: go workspaces #12675

faddat · 2022-07-21T12:29:26Z

Description

A much better, cleaner go workspaces PR.

In the previous state, the replaces in go.mod were keeping Go from making pseudo versions that referenced an exact commit properly. Once I got the pseudo versions in using go get, things worked great.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

julienrbrt

So, I am super not up to speed with go workspaces, but the more I read about it, the less I am sure we should commit a go.work and go.work.sum at all.

From my understanding, that is just for avoiding committing the replaces like these https://github.com/cosmos/cosmos-sdk/blob/main/go.mod#L282-L287.

We should avoid having any replace anyway (these) and start tagging our changes. Which means when we need a feature, first update the needed module, and then use that feature.

Otherwise, it seems like we should add GOWORK=off for our build, but this seems hacky. Maybe this is something we should all use locally but never commit, for ensuring every module are always up-to-date and tagged as they should be. Something to have in https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md instead, and actually add go.work and go.work.sum in the .gitignore?

If I am saying something wrong, please correct me, I am still learning and reading about this feature.

I would gladly have @aaronc or maybe even @matloob input on this one.

faddat · 2022-07-22T02:09:04Z

@julienrbrt you may be 100% correct here.

I'm not sure.

The biggest benefit I observed was that go work sync replaces the go-mod-tidy-all.sh script and that it makes my vscode a lot happier.

I guess another thing I'd like to see us avoid is becoming a module soup like libp2p, but i suppose there's a question on that:

will go workspaces help us to avoid module-soupification?

faddat · 2022-07-26T10:17:46Z

@julienrbrt --

from my (limited) experience working with Go workspaces, I think that you may have one thing wrong.

I (think) they're designed for monorepo situations like the cosmos-sdk, and yes, they ought to replace the replace statements, but also, maybe most importantly they'll give developer's editors (like vscode) some information about the sdk so that the editor's usual features work correctly.

So, overall, I do think that we would want to commit a go.work and a go.work.sum -- but .... I'm definitely not certain of any of this.

One thing that I can say with decent certainty is that I don't think that this does us any harm, and may make editing easier.

Maybe check out additional comments in #11450 :).

julienrbrt

Fair enough, I see the advantage for main. However, I do think this should not make it in a release. Instead, we should remove it and tag the modules.
This means we need to update the RELEASE_PROCESS.md to reflect that.

faddat · 2022-07-26T10:30:12Z

@julienrbrt -- since this is now passing every test (honestly for reasons I don't fully understand) -- I think that it makes sense to merge.

I think that it will make working on and with the sdk easier.

If it doesn't then I figure we just pull it out.

wdyt?

issues addressed

julienrbrt · 2022-07-26T10:34:06Z

Just curious, it is normal that these replace are still there https://github.com/notional-labs/cosmos-sdk/blob/11b3d8b555b1121866b90dca8671d61fb3d9a2d7/go.mod#L284-L289?

faddat · 2022-07-26T12:41:28Z

@julienrbrt -- I don't know. Happy to play around a bit locally on that & revert w/ info.

faddat · 2022-07-26T12:44:17Z

Fair enough, I see the advantage for main. However, I do think this should not make it in a release. Instead, we should remove it and tag the modules.

Unsure of this-- depends what we want... LIkely we do tag the modules anyway, because iirc the idea is to have idependent release schedules for them.

hmmmmm

inconclusive, will think some

This means we need to update the RELEASE_PROCESS.md to reflect that.

faddat · 2022-07-26T12:47:16Z

@julienrbrt I played around, result is:

I think that we need replaces, but only for the modules that we publish at cosmossdk.io

The others can be removed, and I figure that it makes sense to do so, so I'm updating this to reflect that.

i was incorrect!

heh-- so I think we need all of them otherwise we get:

go: github.com/cosmos/cosmos-sdk/store/tools/[email protected]: invalid version: unknown revision 000000000000

I'll try a mod tidy all then a work sync and see if that makes it happier...

faddat · 2022-07-26T12:55:05Z

ok, might have a solution to some of the replaces. Our replaces may be breaking psseudo-versions.

The question is then:

Do we want the sdk's main branch to always use the versions of modules that are also in main?

If we keep the replaces, that's what'll happen.

If we remove them, main will import a specific version of those modules.

julienrbrt · 2022-07-26T13:03:03Z

Do we need to tag a few go modules then (as beta)? This is long overdue anyway.

faddat · 2022-07-26T13:03:44Z

but, 3rd update:

I can get rid of the replaces... I'll push, and would love to hear your opinion. 10m maybe

faddat · 2022-07-26T13:06:38Z

I think it'll help you to see my terminal log....

Functionally-speaking, the replaces (all of them) were ensuring that main always referenced the exact versions of things in main.

I've now got a go.mod without replaces.

Context: The replaces were stopping go get, etc from peoperly making pseudo-versions. The solution was to go get while the replaces were still there, then remove them.

This seems to do / work how we would like it to.

gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go work sync
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get github.com/cosmos/cosmos-sdk/store/tools/ics23@main
go: github.com/cosmos/cosmos-sdk/store/tools/[email protected]: invalid version: unknown revision 000000000000
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get github.com/cosmos/cosmos-sdk/store/tools/ics23@main
go: github.com/cosmos/cosmos-sdk/store/tools/[email protected]: invalid version: unknown revision 000000000000
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get github.com/cosmos/cosmos-sdk/store/tools/ics23@dc03ed1
go: github.com/cosmos/cosmos-sdk/store/tools/[email protected]: invalid version: unknown revision 000000000000
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get github.com/cosmos/cosmos-sdk/store/tools/ics23@main
go: upgraded github.com/cosmos/cosmos-sdk/store/tools/ics23 v0.0.0-00010101000000-000000000000 => v0.0.0-20220726092710-f848e4300a8a
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get github.com/cosmos/db@main
go: github.com/cosmos/db@main: invalid version: git ls-remote -q origin in /workspace/go/pkg/mod/cache/vcs/95f1ea7b09879117d08d9b90f66216346a3e281f68212b4ff5d29ab47ff3d754: exit status 128:
        remote: Repository not found.
        fatal: repository 'https://github.com/cosmos/db/' not found
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get github.com/cosmos/cosmos-sdk/db@main
go: upgraded github.com/cosmos/cosmos-sdk/db v1.0.0-beta.1 => v1.0.0-beta.1.0.20220726092710-f848e4300a8a
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go mod tidy
go: downloading github.com/cosmos/cosmos-sdk/store/tools/ics23 v0.0.0-20220726092710-f848e4300a8a
go: downloading github.com/cosmos/cosmos-sdk/db v1.0.0-beta.1.0.20220726092710-f848e4300a8a
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go work sync
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get cosmossdk.io/api@main
go: upgraded cosmossdk.io/api v0.1.0-alpha8 => v0.1.1-0.20220726092710-f848e4300a8a
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get cosmossdk.io/core@main
go: upgraded cosmossdk.io/core v0.0.0 => v0.1.1-0.20220726092710-f848e4300a8a
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get cosmossdk.io/depinject@main
go: downgraded cosmossdk.io/core v0.1.1-0.20220726092710-f848e4300a8a => v0.1.0
go: downgraded cosmossdk.io/depinject v1.0.0-alpha.4 => v1.0.0-alpha.1.0.20220726092710-f848e4300a8a
gitpod /workspace/cosmos-sdk (new-go-workspaces) $ go get cosmossdk.io/math@main

julienrbrt

thanks! lgtm! I am still not sure if we should update the release process for describing that we remove the go.work and go.work.sum when cutting a new release.

julienrbrt · 2022-07-26T14:29:21Z

go.mod

@@ -65,7 +65,7 @@ require (
 	sigs.k8s.io/yaml v1.3.0
 )

-require github.com/cosmos/cosmos-sdk/store/tools/ics23 v0.0.0-00010101000000-000000000000
+require github.com/cosmos/cosmos-sdk/store/tools/ics23 v0.0.0-20220726092710-f848e4300a8a


nit can we merge this require block with the one above?

julienrbrt · 2022-07-26T19:35:04Z

Let's remember this issue when we need to release a new version and decide the process. Merging now. Thanks @faddat!

tac0turtle · 2022-07-27T09:50:45Z

cosmovisor/go.mod

@@ -108,7 +101,7 @@ require (
 	github.com/tendermint/btcd v0.1.1 // indirect
 	github.com/tendermint/crypto v0.0.0-20191022145703-50d29ede1e15 // indirect
 	github.com/tendermint/go-amino v0.16.0 // indirect
-	github.com/tendermint/tendermint v0.34.20 // indirect
+	github.com/tendermint/tendermint v0.35.9 // indirect


we should figure out where this upgrade is coming from. We should have the whole repo on 0.34.20

Yes, I was checking exactly that in #12741. It turns out if we go mod tidy in ./cosmovisor right now we get an error. If we remove these indirect imports and go mod tidy it will put it back to 0.34.20. However, as soon as we go work sync this forces cosmovisor to have tm 0.35.9 as indirect dependency.

I don't think we need cosmovisor to be included in the go.work anyway, as cosmovisor is not used by the sdk. Investigating...

go workspaces

28b0c9c

faddat requested a review from a team as a code owner July 21, 2022 12:29

github-actions bot added C:CLI C:Cosmovisor Issues and PR related to Cosmovisor C:Store labels Jul 21, 2022

faddat changed the title ~~go workspaces~~ feat: go workspaces Jul 21, 2022

faddat added 2 commits July 21, 2022 14:22

tidy

3c7b176

catch all 11 modules

416cc10

github-actions bot added the C:orm label Jul 21, 2022

Merge branch 'main' into new-go-workspaces

4946595

julienrbrt previously requested changes Jul 21, 2022

View reviewed changes

Merge branch 'main' into new-go-workspaces

ed60717

faddat added 4 commits July 22, 2022 15:57

Merge branch 'main' into new-go-workspaces

06354c8

Merge branch 'main' into new-go-workspaces

9735a70

Merge branch 'main' into new-go-workspaces

11b3d8b

Merge branch 'main' into new-go-workspaces

26d8c32

faddat mentioned this pull request Jul 26, 2022

go workspaces #11450

Closed

julienrbrt reviewed Jul 26, 2022

View reviewed changes

go.mod no longer replaces modules in the sdk

d619229

julienrbrt approved these changes Jul 26, 2022

View reviewed changes

faddat mentioned this pull request Jul 26, 2022

use go workspaces cosmos/ibc-go#1784

Closed

9 tasks

faddat added 5 commits July 26, 2022 18:38

correct ics23 import

ccb0ebe

indirect in indirect list

daea751

Merge remote-tracking branch 'upstream/main' into new-go-workspaces

df44c11

Merge branch 'main' into new-go-workspaces

4c500a8

fix cosmovisor

16366e1

julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 26, 2022

julienrbrt merged commit 53347cd into cosmos:main Jul 26, 2022

likhita-809 mentioned this pull request Jul 27, 2022

feat: Add x/authz SendAuthorization AllowList #12648

Merged

19 tasks

tac0turtle reviewed Jul 27, 2022

View reviewed changes

julienrbrt mentioned this pull request Jul 27, 2022

build: remove comosvisor from go.work #12743

Merged

19 tasks

faddat mentioned this pull request Aug 1, 2022

gogo/protobuf to 1.3.2, which exists, enabling go workspaces downstream cosmos/ibc-go#1844

Merged

9 tasks

julienrbrt mentioned this pull request Aug 17, 2022

build: ensure GOWORK=off make test passes #12951

Merged

19 tasks

julienrbrt mentioned this pull request Aug 30, 2022

go.work should be in .gitignore #13098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: go workspaces #12675

feat: go workspaces #12675

faddat commented Jul 21, 2022 •

edited

Loading

julienrbrt left a comment •

edited

Loading

faddat commented Jul 22, 2022 •

edited

Loading

faddat commented Jul 26, 2022 •

edited

Loading

julienrbrt left a comment

faddat commented Jul 26, 2022

julienrbrt commented Jul 26, 2022 •

edited

Loading

faddat commented Jul 26, 2022

faddat commented Jul 26, 2022

faddat commented Jul 26, 2022 •

edited

Loading

faddat commented Jul 26, 2022

julienrbrt commented Jul 26, 2022 •

edited

Loading

faddat commented Jul 26, 2022

faddat commented Jul 26, 2022 •

edited

Loading

julienrbrt left a comment

julienrbrt Jul 26, 2022

faddat Jul 26, 2022

julienrbrt commented Jul 26, 2022

tac0turtle Jul 27, 2022

julienrbrt Jul 27, 2022

julienrbrt Jul 27, 2022 •

edited

Loading

feat: go workspaces #12675

feat: go workspaces #12675

Conversation

faddat commented Jul 21, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

julienrbrt left a comment • edited Loading

Choose a reason for hiding this comment

faddat commented Jul 22, 2022 • edited Loading

faddat commented Jul 26, 2022 • edited Loading

julienrbrt left a comment

Choose a reason for hiding this comment

faddat commented Jul 26, 2022

julienrbrt commented Jul 26, 2022 • edited Loading

faddat commented Jul 26, 2022

faddat commented Jul 26, 2022

faddat commented Jul 26, 2022 • edited Loading

faddat commented Jul 26, 2022

julienrbrt commented Jul 26, 2022 • edited Loading

faddat commented Jul 26, 2022

faddat commented Jul 26, 2022 • edited Loading

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt Jul 26, 2022

Choose a reason for hiding this comment

faddat Jul 26, 2022

Choose a reason for hiding this comment

julienrbrt commented Jul 26, 2022

tac0turtle Jul 27, 2022

Choose a reason for hiding this comment

julienrbrt Jul 27, 2022

Choose a reason for hiding this comment

julienrbrt Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

faddat commented Jul 21, 2022 •

edited

Loading

julienrbrt left a comment •

edited

Loading

faddat commented Jul 22, 2022 •

edited

Loading

faddat commented Jul 26, 2022 •

edited

Loading

julienrbrt commented Jul 26, 2022 •

edited

Loading

faddat commented Jul 26, 2022 •

edited

Loading

julienrbrt commented Jul 26, 2022 •

edited

Loading

faddat commented Jul 26, 2022 •

edited

Loading

julienrbrt Jul 27, 2022 •

edited

Loading