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

proto: update the mechanism for generating protos from spec repo #7269

Merged
merged 23 commits into from
Nov 15, 2021

Conversation

williambanfield
Copy link
Contributor

@williambanfield williambanfield commented Nov 9, 2021

This pull request updates the protocgen.sh script to insert the go_package option to all of the downloaded proto files. A related pull request into the spec repo removes this options from the .proto files: tendermint/spec#358

This pull requests, along with the related spec PR, aim to move the creation of the tendermintdev/docker-build-proto container into the spec repo. This change also relies on several fixes to that container that are made in the PR into the spec repo.

Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

This looks pretty reasonable to me. I have a few small things, but nothing that crucial. (Mostly just quoting and formatting nonsense, because shells are pernicious)

Makefile Show resolved Hide resolved
scripts/protopackage.sh Outdated Show resolved Hide resolved
scripts/protopackage.sh Outdated Show resolved Hide resolved
scripts/protopackage.sh Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen.sh Show resolved Hide resolved
scripts/protocgen.sh Show resolved Hide resolved
# Get shortened ref of commit
REF=$(curl -H "Accept: application/vnd.github.v3.sha" -qL \
"https://api.github.com/repos/tendermint/spec/commits/${VERS}" \
| cut -c -7)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably no real need to truncate the hash (though it is harmless).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see why you did that, because of the way the tarballer names its directory. ✔️

scripts/protocgen.sh Outdated Show resolved Hide resolved
scripts/protocgen.sh Outdated Show resolved Hide resolved
@williambanfield williambanfield added the S:automerge Automatically merge PR when requirements pass label Nov 15, 2021
@mergify mergify bot merged commit d5df412 into master Nov 15, 2021
@mergify mergify bot deleted the wb/proto-fixes branch November 15, 2021 20:06
tychoish pushed a commit to tychoish/tendermint that referenced this pull request Nov 19, 2021
…dermint#7269)

This pull request updates the `protocgen.sh` script to insert the `go_package` option to all of the downloaded proto files. A related pull request into the spec repo removes this options from the .proto files: tendermint/spec#358

This pull requests, along with the related spec PR, aim to move the creation of the `tendermintdev/docker-build-proto` container into the spec repo. This change also relies on several fixes to that container that are made in the PR into the spec repo.
creachadair pushed a commit that referenced this pull request Nov 19, 2021
This is a manual backport of the changes to how we build and run the protobuf
toolchain images in Docker. The main effect here is to point to the new image
from ghcr.io/tendermint/docker-proto-builder, but to make that work it is also
necessary to update some of the branch pointers.

This change does NOT include the changes from #7269 and #7291 to point to the
proto files in the spec repo. To do that, we will need to create a branch or
tag on the spec that has the released version, which does not exist in the spec
history as it currently stands.
creachadair pushed a commit that referenced this pull request Nov 22, 2021
This is a manual backport of the changes to how we build and run the protobuf
toolchain images in Docker. The main effect here is to point to the new image
from ghcr.io/tendermint/docker-proto-builder, but to make that work it is also
necessary to update some of the branch pointers.

This change does NOT include the changes from #7269 and #7291 to point to the
proto files in the spec repo. To do that, we will need to create a branch or
tag on the spec that has the released version, which does not exist in the spec
history as it currently stands.
lklimek referenced this pull request in dashpay/tenderdash Mar 25, 2022
This is a manual backport of the changes to how we build and run the protobuf
toolchain images in Docker. The main effect here is to point to the new image
from ghcr.io/tendermint/docker-proto-builder, but to make that work it is also
necessary to update some of the branch pointers.

This change does NOT include the changes from #7269 and #7291 to point to the
proto files in the spec repo. To do that, we will need to create a branch or
tag on the spec that has the released version, which does not exist in the spec
history as it currently stands.

(cherry picked from commit a97b081)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:automerge Automatically merge PR when requirements pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants