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

feat: Go Workspace #11453

Closed
wants to merge 94 commits into from
Closed

feat: Go Workspace #11453

wants to merge 94 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Mar 24, 2022

Description

Closes: #11450

This PR adds go workspaces, and bumps the version of go in all of the cosmos-sdk modules to v1.18.

It also stops using the diff action because that breaks ci / workspaces in ci.


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

@github-actions github-actions bot added C:depinject Issues and PR related to depinject C:Cosmovisor Issues and PR related to Cosmovisor C:orm C:Store labels Mar 24, 2022
@faddat faddat changed the title Workspace feat: Go Workspace Mar 25, 2022
api/go.mod Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome, I need to read up on how this works exactly. im slightly confused.

Thank you for the PR. ill try to do a better review later

@faddat
Copy link
Contributor Author

faddat commented Mar 25, 2022

I was slightly confused, as well :)

@github-actions github-actions bot added C:Rosetta Issues and PR related to Rosetta Type: Build T: CI labels Mar 25, 2022
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left more questions, I still haven't read how workspaces work, so the questions are curiosity based

README.md Outdated Show resolved Hide resolved
api/go.mod Outdated Show resolved Hide resolved
contrib/devtools/dockerfile Outdated Show resolved Hide resolved
cosmovisor/go.mod Outdated Show resolved Hide resolved
go.work Outdated Show resolved Hide resolved
orm/go.mod Outdated Show resolved Hide resolved
@faddat
Copy link
Contributor Author

faddat commented Apr 12, 2022

Hey, so the sync command is broken again due to imports of gogo/protobuf v1.3.3 which doesn’t actually exist.

as for some of your other questions I am not certain but I figure what I’ll do is get this into a working state locally and push. Seems we may need the replace for sdk in cosmovisor, which is less than ideal.

probably just another driver to pull gogo out of the stack where we can

@faddat
Copy link
Contributor Author

faddat commented Apr 15, 2022

hi, I reworked this today.

I now have an answer to your question about replaces:

As soon as there's a release of the cosmos-sdk that doesn't refrence:

	github.com/cosmos/[email protected] requires github.com/gogo/[email protected], but github.com/gogo/[email protected] is requested

-- a version of gogo/protobuf that does not exist upstream, then the modules in the sdk can refer to those "other cosmos SDK's" without trouble.

Cosmovisor seems to be the last one that is extremely picky about this.

@faddat
Copy link
Contributor Author

faddat commented Apr 15, 2022

basically cleanly making this PR without those replaces depends on #11414

Would it be okay to:

the specific issue is that gogo/protobuf v1.3.3 does not exist -- everything must reference gogo/protobuf v1.3.2 (even a replace in go.work doesn't work, I am not sure why)

The fastest and best path to ship this may be:

Update 6/10/2022

This branch has working go workspaces. That said, the go workspaces have killed CI.

go 1.18 (installs that are grouped must have same version so there are more run commands)
@faddat faddat marked this pull request as draft April 15, 2022 22:23
@github-actions github-actions bot removed the T: CI label Apr 19, 2022
@robert-zaremba
Copy link
Collaborator

FYI, I was trying something similar in Umee, but got into the following blocker and decided to not continue: umee-network/umee#1056 (comment)

@github-actions github-actions bot removed the Type: CI label Jul 15, 2022
@faddat faddat mentioned this pull request Jul 15, 2022
19 tasks
mergify bot pushed a commit that referenced this pull request Jul 18, 2022
## Description

This helps to prepare for #11453 by removing the ics23 dependency on gogo/protobuf v1.3.3 which does not exist.

It will have an outsized quality of life impact.

For reference the way to do this is:

* keep the replace as-is
* `go get github.com/gogo/[email protected]`

### Affects
* cosmovisor
* ics23
* sdk

### Prerequisite to:

* #11453 (or maybe it helps it happen?  I think after this we won't get those troublesome 1.3.3 imports)

Maybe not exactly a prerequisite, more like something that will reduce the review scope of #11453 and also eliminate this problem for downstream users of the sdk. 

To clarify the purpose here, the command go work sink, does not work because the upstream version 1.3.3 does not exist in gogo/protobuf

---

### 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...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
@@ -5,8 +5,8 @@ import (
fmt "fmt"
_ "github.com/cosmos/cosmos-proto"
runtime "github.com/cosmos/cosmos-proto/runtime"
v1beta11 "github.com/cosmos/cosmos-sdk/api/cosmos/base/query/v1beta1"
v1beta1 "github.com/cosmos/cosmos-sdk/api/cosmos/base/v1beta1"
v1beta11 "cosmossdk.io/api/cosmos/base/query/v1beta1"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no clue why this is happening.

did we make such a version?

I cannot find any reference to this, anywhere else in the code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably there is a name conflict

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder why the changes would cause this....

@faddat
Copy link
Contributor Author

faddat commented Jul 21, 2022

I am going to wait to resolve conflicts here until #12645 is merged. That will make landing this PR much easier.

@faddat
Copy link
Contributor Author

faddat commented Jul 21, 2022

Closing to reopen, now that some of the blockers are out of the way in main. It doesn't make sense to review a 94 commit behemoth like this one imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:Cosmovisor Issues and PR related to Cosmovisor C:depinject Issues and PR related to depinject C:orm C:Store S:blocked Status: Blocked
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

go workspaces
4 participants