Add use_default_shell_env = True to all ctx.actions.run_shell rules

[Flamefire]

To start subprograms, even simple bash snippets, Bazel uses an
executable process-wrapper which is potentially built using a custom
toolchain and hence requires a set up LD_LIBRARY_PATH.
Ommitting the use_default_shell_env (defaulting to false) clears the
whole environment and the binary may try to use older system libs such
as /lib64/libstdc++.so causing it to fail in case it is (much) older
than the used libstdc++ from the custom toolchain which is very common
in HPC environments.
Hence I added use_default_shell_env = True as already done in e.g.
_local_genrule_impl.

[Flamefire]

For some reason I was also seeing the environment dropped from a flatc --python invocation which happens through flatbuffer_py_library. However prior to 2.4 that was working. Checking the compile log for 2.3.1 I don't see any invocation of flatc --python so it seems it was simply not used until 2.4 and hence not compiled which explains why the faulty Bazel rule didn't cause problems before

[chsigg]

@buchgr, would you mind to take a look at this change? I fear it might not play nice with RBE.

[chsigg]

@buchgr, would you mind to take a look at this change? I fear it might not play nice with RBE.

@buchgr confirmed that this prevent RBE caching between different users, and would cause problems when the host's LD_LIBRARY_PATH doesn't fit the remote environment.

Is there any chance you could ldconfig to work around this in your environment?

[gbaned]

@Flamefire Can you please check @chsigg's comments and keep us posted ? Thanks!

[Flamefire]

@gbaned If you are referring to

Is there any chance you could ldconfig to work around this in your environment?

No, this is not possible. Usual HPC environments have multiple versions of a software installed and decide via environment variables which is active (in particular LD_LIBRARY_PATH and PATH for binaries, CPATH & LIBRARY_PATH for dependencies)

I'm unsure why this is now a problem to add this as e.g. _local_genrule_impl already uses this.

Unfortunately I don't understand enough about Bazel and especially RBE but for us sysadmins it is a real pain that the environment gets cleared on multiple levels

[gbaned]

@chsigg Can you please take a look on above comments from @Flamefire. Thanks!

[chsigg]

Another idea, would --action_env or --host_action_env maybe do what you need?

[Flamefire]

@chsigg No this does not work. This came up in a recent Bazel issue (bazelbuild/bazel#12579 (comment)) where it was observed that action_env and such are seemingly not passed when use_default_shell_env=True is not set.
I haven't tried --host_action_env yet (it is very new) but I assume --action_env already sets those, especially as we use --distinct_host_configuration=false already. It also seems like there is an as-of-yet unreleased bugfix to the host_action_env flag: bazelbuild/bazel@e667082

[gbaned]

@Flamefire Can you please resolve conflicts? Thanks!

[chsigg]

To be honest, I don't know how we can resolve this. @buchgr, do you have any suggestions?

There is a need to properly pass environment variables to build actions, but bazel sometimes drops them (due to bugs, IIUC). The change here would work around those bugs, but it doesn't play well with RBE.

[Flamefire]

There is a need to properly pass environment variables to build actions, but bazel sometimes drops them (due to bugs, IIUC). The change here would work around those bugs, but it doesn't play well with RBE.

IIUC the use_default_shell_env=True makes Bazel pass the --action_env variables and PATH/LD_LIBRARY_PATH while without (=False) it will pass a cleaned up environment with nothing in it.
So this "sometimes" is the case for use_default_shell_env=False, which seems to me like the design, not a bug.
Having said that: There certainly are bugs, some of which I reported and some seem to be "unfixable" (Bazel contains a binary which requires LD_LIBRARY_PATH itself, "fixed" by building Bazel statically)

In the case of flatc (one of the fixed things here) a binary is called which has dependencies, so LD_LIBRARY_PATH must be passed. And as Bazel doesn't seem to have anything else but use_default_shell_env=True to do so, it should be used.
It could be checked what effect --distinct_host_configuration=true (default) has here in respect to RBE, maybe it already means that the hosts LD_LIBRARY_PATH isn't passed to the RBE even with use_default_shell_env=True? I'd expect so but am not using RBE.

Anyway, 3 strong arguments in favor of this change:

• It is only the flatbuffers compilation i.e. a code generation which is likely fast, so even a potential cache miss isn't expensive
• Short of patching the source there is no other way (AFAIK) by now to run a flatc build with a different (LD_)LIBRARY_PATH (which most likely happens when it is modified at all)
• The same is already used for protoc, where it is seemingly ok. So why the discussion here?

[gbaned]

@Flamefire Can you please resolve conflicts? Thanks!

[Flamefire]

@gbaned Rebased

[Flamefire]

Had to close+reopen due to Github not detecting the change...