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

[Code Health] Investigate a permanent solution to pulsar self-import issue #405

Open
5 tasks
bryanchriswhite opened this issue Feb 29, 2024 · 7 comments
Open
5 tasks
Assignees
Labels
code health Cleans up some code devex Developer experience related improvements. tooling Tooling - CLI, scripts, helpers, off-chain, etc...

Comments

@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Feb 29, 2024

Objective

Remove proto_fix_self_import make target workaround.

Origin Document

  1. Post v0.50.x cosmos-sdk leverages the v2 go protobuf API, which necessitates tracking api/poktroll/**/*.go (generated) files in version control. Otherwise, the ignite CLI becomes unusable as it requries go mod tidy to succeed for all subcommands, which is not possible without these files as they are imported by each respective module's module.go and autocli.go files.
  2. When .proto files import protobuf types from other .proto files, it seems that we MUST add a corresponding option in the plugins.opt field in buf.gen.pulsar.yaml to mitigate the following error during generation:
    image
  3. So long as we MUST do 2 (and we do not apply the workaround), a new self-import error arises in the corresponding generated *.pulsar.go code immediately after generation (i.e. try ignite generate proto-go && ignite chain build --skip-proto):
    image

Goals

  • Determine whether this phenomenon is a result of misconfiguration on our end or a bug in cosmos-proto's
    • Outcome:
  • Either:
    • Open an issue in the cosmos-proto (or other appropriate) repo.
    • Correct the misconfiguration.

Deliverables

  • ...

Non-goals / Non-deliverables

  • ...

General deliverables

  • Comments: Add/update TODOs and comments alongside the source code so it is easier to follow.
  • Testing: Add new tests (unit and/or E2E) to the test suite.
  • Makefile: Add new targets to the Makefile to make the new functionality easier to use.
  • Documentation: Update architectural or development READMEs; use mermaid diagrams where appropriate.

Creator: @bryanchriswhite
Co-Owners: @red-0ne

@bryanchriswhite bryanchriswhite self-assigned this Feb 29, 2024
@bryanchriswhite bryanchriswhite added the tooling Tooling - CLI, scripts, helpers, off-chain, etc... label Feb 29, 2024
@bryanchriswhite bryanchriswhite moved this to 🔖 Ready in Shannon Feb 29, 2024
@bryanchriswhite bryanchriswhite added the code health Cleans up some code label Mar 1, 2024
@Olshansk Olshansk moved this from 🔖 Ready to 📋 Backlog in Shannon Aug 5, 2024
@bryanchriswhite
Copy link
Contributor Author

bryanchriswhite commented Aug 21, 2024

Try renaming any .proto files which are currently named after their respective modules; e.g., proto/poktroll/session/session.proto, proto/poktroll/service/service.proto, etc.

See:

image

We can also probably remove these files from the buf.gen.pulsar.yaml plugins.opt field:

plugins:
  - name: go-pulsar
    out: ./api
-    opt: paths=source_relative,Mpoktroll/shared/service.proto=github.com/pokt-network/poktroll/api/poktroll/shared,Mpoktroll/shared/supplier.proto=github.com/pokt-network/poktroll/api/poktroll/shared,Mpoktroll/supplier/supplier.proto=github.com/pokt-network/poktroll/api/poktroll/supplier,Mpoktroll/session/session.proto=github.com/pokt-network/poktroll/api/poktroll/session
+    opt: paths=source_relative

@bryanchriswhite bryanchriswhite moved this from 📋 Backlog to 🏗 In progress in Shannon Aug 21, 2024
@bryanchriswhite bryanchriswhite moved this from 🏗 In progress to 📋 Backlog in Shannon Aug 21, 2024
@bryanchriswhite
Copy link
Contributor Author

Try renaming any .proto files which are currently named after their respective modules; e.g., proto/poktroll/session/session.proto, proto/poktroll/service/service.proto, etc.

We can also probably remove these files from the buf.gen.pulsar.yaml plugins.opt field:

No dice. 🎲 The proto_fix_self_import target seems to still be necessary despite these changes (see: #749). 😞

bryanchriswhite added a commit that referenced this issue Aug 22, 2024
## Summary

Renames all .proto files which are named the same as their protobuf
package to workaround/mitigate an issue which presents when importing
them in other .proto files.

## Issue

- Potentially related to (but **does not** fix) #405 

![Screenshot 2024-08-21 at 3 39
51 PM](https://github.com/user-attachments/assets/6a7d1d2a-daba-424c-a930-d065061fe4d2)


## Type of change

Select one or more:

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [x] **Unit Tests**: `make go_develop_and_test`
- [x] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [ ] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [x] I have left TODOs throughout the codebase, if applicable
@Olshansk
Copy link
Member

Olshansk commented Nov 4, 2024

@bryanchriswhite Is this still an open issue? Please close out if not.

1 similar comment
@Olshansk
Copy link
Member

Olshansk commented Nov 4, 2024

@bryanchriswhite Is this still an open issue? Please close out if not.

@bryanchriswhite
Copy link
Contributor Author

This is still an issue that we're applying a workaround for.

@Olshansk
Copy link
Member

Olshansk commented Nov 5, 2024

@bryanchriswhite Did you ever open an issue in https://github.com/cosmos/cosmos-proto?

Seemed to be one of the notes in the original description.

@bryanchriswhite
Copy link
Contributor Author

I did not. I wanted to ensure that it wasn't our fault first. My plan was to scaffold a new chain and try to reproduce the issue there. I think this exercise will also improve the quality of any issue that we do open.

@bryanchriswhite bryanchriswhite added the devex Developer experience related improvements. label Nov 6, 2024
okdas pushed a commit that referenced this issue Nov 14, 2024
## Summary

Renames all .proto files which are named the same as their protobuf
package to workaround/mitigate an issue which presents when importing
them in other .proto files.

## Issue

- Potentially related to (but **does not** fix) #405 

![Screenshot 2024-08-21 at 3 39
51 PM](https://github.com/user-attachments/assets/6a7d1d2a-daba-424c-a930-d065061fe4d2)


## Type of change

Select one or more:

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [x] **Unit Tests**: `make go_develop_and_test`
- [x] **LocalNet E2E Tests**: `make test_e2e`
- [x] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [ ] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [x] I have left TODOs throughout the codebase, if applicable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code devex Developer experience related improvements. tooling Tooling - CLI, scripts, helpers, off-chain, etc...
Projects
Status: 📋 Backlog
Development

No branches or pull requests

3 participants