Fix proto import paths #2920
Merged
Fix proto import paths #2920
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A proto file's *import path* is relative to one of the `--proto-path`s. Previously, the proto files were compiled separately. Some invocations used different values for the `--proto_path`, which led to inconsistent import paths in proto file descriptors. Typically, this wouldn't be a problem. However, if a downstream project uses `protoregistry.GlobalFiles` to inspect proto dependencies, it will fail to find a dependency's file descriptor when the dependency was compiled with a different `--proto_path`. By using a single script to generate all protobuf files, we can ensure the `--proto_path` is always set to the same sane value (the root of the project, as suggested in the [official documentation]). [official documentation]: https://protobuf.dev/programming-guides/proto2/#importing
joshklop
force-pushed
the
fix-proto-paths
branch
from
debaa0b to
c6b91fa
Compare
|
The commits start with a failing test and builds up to a fully working solution. They should probably be squashed into a single commit before merging to |
joshklop
added a commit
to polymerdao/monomer
that referenced
this pull request
Aug 14, 2024
We recently expunged Pulsar from our repo (yay!). In the process, we needed to upgrade to the next patch version of the Cosmos SDK (v0.50.8). Version v0.50.8 has dependencies on modules that rely on `gogoproto`'s `MergedRegistry` function. Thus, it's not possible to comment out the line (as we were doing [before]), unless we wanted to multiply the number of `go replace` directives. Instead, we address the root cause in the `go-libp2p` package. That's the only new replace directive we need. We have [upstreamed the fix], but that may not be used by the op-node for quite some time. A single `replace` is fine for now. This commit updates the docs with the correct instructions for the latest Monomer changes. [before]: https://github.com/polymerdao/monomer/blob/c7ecbee46914c943bfbecaf2bd5ded3076df09f2/doc/docs/learn/create-an-app-with-monomer.md?plain=1#L150 [upstreamed a fix]: libp2p/go-libp2p#2920
joshklop
added a commit
to polymerdao/monomer
that referenced
this pull request
Aug 14, 2024
We recently expunged Pulsar from our repo (yay!). In the process, we needed to upgrade to the next patch version of the Cosmos SDK (v0.50.8). Version v0.50.8 has dependencies on modules that rely on `gogoproto`'s `MergedRegistry` function. Thus, it's not possible to comment out the line (as we were doing [before]), unless we wanted to multiply the number of `go replace` directives. Instead, we address the root cause in the `go-libp2p` package. That's the only new replace directive we need. We have [upstreamed the fix], but that may not be used by the op-node for quite some time. A single `replace` is fine for now. This commit updates the docs with the correct instructions for the latest Monomer changes. [before]: https://github.com/polymerdao/monomer/blob/c7ecbee46914c943bfbecaf2bd5ded3076df09f2/doc/docs/learn/create-an-app-with-monomer.md?plain=1#L150 [upstreamed the fix]: libp2p/go-libp2p#2920
joshklop
added a commit
to polymerdao/monomer
that referenced
this pull request
Aug 14, 2024
We recently expunged Pulsar from our repo (yay!). In the process, we needed to upgrade to the next patch version of the Cosmos SDK (v0.50.8). Version v0.50.8 has dependencies on modules that rely on `gogoproto`'s `MergedRegistry` function. Thus, it's not possible to comment out the line (as we were doing [before]), unless we wanted to multiply the number of `go replace` directives. Instead, we address the root cause in the `go-libp2p` package. That's the only new replace directive we need. We have [upstreamed the fix], but that may not be used by the op-node for quite some time. A single `replace` is fine for now. This commit updates the docs with the correct instructions for the latest Monomer changes. [before]: https://github.com/polymerdao/monomer/blob/c7ecbee46914c943bfbecaf2bd5ded3076df09f2/doc/docs/learn/create-an-app-with-monomer.md?plain=1#L150 [upstreamed the fix]: libp2p/go-libp2p#2920
joshklop
added a commit
to polymerdao/monomer
that referenced
this pull request
Aug 14, 2024
We recently expunged Pulsar from our repo (yay!). In the process, we needed to upgrade to the next patch version of the Cosmos SDK (v0.50.8). Version v0.50.8 has dependencies on modules that rely on `gogoproto`'s `MergedRegistry` function. Thus, it's not possible to comment out the line (as we were doing [before]), unless we wanted to multiply the number of `go replace` directives. Instead, we address the root cause in the `go-libp2p` package. That's the only new replace directive we need. We have [upstreamed the fix], but that may not be used by the op-node for quite some time. A single `replace` is fine for now. This commit updates the docs with the correct instructions for the latest Monomer changes. [before]: https://github.com/polymerdao/monomer/blob/c7ecbee46914c943bfbecaf2bd5ded3076df09f2/doc/docs/learn/create-an-app-with-monomer.md?plain=1#L150 [upstreamed the fix]: libp2p/go-libp2p#2920
joshklop
added a commit
to polymerdao/monomer
that referenced
this pull request
Aug 14, 2024
We recently expunged Pulsar from our repo (yay!). In the process, we needed to upgrade to the next patch version of the Cosmos SDK (v0.50.8). Version v0.50.8 has dependencies on modules that rely on `gogoproto`'s `MergedRegistry` function. Thus, it's not possible to comment out the line (as we were doing [before]), unless we wanted to multiply the number of `go replace` directives. Instead, we address the root cause in the `go-libp2p` package. That's the only new replace directive we need. We have [upstreamed the fix], but that may not be used by the op-node for quite some time. A single `replace` is fine for now. This commit updates the docs with the correct instructions for the latest Monomer changes. [before]: https://github.com/polymerdao/monomer/blob/c7ecbee46914c943bfbecaf2bd5ded3076df09f2/doc/docs/learn/create-an-app-with-monomer.md?plain=1#L150 [upstreamed the fix]: libp2p/go-libp2p#2920
MarcoPolo
approved these changes
Aug 15, 2024
|
Interop test is failing, but it's not this PR's fault. It's just running out of space on the runner. |
MarcoPolo
added a commit
that referenced
this pull request
Sep 4, 2024
* Add failing proto test * Add a new proto compilation script A proto file's *import path* is relative to one of the `--proto-path`s. Previously, the proto files were compiled separately. Some invocations used different values for the `--proto_path`, which led to inconsistent import paths in proto file descriptors. Typically, this wouldn't be a problem. However, if a downstream project uses `protoregistry.GlobalFiles` to inspect proto dependencies, it will fail to find a dependency's file descriptor when the dependency was compiled with a different `--proto_path`. By using a single script to generate all protobuf files, we can ensure the `--proto_path` is always set to the same sane value (the root of the project, as suggested in the [official documentation]). [official documentation]: https://protobuf.dev/programming-guides/proto2/#importing * Add go_package options so scripts/gen-proto.sh succeeds * Remove undesirable `go:generate protoc` directives * Run `go generate ./...` * Script uses arrays, I think we need bash --------- Co-authored-by: Marco Munizaga <git@marcopolo.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cosmos SDK modules may inspect
protoregistry.GlobalFilesin SDK v0.50.8 and later. As a result, this appears to be necessary to use libp2p in the same binary as those modules.