Cache the grpcbox tools for make grpc#54033
Conversation
espadolini
added
the
no-changelog
espadolini
requested review from
ravicious
and removed request for
fspmarshall and
gabrielcorado
|
cc @ravicious for |
| - npm | ||
| - exec | ||
| - --yes | ||
| - --prefer-offline |
There was a problem hiding this comment.
I guess we could use --offline here, but then make grpc/host would fail on machines that don't have @protobuf-ts/plugin installed. Maybe we could leave a comment about that?
There was a problem hiding this comment.
I specifically made it --prefer-offline for that (and also because, generally speaking, we know which exact version we want, so if it's already installed why should we bother looking for it online again?)
| # anything. | ||
| # | ||
| # The version should be kept in sync with buf-ts.gen.yaml. | ||
| RUN npm exec --yes --package=@protobuf-ts/plugin@2.9.3 -- true |
There was a problem hiding this comment.
| RUN npm exec --yes --package=@protobuf-ts/plugin@2.9.3 -- true | |
| RUN npm install --global @protobuf-ts/plugin@2.9.3 |
npm exec is going to look first for locally installed packages and then globally installed packages. I verified that this works by changing --prefer-offline to --offline, running the new version with success, then commenting out npm install --global and verifying that npm exec fails with the package not being installed.
There was a problem hiding this comment.
Installing the package globally is slightly stronger, isn't it? Assuming that the PATH is set up correctly, it would actually surface protoc-gen-ts and protoc in the path for all command executions, which is not something that we need or want (even though it would be harmless in here).
| --build-arg BUF_VERSION=$(BUF_VERSION) \ | ||
| --build-arg GOGO_PROTO_TAG=$(GOGO_PROTO_TAG) \ | ||
| --build-arg NODE_GRPC_TOOLS_VERSION=$(NODE_GRPC_TOOLS_VERSION) \ | ||
| --build-arg NODE_PROTOC_TS_VERSION=$(NODE_PROTOC_TS_VERSION) \ |
There was a problem hiding this comment.
Do you mind creating another PR that completely removes NODE_GRPC_TOOLS_VERSION and NODE_PROTOC_TS_VERSION from the project? Those are used only in the Dockerfile where I suspect they're no longer needed – someone probably didn't clean that up after creating grpcbox.
There was a problem hiding this comment.
Will do, I haven't done it here because modifying the actual buildbox is always a bit sketchy 😅
public-teleport-github-review-bot
Bot
removed request for
fspmarshall and
gabrielcorado
3 participants
This PR cleans up the "grpcbox" docker image such that no download or go compilation happens during a
make grpcinvocation - currently the goprotocplugins end up being compiled (but not downloaded) during generation andprotoc-gen-tsgets downloaded. In addition, this PR gets rid of some dependencies in the image that are no longer used.