Repository: bazelbuild/bazel
Issue: Graceful failure on duplicate environ keys in repository_rules
Labels: type: feature request
team-ExternalDeps

### Description of the feature request:


https://github.com/bazelbuild/bazel/commit/9cd182e446f9f334930091540a58e131a9d58540#diff-5652706121f964f38ee7253331b650d61975116b50127fece8f2d30882003b61 introduces checking of duplicate keys in the environment. This fails with a stack trace if duplicates exist in the definition e.g. with `PATH` duplicated in environ I see the following backtrace:

```
(15:06:19) FATAL: bazel crashed due to an internal error. Printing stack trace:
java.lang.RuntimeException: Unrecoverable error while evaluating node 'REPOSITORY_DIRECTORY:@zenotech_cc_toolchain' (requested by nodes 'IGNORED_PACKAGE_PREFIXES:@zenotech_cc_toolchain')
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:633)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:365)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Caused by: java.lang.IllegalArgumentException: Multiple entries with same key: PATH=/home/james.sharpe/.nvm/versions/node/v18.16.0/bin:/home/james.sharpe/.local/bin:/opt/intel/oneapi/vtune/2022.0.0/bin64:/opt/intel/oneapi/vpl/2022.0.0/bin:/opt/intel/oneapi/mpi/2021.5.0//libfabric/bin:/opt/intel/oneapi/mpi/2021.5.0//bin:/opt/intel/oneapi/mkl/2022.0.1/bin/intel64:/opt/intel/oneapi/itac/2021.5.0/bin:/opt/intel/oneapi/intelpython/latest/bin:/opt/intel/oneapi/intelpython/latest/condabin:/opt/intel/oneapi/inspector/2022.0.0/bin64:/opt/intel/oneapi/dev-utilities/2021.5.1/bin:/opt/intel/oneapi/debugger/2021.5.0/gdb/intel64/bin:/opt/intel/oneapi/compiler/2022.0.1/linux/lib/oclfpga/bin:/opt/intel/oneapi/compiler/2022.0.1/linux/bin/intel64:/opt/intel/oneapi/compiler/2022.0.1/linux/bin:/opt/intel/oneapi/clck/2021.5.0/bin/intel64:/opt/intel/oneapi/advisor/2022.0.0/bin64:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/cuda/bin and PATH=/home/james.sharpe/.nvm/versions/node/v18.16.0/bin:/home/james.sharpe/.local/bin:/opt/intel/oneapi/vtune/2022.0.0/bin64:/opt/intel/oneapi/vpl/2022.0.0/bin:/opt/intel/oneapi/mpi/2021.5.0//libfabric/bin:/opt/intel/oneapi/mpi/2021.5.0//bin:/opt/intel/oneapi/mkl/2022.0.1/bin/intel64:/opt/intel/oneapi/itac/2021.5.0/bin:/opt/intel/oneapi/intelpython/latest/bin:/opt/intel/oneapi/intelpython/latest/condabin:/opt/intel/oneapi/inspector/2022.0.0/bin64:/opt/intel/oneapi/dev-utilities/2021.5.1/bin:/opt/intel/oneapi/debugger/2021.5.0/gdb/intel64/bin:/opt/intel/oneapi/compiler/2022.0.1/linux/lib/oclfpga/bin:/opt/intel/oneapi/compiler/2022.0.1/linux/bin/intel64:/opt/intel/oneapi/compiler/2022.0.1/linux/bin:/opt/intel/oneapi/clck/2021.5.0/bin/intel64:/opt/intel/oneapi/advisor/2022.0.0/bin64:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/cuda/bin
	at com.google.common.collect.ImmutableMap.conflictException(ImmutableMap.java:377)
	at com.google.common.collect.ImmutableMap.checkNoConflict(ImmutableMap.java:371)
	at com.google.common.collect.RegularImmutableMap.checkNoConflictInKeyBucket(RegularImmutableMap.java:241)
	at com.google.common.collect.RegularImmutableMap.fromEntryArrayCheckingBucketOverflow(RegularImmutableMap.java:132)
	at com.google.common.collect.RegularImmutableMap.fromEntryArray(RegularImmutableMap.java:94)
	at com.google.common.collect.ImmutableMap$Builder.build(ImmutableMap.java:573)
	at com.google.common.collect.ImmutableMap$Builder.buildOrThrow(ImmutableMap.java:601)
	at com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction.getEnvironmentView(ActionEnvironmentFunction.java:105)
	at com.google.devtools.build.lib.rules.repository.RepositoryFunction.getEnvVarValues(RepositoryFunction.java:305)
	at com.google.devtools.build.lib.rules.repository.RepositoryFunction.declareEnvironmentDependencies(RepositoryFunction.java:293)
	at com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryFunction.fetch(StarlarkRepositoryFunction.java:123)
	at com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction.fetchRepository(RepositoryDelegatorFunction.java:411)
	at com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction.compute(RepositoryDelegatorFunction.java:342)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:562)
	... 7 more
```

### Which category does this issue belong to?


Rules API

### What underlying problem are you trying to solve with this feature?


Bazel should fail gracefully when duplicate keys are present and not present the user with a stacktrace.

### Which operating system are you running Bazel on?


Ubuntu linux

### What is the output of `bazel info release`?


6.3.2

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Have you found anything relevant by searching the web?

_No response_

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
Before 6.3.2 this was not an error; Bazel would silently consume the duplicate env variables. 6.3.2 made this a failure mode which presents itself in an not so user friendly manner.
A fix for this issue has been included in [Bazel 6.4.0 RC4](https://releases.bazel.build/6.4.0/rc4/index.html). Please test out the release candidate and report any issues as soon as possible. Thanks!

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
index 748735a88288e2..4d00ea10c18a3e 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
@@ -159,7 +159,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
 
     ModuleExtension extension = ((ModuleExtension) exported);
     ImmutableMap<String, String> extensionEnvVars =
-        RepositoryFunction.getEnvVarValues(env, extension.getEnvVariables());
+        RepositoryFunction.getEnvVarValues(env, ImmutableSet.copyOf(extension.getEnvVariables()));
     if (extensionEnvVars == null) {
       return null;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
index 140697445739bb..fc2c43d1a1b276 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
@@ -285,8 +285,8 @@ public RepositoryDirectoryValue.Builder fetch(
   }
 
   @SuppressWarnings("unchecked")
-  private static Iterable<String> getEnviron(Rule rule) {
-    return (Iterable<String>) rule.getAttr("$environ");
+  private static ImmutableSet<String> getEnviron(Rule rule) {
+    return ImmutableSet.copyOf((Iterable<String>) rule.getAttr("$environ"));
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
index d6bfd2d4d0f420..934fa82e1b4ccd 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
@@ -18,6 +18,7 @@
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -71,7 +72,7 @@ public class AndroidNdkRepositoryFunction extends AndroidRepositoryFunction {
   private static final String PATH_ENV_VAR = "ANDROID_NDK_HOME";
   private static final PathFragment PLATFORMS_DIR = PathFragment.create("platforms");
 
-  private static final ImmutableList<String> PATH_ENV_VAR_AS_LIST = ImmutableList.of(PATH_ENV_VAR);
+  private static final ImmutableSet<String> PATH_ENV_VAR_AS_SET = ImmutableSet.of(PATH_ENV_VAR);
 
   private static String getDefaultCrosstool(Integer majorRevision) {
     // From NDK 17, libc++ replaces gnu-libstdc++ as the default STL.
@@ -262,7 +263,7 @@ public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Envir
     if (attributes.isAttributeValueExplicitlySpecified("path")) {
       return true;
     }
-    return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_LIST);
+    return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_SET);
   }
 
   @Override
@@ -276,7 +277,7 @@ public RepositoryDirectoryValue.Builder fetch(
       SkyKey key)
       throws InterruptedException, RepositoryFunctionException {
     Map<String, String> environ =
-        declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_LIST);
+        declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET);
     if (environ == null) {
       return null;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
index 5875814a248c8e..e52dbb6303f011 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
@@ -19,6 +19,7 @@
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
@@ -163,7 +164,7 @@ public String toString() {
   private static final PathFragment SYSTEM_IMAGES_DIR = PathFragment.create("system-images");
   private static final AndroidRevision MIN_BUILD_TOOLS_REVISION = AndroidRevision.parse("30.0.0");
   private static final String PATH_ENV_VAR = "ANDROID_HOME";
-  private static final ImmutableList<String> PATH_ENV_VAR_AS_LIST = ImmutableList.of(PATH_ENV_VAR);
+  private static final ImmutableSet<String> PATH_ENV_VAR_AS_SET = ImmutableSet.of(PATH_ENV_VAR);
   private static final ImmutableList<String> LOCAL_MAVEN_REPOSITORIES =
       ImmutableList.of(
           "extras/android/m2repository", "extras/google/m2repository", "extras/m2repository");
@@ -180,7 +181,7 @@ public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Envir
     if (attributes.isAttributeValueExplicitlySpecified("path")) {
       return true;
     }
-    return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_LIST);
+    return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_SET);
   }
 
   @Override
@@ -194,7 +195,7 @@ public RepositoryDirectoryValue.Builder fetch(
       SkyKey key)
       throws RepositoryFunctionException, InterruptedException {
     Map<String, String> environ =
-        declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_LIST);
+        declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET);
     if (environ == null) {
       return null;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
index 26d04ae28c2288..2ac13825d39ce8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -19,8 +19,8 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -55,6 +55,7 @@
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
@@ -172,11 +173,11 @@ public abstract RepositoryDirectoryValue.Builder fetch(
       throws InterruptedException, RepositoryFunctionException;
 
   @SuppressWarnings("unchecked")
-  private static Collection<String> getEnviron(Rule rule) {
+  private static ImmutableSet<String> getEnviron(Rule rule) {
     if (rule.isAttrDefined("$environ", Type.STRING_LIST)) {
-      return (Collection<String>) rule.getAttr("$environ");
+      return ImmutableSet.copyOf((Collection<String>) rule.getAttr("$environ"));
     }
-    return ImmutableList.of();
+    return ImmutableSet.of();
   }
 
   /**
@@ -288,7 +289,7 @@ public static RootedPath getRootedPathFromLabel(Label label, Environment env)
    */
   @Nullable
   protected Map<String, String> declareEnvironmentDependencies(
-      Map<String, String> markerData, Environment env, Iterable<String> keys)
+      Map<String, String> markerData, Environment env, Set<String> keys)
       throws InterruptedException {
     ImmutableMap<String, String> envDep = getEnvVarValues(env, keys);
     if (envDep == null) {
@@ -300,7 +301,7 @@ protected Map<String, String> declareEnvironmentDependencies(
   }
 
   @Nullable
-  public static ImmutableMap<String, String> getEnvVarValues(Environment env, Iterable<String> keys)
+  public static ImmutableMap<String, String> getEnvVarValues(Environment env, Set<String> keys)
       throws InterruptedException {
     ImmutableMap<String, String> environ = ActionEnvironmentFunction.getEnvironmentView(env, keys);
     if (environ == null) {
@@ -329,7 +330,7 @@ public static ImmutableMap<String, String> getEnvVarValues(Environment env, Iter
    * Environment)} function to verify the values for environment variables.
    */
   protected boolean verifyEnvironMarkerData(
-      Map<String, String> markerData, Environment env, Collection<String> keys)
+      Map<String, String> markerData, Environment env, Set<String> keys)
       throws InterruptedException {
     ImmutableMap<String, String> environ = ActionEnvironmentFunction.getEnvironmentView(env, keys);
     if (env.valuesMissing()) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
index 4edf0fb1b31078..2a8cd3f8c80963 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
@@ -14,7 +14,8 @@
 
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Interner;
 import com.google.devtools.build.lib.bugreport.BugReport;
@@ -27,6 +28,7 @@
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.SkyframeLookupResult;
 import java.util.Map;
+import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
@@ -77,13 +79,9 @@ public SkyFunctionName functionName() {
    * if and only if some dependencies from Skyframe still need to be resolved.
    */
   @Nullable
-  public static ImmutableMap<String, String> getEnvironmentView(
-      Environment env, Iterable<String> keys) throws InterruptedException {
-    ImmutableList.Builder<SkyKey> skyframeKeysBuilder = ImmutableList.builder();
-    for (String key : keys) {
-      skyframeKeysBuilder.add(key(key));
-    }
-    ImmutableList<SkyKey> skyframeKeys = skyframeKeysBuilder.build();
+  public static ImmutableMap<String, String> getEnvironmentView(Environment env, Set<String> keys)
+      throws InterruptedException {
+    var skyframeKeys = keys.stream().map(ActionEnvironmentFunction::key).collect(toImmutableSet());
     SkyframeLookupResult values = env.getValuesAndExceptions(skyframeKeys);
     if (env.valuesMissing()) {
       return null;


Test patch:
diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh
index 5569735b30d612..4ded3acfc5bb0d 100755
--- a/src/test/shell/bazel/starlark_repository_test.sh
+++ b/src/test/shell/bazel/starlark_repository_test.sh
@@ -2356,4 +2356,26 @@ EOF
   bazel build --experimental_repository_disable_download //:it || fail "Failed to build"
 }
 
+function test_duplicate_value_in_environ() {
+  cat >> WORKSPACE <<EOF
+load('//:def.bzl', 'repo')
+repo(name='foo')
+EOF
+
+  touch BUILD
+  cat > def.bzl <<'EOF'
+def _impl(repository_ctx):
+  repository_ctx.file("WORKSPACE")
+  repository_ctx.file("BUILD", """filegroup(name="bar",srcs=[])""")
+
+repo = repository_rule(
+    implementation=_impl,
+    environ=["FOO", "FOO"],
+)
+EOF
+
+  FOO=bar bazel build @foo//:bar >& $TEST_log \
+    || fail "Expected build to succeed"
+}
+
 run_suite "local repository tests"