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: allow shell vars to propagate to protoc. #779

Closed
wants to merge 1 commit into from

Conversation

htuch
Copy link
Contributor

@htuch htuch commented Aug 31, 2017

Similar to protocolbuffers/protobuf#2508, when using
protoc with non-standard LD_LIBRARY_PATH, we need to be able to have it
pick up from the environment the correct LD_LIBRARY_PATH.

Similar to protocolbuffers/protobuf#2508, when using
protoc with non-standard LD_LIBRARY_PATH, we need to be able to have it
pick up from the environment the correct LD_LIBRARY_PATH.
@bazel-io
Copy link

Can one of the admins verify this patch?

@ianthehat
Copy link
Contributor

I think that use_default_shell_env is fundamentally incompatible with remote execution, this is probably not the right way to solve the problem.
Why is protoc special (compared to any other host binary), why does it need a custom LD_LIBRARY_PATH?

@htuch
Copy link
Contributor Author

htuch commented Aug 31, 2017

It's built under Bazel using a C++ toolchain configured with CC/CXX/LD_LIBRARY_PATH. Other host binaries are built with whatever the host base image is using. FWIW, we're building on Goobuntu with CXX/LD_LIBRARY_PATH pointing at crosstools. Would anyone want to remotely execute this rule when building with a custom local toolchain?

@htuch
Copy link
Contributor Author

htuch commented Aug 31, 2017

Closing this out based on offline discussion with @ianthehat. We have a bunch of needs (this PR, arbitrary plugins) that don't match well with the current go_proto_library rule, which is in a state of transition. The recommended approach is to fork go_proto_library.bzl in our repo (CC: @rodaine).

@htuch htuch closed this Aug 31, 2017
@htuch htuch deleted the use-default-shell branch August 31, 2017 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants