From 80270900b12802a566b30adb2c6ad5b76a222405 Mon Sep 17 00:00:00 2001 From: Torgil Svensson Date: Fri, 2 Dec 2022 13:47:12 +0100 Subject: [PATCH] Revert "Remove outdated "output directory name" option." This reverts commit af0e20f9d9808d8fba03b712491ad8851b3d5c26. --- .../config/BuildConfigurationValue.java | 7 +++++++ .../lib/analysis/config/CoreOptions.java | 17 ++++++++++++++++ .../config/ExecutionTransitionFactory.java | 3 +++ .../analysis/config/OutputDirectories.java | 17 +++++++++------- .../build/lib/analysis/BuildViewTest.java | 20 +++++++++++++++++++ .../skyframe/PlatformMappingFunctionTest.java | 7 +++++-- 6 files changed, 62 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 5276e0e46fdd79..4a252ce22dba14 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java @@ -35,6 +35,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.BlazeInterners; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.skyframe.BuildConfigurationKey; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -138,6 +139,12 @@ public void reportInvalidOptions(EventHandler reporter) { for (Fragment fragment : fragments.values()) { fragment.reportInvalidOptions(reporter, this.buildOptions); } + + if (options.outputDirectoryName != null) { + reporter.handle( + Event.error( + "The internal '--output directory name' option cannot be used on the command line")); + } } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 7bcd6d75fdd4d0..8079d321c02242 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -259,6 +259,23 @@ public class CoreOptions extends FragmentOptions implements Cloneable { + "'fastbuild', 'dbg', 'opt'.") public CompilationMode hostCompilationMode; + /** + * This option is used internally to set output directory name of the host configuration to + * a constant, so that the output files for the host are completely independent of those for the + * target, no matter what options are in force (k8/piii, opt/dbg, etc). + */ + @Option( + name = "output directory name", + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = { + OptionEffectTag.LOSES_INCREMENTAL_STATE, + OptionEffectTag.AFFECTS_OUTPUTS, + OptionEffectTag.LOADING_AND_ANALYSIS + }, + metadataTags = {OptionMetadataTag.INTERNAL}) + public String outputDirectoryName; + @Option( name = "experimental_enable_aspect_hints", defaultValue = "false", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java index d82bd1a84c538b..12e96b9416fe52 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java @@ -126,8 +126,11 @@ private static BuildOptions transitionImpl(BuildOptionsView options, Label execu CoreOptions coreOptions = checkNotNull(execOptions.get(CoreOptions.class)); coreOptions.isExec = true; + // Disable extra actions coreOptions.actionListeners = ImmutableList.of(); + coreOptions.actionListeners = null; + coreOptions.outputDirectoryName = null; // Then set the target to the saved execution platform if there is one. PlatformOptions platformOptions = execOptions.get(PlatformOptions.class); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java index 349b9f39a8a2bb..a4eb273e8990f5 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java @@ -109,6 +109,7 @@ public ArtifactRoot getRoot( private final BlazeDirectories directories; private final String mnemonic; + private final String outputDirName; private final ArtifactRoot outputDirectory; private final ArtifactRoot binDirectory; @@ -136,20 +137,22 @@ public ArtifactRoot getRoot( this.directories = directories; this.mnemonic = buildMnemonic(options, platformOptions, fragments, transitionDirectoryNameFragment); + this.outputDirName = + (options.outputDirectoryName != null) ? options.outputDirectoryName : mnemonic; this.outputDirectory = - OutputDirectory.OUTPUT.getRoot(mnemonic, directories, mainRepositoryName); - this.binDirectory = OutputDirectory.BIN.getRoot(mnemonic, directories, mainRepositoryName); + OutputDirectory.OUTPUT.getRoot(outputDirName, directories, mainRepositoryName); + this.binDirectory = OutputDirectory.BIN.getRoot(outputDirName, directories, mainRepositoryName); this.buildInfoDirectory = - OutputDirectory.BUILDINFO.getRoot(mnemonic, directories, mainRepositoryName); + OutputDirectory.BUILDINFO.getRoot(outputDirName, directories, mainRepositoryName); this.genfilesDirectory = - OutputDirectory.GENFILES.getRoot(mnemonic, directories, mainRepositoryName); + OutputDirectory.GENFILES.getRoot(outputDirName, directories, mainRepositoryName); this.coverageDirectory = - OutputDirectory.COVERAGE.getRoot(mnemonic, directories, mainRepositoryName); + OutputDirectory.COVERAGE.getRoot(outputDirName, directories, mainRepositoryName); this.testlogsDirectory = - OutputDirectory.TESTLOGS.getRoot(mnemonic, directories, mainRepositoryName); + OutputDirectory.TESTLOGS.getRoot(outputDirName, directories, mainRepositoryName); this.middlemanDirectory = - OutputDirectory.MIDDLEMAN.getRoot(mnemonic, directories, mainRepositoryName); + OutputDirectory.MIDDLEMAN.getRoot(outputDirName, directories, mainRepositoryName); this.mergeGenfilesDirectory = options.mergeGenfilesDirectory; this.siblingRepositoryLayout = siblingRepositoryLayout; diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java index a626462d316132..4372057e8ed83a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.FailAction; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.transitions.NoTransition; import com.google.devtools.build.lib.analysis.config.transitions.NullTransition; import com.google.devtools.build.lib.analysis.configuredtargets.InputFileConfiguredTarget; @@ -52,6 +53,8 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.NotifyingHelper.Listener; +import com.google.devtools.common.options.Options; +import com.google.devtools.common.options.OptionsParsingException; import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashSet; @@ -541,6 +544,23 @@ public void testGetDirectPrerequisiteDependencies() throws Exception { assertThat(targets).containsExactly(innerDependency, fileDependency); } + /** Tests that the {@code --output directory name} option cannot be used on the command line. */ + @Test + public void testConfigurationShortName() { + // Check that output directory name is still the name, otherwise this test is not testing what + // we expect. + CoreOptions options = Options.getDefaults(CoreOptions.class); + options.outputDirectoryName = "/home/wonkaw/wonka_chocolate/factory/out"; + assertWithMessage("The flag's name may have been changed; this test may need to be updated.") + .that(options.asMap().get("output directory name")) + .isEqualTo("/home/wonkaw/wonka_chocolate/factory/out"); + + OptionsParsingException e = + assertThrows( + OptionsParsingException.class, () -> useConfiguration("--output directory name=foo")); + assertThat(e).hasMessageThat().isEqualTo("Unrecognized option: --output directory name=foo"); + } + // Regression test: "output_filter broken (but in a different way)" @Test @Ignore("b/182560362 Starlark java_library can't output warnings") diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java index c4237ade5d591a..69a955b2dbdbc5 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java @@ -210,7 +210,7 @@ public void multiplePackagePathsFirstWins() throws Exception { assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } - // Internal flags (OptionMetadataTag.INTERNAL) cannot be set from the command-line, but + // Internal flags, such as "output directory name", cannot be set from the command-line, but // platform mapping needs to access them. @Test public void ableToChangeInternalOption() throws Exception { @@ -218,7 +218,8 @@ public void ableToChangeInternalOption() throws Exception { "my_mapping_file", "platforms:", // Force line break " //platforms:one", // Force line break - " --internal foo=something_new"); + " --internal foo=something_new", + " --output directory name=updated_output_dir"); PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); @@ -230,6 +231,8 @@ public void ableToChangeInternalOption() throws Exception { assertThat(mapped.getOptions().get(DummyTestFragment.DummyTestOptions.class).internalFoo) .isEqualTo("something_new"); + assertThat(mapped.getOptions().get(CoreOptions.class).outputDirectoryName) + .isEqualTo("updated_output_dir"); } private PlatformMappingValue executeFunction(PlatformMappingValue.Key key) throws Exception {