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..81d06a85f6f96f 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 @@ -81,10 +81,10 @@ default int size() { */ 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; + static EnvironmentVariables create(Map fixedVars, Set inheritedVars, + EnvironmentVariables base) { + 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..b6f619667ae0a2 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,8 @@ 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..1ea4fb4ef83f0f 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,8 @@ 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..c799fd2a478494 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"); @@ -45,12 +45,11 @@ public void compoundEnvOrdering() { @Test public void fixedInheritedInteraction() { - ActionEnvironment env = - ActionEnvironment.create( - ImmutableMap.of("FIXED_ONLY", "fixed"), ImmutableSet.of("INHERITED_ONLY")) - .addVariables( - ImmutableMap.of("FIXED_AND_INHERITED", "fixed"), - ImmutableSet.of("FIXED_AND_INHERITED")); + ActionEnvironment env = ActionEnvironment.create( + ImmutableMap.of("FIXED_ONLY", "fixed"), + ImmutableSet.of("INHERITED_ONLY")) + .withAdditionalVariables(ImmutableMap.of("FIXED_AND_INHERITED", "fixed"), + ImmutableSet.of("FIXED_AND_INHERITED")); Map clientEnv = ImmutableMap.of("INHERITED_ONLY", "inherited", "FIXED_AND_INHERITED", "inherited"); Map result = new HashMap<>(); @@ -65,4 +64,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); + } }