Environment is not propagated during linking step

[blinkseb]

Description of the problem / feature request / question:

PATH, LD_LIBRARY_PATH and co are not set when executing the linking step of a C/C++ library while those variables are correctly set for the compiling steps.

We use a non-system set of GCC tools, and the linker depends on some libraries which are not found unless LD_LIBRARY_PATH is correctly set:

ldd /cvmfs/cms.cern.ch/slc6_amd64_gcc530/external/gcc/5.3.0/bin/ar
	linux-vdso.so.1 =>  (0x00007fffb9fd5000)
	libfl.so.2 => /cvmfs/cms.cern.ch/slc6_amd64_gcc530/external/gcc/5.3.0/lib/libfl.so.2 (0x00007f4fc9b33000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f4fc9899000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f4fc9695000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f4fc9300000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f4fc9d36000)

Environment info

• Operating System: Scientific Linux 6.5

• Bazel version (output of bazel info release): release 0.4.0-2016-11-17 (@a1699bd)

Anything else, information or logs or outputs that would be helpful?

Looking at how things were done for the compilation steps (in CppCompileAction.java), I've been able to craft a patch which solve the issue. If needed I can open a PR, but I'm not really sure it's the correct / optimal way to solve the issue. Anyway, here is the patch

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
index 4400474..b2555c7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
@@ -53,6 +53,7 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.util.Collection;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -95,6 +96,7 @@ public final class CppLinkAction extends AbstractAction
   private static final String LINK_GUID = "58ec78bd-1176-4e36-8143-439f656b181d";
   private static final String FAKE_LINK_GUID = "da36f819-5a15-43a9-8a45-e01b60e10c8b";
   
+  private final BuildConfiguration configuration;
   private final CppConfiguration cppConfiguration;
   private final LibraryToLink outputLibrary;
   private final Artifact linkOutput;
@@ -140,6 +142,7 @@ public final class CppLinkAction extends AbstractAction
       ActionOwner owner,
       Iterable<Artifact> inputs,
       ImmutableList<Artifact> outputs,
+      BuildConfiguration configuration,
       CppConfiguration cppConfiguration,
       LibraryToLink outputLibrary,
       Artifact linkOutput,
@@ -152,6 +155,7 @@ public final class CppLinkAction extends AbstractAction
       ImmutableSet<String> executionRequirements) {
     super(owner, inputs, outputs);
     this.mandatoryInputs = inputs;
+    this.configuration = configuration;
     this.cppConfiguration = cppConfiguration;
     this.outputLibrary = outputLibrary;
     this.linkOutput = linkOutput;
@@ -179,7 +183,7 @@ public final class CppLinkAction extends AbstractAction
 
   @Override
   public ImmutableMap<String, String> getEnvironment() {
-    ImmutableMap.Builder<String, String> result = ImmutableMap.<String, String>builder();
+    Map<String, String> result = new LinkedHashMap<>(configuration.getLocalShellEnvironment());
 
     result.putAll(toolchainEnv);
 
@@ -200,7 +204,7 @@ public final class CppLinkAction extends AbstractAction
               .getParentDirectory()
               .getPathString());
     }
-    return result.build();
+    return ImmutableMap.copyOf(result);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index f2690c6..a5d2d6a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -739,6 +739,7 @@ public class CppLinkActionBuilder {
         getOwner(),
         inputsBuilder.deduplicate().build(),
         actionOutputs,
+        configuration,
         cppConfiguration,
         outputLibrary,
         output,

[aehlig]

Description of the problem / feature request / question:

PATH, LD_LIBRARY_PATH and co are not set when executing the linking step of a C/C++ library
while those variables are correctly set for the compiling steps.

I agree that this should be changed.

Looking at how things were done for the compilation steps (in CppCompileAction.java),
I've been able to craft a patch which solve the issue. [...]

Thanks for the patch. I believe it is a good solution for half of the problem; environment
variables can be passed in two forms:

• with explicit value, e.g., --action_env=LD_LIBRARY_PATH=/some/strange/path:/yet/another/path
• implicitly referring to the invocation environment of the bazel client, e.g., --action_env=LD_LIBRARY_PATH

Your patch solves the first problem, i.e., environment variables
explicitly specified (or, during the transition period the whitelisted
variables from the server environment). For the second form, the
getClientEnvironmentVariables() method would have to be specified
(see 833fa07#diff-01090edc6d44593102eee4aaf46472a6).
Still, solving half of the problem, I think your patch is valuable and should be included;
however, I want to hear @lberki's opinion first on how this interacts with the internal
logic of the linking action that also tries to be smart about the environment.

Klaus Aehlig
Google Germany GmbH, Erika-Mann-Str. 33, 80636 Muenchen
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschaeftsfuehrer: Matthew Scott Sucherman, Paul Terence Manicle

[lberki]

@aehlig : well, LD_LIBRARY_PATH does affect the C++ link action, but the trick is that it's cached:

https://github.com/bazelbuild/bazel/blob/master/tools/cpp/cc_configure.bzl#L117

We have a plan for invalidating the C++ repository if LD_LIBRARY_PATH changes:

https://bazel.build/designs/2016/10/18/repository-invalidation.html

So it's unclear whether this change should go in. I think we should either merge change and remove the dependency on LD_LIBRARY_PATH from cc_configure.bzl or fix repository invalidation as in the design doc and not merge this change.

Since there are numerous bugs about us needing LD_LIBRARY_PATH in various actions (#1420, #1358, #1341, #1152), I think the former approach is better. @philwo , WDYT?

[lberki]

After some in-person discussion, we decided to go with the former approach. I sent out https://bazel-review.googlesource.com/#/c/7530/, which is essentially your patch above except that CppLinkAction doesn't keep a reference to BuildConfiguration in it (the less data it can access the better). If all goes well, it'll be merged pretty soon.