add a key to ctx.action dict to prevent protoc losing the default env

[yliu120]

This is a small change on the protobuf.bzl but very reasonable and useful.

In practice, a few protobuf client (like tensorflow, see below and etc) would call _proto_gen_impl(ctx) indirectly to generate proto implementations. However by the current definition of bazel's ctx.action rule (http://www.bazel.io/versions/master/docs/skylark/lib/ctx.html#action), the default user's custom shell env (PATH/LD_LIBRARY_PATH) is not passed to the compiled protoc binary because the user_default_shell_env is set to be False as default. This will invoke link error cuz users may link protoc with a customized library. There are a lot of reports on this issue.

In principle, when calling _proto_gen_impl(ctx) to generate proto impls, we should pass user's env to the protoc binary.

Another small change is to delete an extra space. (a bonus created by my vim editor).

Please see some issues related to this small CL:
tensorflow/tensorflow#3261

This change will not affect any other reasonable implementations.

[grpc-kokoro]

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

[grpc-kokoro]

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

[bazel-io]

Can one of the admins verify this patch?

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[yliu120]

I signed it!

[googlebot]

CLAs look good, thanks!

[mitar]

This really solves a problem compiling protobuf with tensorflow with environment variables.

[htuch]

+1. Can we get a merge on this? This is very useful when building with non-default toolchains and setting LD_LIBRARY_PATH.

[xfxyjwf]

@cgrushko Can you help review this PR?

[xfxyjwf]

@cgrushko The change seems good to me, but given that default value of that flag is False, I'm wondering if there are concerns about making it True. Maybe bazel team has other recommended solutions for overriding CC/LD_LIBRARY_PATH?

[yliu120]

@xfxyjwf @cgrushko Thanks for LGTM and the comment. I guess Bazel team made that flag False as default just because Bazel usually will let the user define a customized compiler toolchain in a crosstool file, so it means Bazel could setup an internal version of customized environment and always pass them to the binary command right before it (it doesn't export it to create global side effects). When this is true, then it will certainly not use the default shell 's environment.

However, the case here is:
Some package depends on protobuf. Like tensorflow or other protobuf clients, when users compile their code from source, Bazel will pull protobuf down first and compile protobuf using a user-customized environment. It will create a protoc binary and then use that binary to compile .proto source files. Bazel will use the gen_rule defined here to compile those proto files. Bazel may have no responsibility for passing its internal customized envs to proto gen rules since they have already been defined in this file. (I may not be correct). So for not dropping those envs, we really should add the customized env back.

[htuch]

@yliu120 is correctly interpreting the use of this attribute - it's intended to allow the user to override. Bazel accepts CC/LD_LIBRARY_PATH and autogenerates a toolchain when doing so, see https://www.bazel.build/blog/2016/03/31/autoconfiguration.html. This avoids the extremely heavyweight notion of writing a CROSSTOOL everytime you want to point the compiler at a non-standard location.

@xfxyjwf Can we merge this? It's something we'd like for the Envoy Bazel build, to allow it to be used internally in Google where we point CC at crosstools when using the OSS version from https://github.com/lyft/envoy/.