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

Using latest protobuf version, fixing protoc-gen-js install... #30

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnsabath
Copy link
Contributor

@johnsabath johnsabath commented Oct 20, 2022

... and adding docker helper script for building outputs locally

@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

ARG PROTOC_GEN_GO_VERSION=v1.28.1
ARG PROTOC_GEN_GO_GRPC_VERSION=v1.1.0
ARG GRPC_VERSION=v1.50.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this impact the Python build? There's specific reasons why we selected this gRPC version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm inclined to say no since the generated python output hasn't changed, but I could verify this with local testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

If no, then ok. Do verify quickly, though.

Copy link
Member

Choose a reason for hiding this comment

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

The version of protoc built here is not used, see here:
#27

WORKDIR /build
RUN git clone --recurse-submodules -b ${GRPC_VERSION} --depth 1 --shallow-submodules https://github.com/grpc/grpc
WORKDIR /build/grpc
RUN mkdir -p cmake/build; cd cmake/build; cmake -DgRPC_INSTALL=ON -DgRPC_BUILD_TESTS=OFF -DCMAKE_INSTALL_PREFIX=/usr/local ../.. && make -j 8 && make install

# Install protoc-gen-js
WORKDIR /build/protoc-gen-js
RUN wget -O protobuf-javascript.zip https://github.com/protocolbuffers/protobuf-javascript/releases/download/v${PROTOBUF_JAVASCRIPT_VERSION}/protobuf-javascript-${PROTOBUF_JAVASCRIPT_VERSION}-linux-x86_64.zip && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap these super-long lines please.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would seem to me that doing a git checkout to a release branch is a lot less brittle than this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would seem to me that doing a git checkout to a release branch is a lot less brittle than this.

To be clear, are you suggesting that we do a git checkout of the release branch and build the binary in docker instead of using the pre-built binary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. Maybe? It would allow the protobufs to be built on non-x86-64 containers, but I'm not sure if this is relevant.

Dockerfile Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants