diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java index 11def1f91b5b9a..2ac661cd711587 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java @@ -83,8 +83,8 @@ static class CompoundEnvironmentVariables implements EnvironmentVariables { static EnvironmentVariables create( Map fixedVars, Set inheritedVars, EnvironmentVariables base) { - if (fixedVars.isEmpty() && inheritedVars.isEmpty() && base.isEmpty()) { - return EMPTY_ENVIRONMENT_VARIABLES; + if (fixedVars.isEmpty() && inheritedVars.isEmpty()) { + return base; } return new CompoundEnvironmentVariables(fixedVars, inheritedVars, base); } @@ -219,18 +219,22 @@ public static ActionEnvironment create(Map fixedEnv) { * Returns a copy of the environment with the given fixed variables added to it, overwriting * any existing occurrences of those variables. */ - public ActionEnvironment addFixedVariables(Map fixedVars) { - return ActionEnvironment.create( - CompoundEnvironmentVariables.create(fixedVars, ImmutableSet.of(), vars)); + public ActionEnvironment withAdditionalFixedVariables(Map fixedVars) { + return withAdditionalVariables(fixedVars, ImmutableSet.of()); } /** * Returns a copy of the environment with the given fixed and inherited variables added to it, * overwriting any existing occurrences of those variables. */ - public ActionEnvironment addVariables(Map fixedVars, Set inheritedVars) { - return ActionEnvironment.create( - CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars)); + public ActionEnvironment withAdditionalVariables( + Map fixedVars, Set inheritedVars) { + EnvironmentVariables newVars = + CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars); + if (newVars == vars) { + return this; + } + return ActionEnvironment.create(newVars); } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index 5825c2ab70b3fa..d7cb43efcd787b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java @@ -413,7 +413,7 @@ private TestParams createTestAction(int shards) testProperties, runfilesSupport .getActionEnvironment() - .addVariables(extraTestEnv, extraInheritedEnv), + .withAdditionalVariables(extraTestEnv, extraInheritedEnv), executionSettings, shard, run, diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index 214cc1060c7b8e..5aa6094071e305 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -205,7 +205,10 @@ public JavaCompileAction build() { toolsBuilder.addTransitive(toolsJars); ActionEnvironment actionEnvironment = - ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT); + ruleContext + .getConfiguration() + .getActionEnvironment() + .withAdditionalFixedVariables(UTF8_ENVIRONMENT); NestedSetBuilder mandatoryInputsBuilder = NestedSetBuilder.stableOrder(); mandatoryInputsBuilder diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java index 0838f8667e1c75..d36f3bd2a9e186 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java @@ -296,7 +296,10 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti } ActionEnvironment actionEnvironment = - ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT); + ruleContext + .getConfiguration() + .getActionEnvironment() + .withAdditionalFixedVariables(UTF8_ENVIRONMENT); OnDemandString progressMessage = new ProgressMessage( diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java index 54119fc4da7332..8ff56fbc18f40d 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java @@ -34,7 +34,7 @@ public void compoundEnvOrdering() { ActionEnvironment.create( ImmutableMap.of("FOO", "foo1", "BAR", "bar"), ImmutableSet.of("baz")); // entries added by env2 override the existing entries - ActionEnvironment env2 = env1.addFixedVariables(ImmutableMap.of("FOO", "foo2")); + ActionEnvironment env2 = env1.withAdditionalFixedVariables(ImmutableMap.of("FOO", "foo2")); assertThat(env1.getFixedEnv()).containsExactly("FOO", "foo1", "BAR", "bar"); assertThat(env1.getInheritedEnv()).containsExactly("baz"); @@ -48,7 +48,7 @@ public void fixedInheritedInteraction() { ActionEnvironment env = ActionEnvironment.create( ImmutableMap.of("FIXED_ONLY", "fixed"), ImmutableSet.of("INHERITED_ONLY")) - .addVariables( + .withAdditionalVariables( ImmutableMap.of("FIXED_AND_INHERITED", "fixed"), ImmutableSet.of("FIXED_AND_INHERITED")); Map clientEnv = @@ -65,4 +65,17 @@ public void fixedInheritedInteraction() { "INHERITED_ONLY", "inherited"); } + + @Test + public void emptyEnvironmentInterning() { + ActionEnvironment emptyEnvironment = + ActionEnvironment.create(ImmutableMap.of(), ImmutableSet.of()); + assertThat(emptyEnvironment).isSameInstanceAs(ActionEnvironment.EMPTY); + + ActionEnvironment base = + ActionEnvironment.create(ImmutableMap.of("FOO", "foo1"), ImmutableSet.of("baz")); + assertThat(base.withAdditionalFixedVariables(ImmutableMap.of())).isSameInstanceAs(base); + assertThat(base.withAdditionalVariables(ImmutableMap.of(), ImmutableSet.of())) + .isSameInstanceAs(base); + } }