From 8f056805b41172821d8fb81f76a40c2eaf467036 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 14 Jun 2024 07:36:35 -0700 Subject: [PATCH] Push `makeExecutable` down into `AbstractFileWriteAction` subclasses Getting rid of the single boolean field on `AbstractFileWriteAction` reduces padding on each subclass instance and in particular frees up a 4-byte field on `CppModuleMapAction`. Also use a lambda to define `newDeterministicWriter` if possible for improved readability. This prepares for future changes that will add fields to `CppModuleMapAction` to support path mapping. Work towards #6526 Closes #22609. PiperOrigin-RevId: 643340715 Change-Id: Id3af26049098e6dfa731f0e7a1be6709bea0d9f2 --- .../analysis/RepoMappingManifestAction.java | 3 +- .../lib/analysis/SourceManifestAction.java | 2 +- .../actions/AbstractFileWriteAction.java | 14 +- .../actions/BinaryFileWriteAction.java | 23 +- .../lib/analysis/actions/FileWriteAction.java | 14 +- .../actions/LauncherFileWriteAction.java | 8 +- .../LazyWriteNestedSetOfTupleAction.java | 12 +- .../actions/LazyWritePathsFileAction.java | 11 +- .../actions/ParameterFileWriteAction.java | 5 +- .../extra/ExtraActionInfoFileWriteAction.java | 3 +- .../analysis/test/BaselineCoverageAction.java | 19 +- .../test/InstrumentedFileManifestAction.java | 29 +-- .../android/AndroidDeployInfoAction.java | 6 +- .../lib/rules/android/WriteAdbArgsAction.java | 2 +- .../lib/rules/cpp/CppModuleMapAction.java | 205 +++++++++--------- .../lib/rules/cpp/UmbrellaHeaderAction.java | 26 +-- .../rules/cpp/WriteBuildInfoHeaderAction.java | 2 +- .../build/lib/rules/genquery/GenQuery.java | 3 +- .../java/WriteBuildInfoPropertiesAction.java | 2 +- .../build/lib/rules/python/PyBuiltins.java | 6 +- .../actions/AbstractFileWriteActionTest.java | 12 +- 21 files changed, 187 insertions(+), 220 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java index 1f4e11fd44a9b8..fdfb6927b83839 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java @@ -105,8 +105,7 @@ public RepoMappingManifestAction( NestedSet runfilesSymlinks, NestedSet runfilesRootSymlinks, String workspaceName) { - super( - owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, /* makeExecutable= */ false); + super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output); this.transitivePackages = transitivePackages; this.runfilesArtifacts = runfilesArtifacts; this.hasRunfilesSymlinks = !runfilesSymlinks.isEmpty(); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java index 61dd5fc1da0be3..a84bf4a1566fc0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java @@ -143,7 +143,7 @@ public SourceManifestAction( @Nullable Artifact repoMappingManifest, boolean remotableSourceManifestActions) { // The real set of inputs is computed in #getInputs(). - super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), primaryOutput, false); + super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), primaryOutput); this.manifestWriter = manifestWriter; this.runfiles = runfiles; this.repoMappingManifest = repoMappingManifest; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java index a6bf082c4df556..42088c47e8450e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java @@ -33,26 +33,20 @@ * Abstract Action to write to a file. */ public abstract class AbstractFileWriteAction extends AbstractAction { - - protected final boolean makeExecutable; - /** * Creates a new AbstractFileWriteAction instance. * * @param owner the action owner. * @param inputs the Artifacts that this Action depends on * @param output the Artifact that will be created by executing this Action. - * @param makeExecutable iff true will change the output file to be executable. */ - public AbstractFileWriteAction( - ActionOwner owner, NestedSet inputs, Artifact output, boolean makeExecutable) { + public AbstractFileWriteAction(ActionOwner owner, NestedSet inputs, Artifact output) { // There is only one output, and it is primary. super(owner, inputs, ImmutableSet.of(output)); - this.makeExecutable = makeExecutable; } public boolean makeExecutable() { - return makeExecutable; + return false; } @Override @@ -64,7 +58,7 @@ public final ActionResult execute(ActionExecutionContext actionExecutionContext) actionExecutionContext.getContext(FileWriteActionContext.class); ImmutableList result = context.writeOutputToFile( - this, actionExecutionContext, deterministicWriter, makeExecutable, isRemotable()); + this, actionExecutionContext, deterministicWriter, makeExecutable(), isRemotable()); afterWrite(actionExecutionContext); return ActionResult.create(result); } catch (ExecException e) { @@ -95,7 +89,7 @@ public String getMnemonic() { @Override protected String getRawProgressMessage() { - return (makeExecutable ? "Writing script " : "Writing file ") + return (makeExecutable() ? "Writing script " : "Writing file ") + Iterables.getOnlyElement(getOutputs()).prettyPrint(); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/BinaryFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/BinaryFileWriteAction.java index cfe01f5018ec54..435438ce88379b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/BinaryFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/BinaryFileWriteAction.java @@ -29,7 +29,6 @@ import com.google.devtools.build.lib.util.Fingerprint; import java.io.IOException; import java.io.InputStream; -import java.io.OutputStream; import javax.annotation.Nullable; /** @@ -41,6 +40,7 @@ public final class BinaryFileWriteAction extends AbstractFileWriteAction { private static final String GUID = "eeee07fe-4b40-11e4-82d6-eba0b4f713e2"; private final ByteSource source; + private final boolean makeExecutable; /** * Creates a new BinaryFileWriteAction instance without inputs. @@ -52,8 +52,14 @@ public final class BinaryFileWriteAction extends AbstractFileWriteAction { */ public BinaryFileWriteAction( ActionOwner owner, Artifact output, ByteSource source, boolean makeExecutable) { - super(owner, /*inputs=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, makeExecutable); + super(owner, /* inputs= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), output); this.source = Preconditions.checkNotNull(source); + this.makeExecutable = makeExecutable; + } + + @Override + public boolean makeExecutable() { + return makeExecutable; } @VisibleForTesting @@ -63,14 +69,11 @@ public ByteSource getSource() { @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { - return new DeterministicWriter() { - @Override - public void writeOutputFile(OutputStream out) throws IOException { - try (InputStream in = source.openStream()) { - ByteStreams.copy(in, out); - } - out.flush(); + return out -> { + try (InputStream in = source.openStream()) { + ByteStreams.copy(in, out); } + out.flush(); }; } @@ -80,7 +83,7 @@ protected void computeKey( @Nullable ArtifactExpander artifactExpander, Fingerprint fp) { fp.addString(GUID); - fp.addString(String.valueOf(makeExecutable)); + fp.addBoolean(makeExecutable()); try (InputStream in = source.openStream()) { byte[] buffer = new byte[512]; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteAction.java index 08c6f51380765f..96d092d8a965bd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteAction.java @@ -69,6 +69,8 @@ public abstract class FileWriteAction extends AbstractFileWriteAction /** Minimum length (in chars) for content to be eligible for compression. */ private static final int COMPRESS_CHARS_THRESHOLD = 256; + private final boolean makeExecutable; + /** * Creates a FileWriteAction to write contents to the resulting artifact fileName in the genfiles * root underneath the package path. @@ -169,7 +171,8 @@ private FileWriteAction( NestedSet inputs, Artifact primaryOutput, boolean makeExecutable) { - super(owner, inputs, primaryOutput, makeExecutable); + super(owner, inputs, primaryOutput); + this.makeExecutable = makeExecutable; } @Override @@ -177,6 +180,11 @@ public final String getFileContents(@Nullable EventHandler eventHandler) { return getFileContents(); } + @Override + public boolean makeExecutable() { + return makeExecutable; + } + /** * Returns the string contents to be written. * @@ -225,7 +233,7 @@ protected void computeKey( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander, Fingerprint fp) { - fp.addString(GUID).addBoolean(makeExecutable).addString(getFileContents()); + fp.addString(GUID).addBoolean(makeExecutable()).addString(getFileContents()); } } @@ -305,7 +313,7 @@ protected void computeKey( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander, Fingerprint fp) { - fp.addString(GUID).addBoolean(makeExecutable).addBytes(compressedBytes); + fp.addString(GUID).addBoolean(makeExecutable()).addBytes(compressedBytes); } } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/LauncherFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/LauncherFileWriteAction.java index a5ffc66919a33d..e100c7ff9c9e12 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/LauncherFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/LauncherFileWriteAction.java @@ -62,13 +62,17 @@ private LauncherFileWriteAction( super( ruleContext.getActionOwner(), NestedSetBuilder.create(Order.STABLE_ORDER, Preconditions.checkNotNull(launcher)), - output, - /* makeExecutable= */ true); + output); this.launcher = launcher; // already null-checked in the superclass c'tor this.launchInfo = Preconditions.checkNotNull(launchInfo); this.isExecutedOnWindows = ruleContext.isExecutedOnWindows(); } + @Override + public boolean makeExecutable() { + return true; + } + @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { // TODO(tjgq): Move this check into createAndRegister. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWriteNestedSetOfTupleAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWriteNestedSetOfTupleAction.java index bd8da23fb2c8a3..66d4f38cf70d6c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWriteNestedSetOfTupleAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWriteNestedSetOfTupleAction.java @@ -26,8 +26,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.util.Fingerprint; -import java.io.IOException; -import java.io.OutputStream; import javax.annotation.Nullable; import net.starlark.java.eval.Tuple; @@ -44,20 +42,14 @@ public final class LazyWriteNestedSetOfTupleAction extends AbstractFileWriteActi public LazyWriteNestedSetOfTupleAction( ActionOwner owner, Artifact output, NestedSet tuplesToWrite, String delimiter) { - super( - owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, /* makeExecutable= */ false); + super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output); this.tuplesToWrite = tuplesToWrite; this.delimiter = delimiter; } @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { - return new DeterministicWriter() { - @Override - public void writeOutputFile(OutputStream out) throws IOException { - out.write(getContents(delimiter).getBytes(UTF_8)); - } - }; + return out -> out.write(getContents(delimiter).getBytes(UTF_8)); } /** Computes the Action key for this action by computing the fingerprint for the file contents. */ diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java index d37dde8014438c..ed3ef808088def 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java @@ -27,8 +27,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.util.Fingerprint; -import java.io.IOException; -import java.io.OutputStream; import java.util.function.Function; import javax.annotation.Nullable; @@ -64,7 +62,7 @@ public LazyWritePathsFileAction( Function converter) { // We don't need to pass the given files as explicit inputs to this action; we don't care about // them, we only need their names, which we already know. - super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, false); + super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output); this.files = files; this.includeDerivedArtifacts = includeDerivedArtifacts; this.filesToIgnore = filesToIgnore; @@ -73,12 +71,7 @@ public LazyWritePathsFileAction( @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { - return new DeterministicWriter() { - @Override - public void writeOutputFile(OutputStream out) throws IOException { - out.write(getContents().toString().getBytes(UTF_8)); - } - }; + return out -> out.write(getContents().getBytes(UTF_8)); } /** Computes the Action key for this action by computing the fingerprint for the file contents. */ diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java index ecaf30879d4516..0df56ee4a3b5a1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java @@ -86,7 +86,7 @@ public ParameterFileWriteAction( Artifact output, CommandLine commandLine, ParameterFileType type) { - super(owner, inputs, output, false); + super(owner, inputs, output); this.commandLine = commandLine; this.type = type; this.hasInputArtifactToExpand = !inputs.isEmpty(); @@ -174,7 +174,6 @@ protected void computeKey( Fingerprint fp) throws CommandLineExpansionException, InterruptedException { fp.addString(GUID); - fp.addString(String.valueOf(makeExecutable)); fp.addString(type.toString()); commandLine.addToFingerprint(actionKeyContext, artifactExpander, fp); } @@ -184,8 +183,6 @@ public String describeKey() { StringBuilder message = new StringBuilder(); message.append("GUID: "); message.append(GUID); - message.append("\nExecutable: "); - message.append(makeExecutable); message.append("\nParam File Type: "); message.append(type); message.append("\nContent digest (approximate): "); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraActionInfoFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraActionInfoFileWriteAction.java index 09c171d48ed8dd..00f8fc845af53e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraActionInfoFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraActionInfoFileWriteAction.java @@ -53,8 +53,7 @@ public final class ExtraActionInfoFileWriteAction extends AbstractFileWriteActio shadowedAction.discoversInputs() ? NestedSetBuilder.stableOrder().addAll(shadowedAction.getOutputs()).build() : NestedSetBuilder.emptySet(Order.STABLE_ORDER), - primaryOutput, - /*makeExecutable=*/ false); + primaryOutput); this.shadowedAction = Preconditions.checkNotNull(shadowedAction, primaryOutput); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java index 2db38d69dbda34..d09c6910028db4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java @@ -33,8 +33,6 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.PathFragment; -import java.io.IOException; -import java.io.OutputStream; import java.io.PrintWriter; import javax.annotation.Nullable; @@ -47,7 +45,7 @@ public final class BaselineCoverageAction extends AbstractFileWriteAction private BaselineCoverageAction( ActionOwner owner, NestedSet instrumentedFiles, Artifact primaryOutput) { - super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), primaryOutput, false); + super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), primaryOutput); this.instrumentedFiles = instrumentedFiles; } @@ -68,16 +66,13 @@ public void computeKey( @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { - return new DeterministicWriter() { - @Override - public void writeOutputFile(OutputStream out) throws IOException { - PrintWriter writer = new PrintWriter(out); - for (Artifact file : instrumentedFiles.toList()) { - writer.write("SF:" + file.getExecPathString() + "\n"); - writer.write("end_of_record\n"); - } - writer.flush(); + return out -> { + PrintWriter writer = new PrintWriter(out); + for (Artifact file : instrumentedFiles.toList()) { + writer.write("SF:" + file.getExecPathString() + "\n"); + writer.write("end_of_record\n"); } + writer.flush(); }; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFileManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFileManifestAction.java index 900dbadf66ea4c..b68a7b66f3143f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFileManifestAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFileManifestAction.java @@ -30,8 +30,6 @@ import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.util.Fingerprint; -import java.io.IOException; -import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.Writer; import java.util.Arrays; @@ -48,28 +46,21 @@ final class InstrumentedFileManifestAction extends AbstractFileWriteAction { @VisibleForTesting InstrumentedFileManifestAction(ActionOwner owner, NestedSet files, Artifact output) { - super( - owner, - /*inputs=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), - output, - /*makeExecutable=*/ false); + super(owner, /* inputs= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), output); this.files = files; } @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { - return new DeterministicWriter() { - @Override - public void writeOutputFile(OutputStream out) throws IOException { - // Sort the exec paths before writing them out. - String[] fileNames = - files.toList().stream().map(Artifact::getExecPathString).toArray(String[]::new); - Arrays.sort(fileNames); - try (Writer writer = new OutputStreamWriter(out, ISO_8859_1)) { - for (String name : fileNames) { - writer.write(name); - writer.write('\n'); - } + return out -> { + // Sort the exec paths before writing them out. + String[] fileNames = + files.toList().stream().map(Artifact::getExecPathString).toArray(String[]::new); + Arrays.sort(fileNames); + try (Writer writer = new OutputStreamWriter(out, ISO_8859_1)) { + for (String name : fileNames) { + writer.write(name); + writer.write('\n'); } } }; diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDeployInfoAction.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDeployInfoAction.java index 7bacbb24d2fc23..7929e4fbc9fc3f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDeployInfoAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDeployInfoAction.java @@ -67,11 +67,7 @@ private static NestedSet makeInputs( Artifact mergedManifest, ImmutableList additionalMergedManifests, ImmutableList apksToDeploy) { - super( - owner, - makeInputs(mergedManifest, additionalMergedManifests, apksToDeploy), - outputFile, - false); + super(owner, makeInputs(mergedManifest, additionalMergedManifests, apksToDeploy), outputFile); this.mergedManifest = mergedManifest; this.additionalMergedManifests = additionalMergedManifests; this.apksToDeploy = apksToDeploy; diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/WriteAdbArgsAction.java b/src/main/java/com/google/devtools/build/lib/rules/android/WriteAdbArgsAction.java index 8d4ca187891137..1c787d75b0db8f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/WriteAdbArgsAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/WriteAdbArgsAction.java @@ -118,7 +118,7 @@ public static final class Options extends OptionsBase { } public WriteAdbArgsAction(ActionOwner owner, Artifact outputFile) { - super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), outputFile, false); + super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), outputFile); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java index 33edc348420dc8..9b6edb86d8d125 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java @@ -29,7 +29,6 @@ import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; -import java.io.OutputStream; import java.io.OutputStreamWriter; import java.nio.charset.StandardCharsets; import java.util.ArrayList; @@ -85,12 +84,7 @@ public CppModuleMapAction( .addAll(Iterables.filter(privateHeaders, Artifact::isTreeArtifact)) .addAll(Iterables.filter(publicHeaders, Artifact::isTreeArtifact)) .build(), - cppModuleMap.getArtifact(), - // In theory, module maps should not be executable but, in practice, we don't care. As - // 'executable' is the default (see - // ActionOutputMetadataStore.setPathReadOnlyAndExecutable()), - // we want to avoid the extra file operation of making this file non-executable. - /* makeExecutable= */ true); + cppModuleMap.getArtifact()); this.cppModuleMap = cppModuleMap; this.moduleMapHomeIsCwd = moduleMapHomeIsCwd; this.privateHeaders = ImmutableList.copyOf(privateHeaders); @@ -104,117 +98,124 @@ public CppModuleMapAction( } @Override - public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { + public boolean makeExecutable() { + // In theory, module maps should not be executable but, in practice, we don't care. As + // 'executable' is the default (see ActionOutputMetadataStore.setPathReadOnlyAndExecutable()), + // we want to avoid the extra file operation of making this file non-executable. + // Note that the opposite is true for Bazel: making a file executable results in an extra file + // operation in com.google.devtools.build.lib.exec.FileWriteStrategy. + return true; + } + + @Override + public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { final ArtifactExpander artifactExpander = ctx.getArtifactExpander(); - return new DeterministicWriter() { - @Override - public void writeOutputFile(OutputStream out) throws IOException { - OutputStreamWriter content = new OutputStreamWriter(out, StandardCharsets.ISO_8859_1); - PathFragment fragment = cppModuleMap.getArtifact().getExecPath(); - int segmentsToExecPath = fragment.segmentCount() - 1; - Optional umbrellaHeader = cppModuleMap.getUmbrellaHeader(); - String leadingPeriods = moduleMapHomeIsCwd ? "" : "../".repeat(segmentsToExecPath); + return out -> { + OutputStreamWriter content = new OutputStreamWriter(out, StandardCharsets.ISO_8859_1); + PathFragment fragment = cppModuleMap.getArtifact().getExecPath(); + int segmentsToExecPath = fragment.segmentCount() - 1; + Optional umbrellaHeader = cppModuleMap.getUmbrellaHeader(); + String leadingPeriods = moduleMapHomeIsCwd ? "" : "../".repeat(segmentsToExecPath); - Iterable separateModuleHdrs = - expandedHeaders(artifactExpander, separateModuleHeaders); + Iterable separateModuleHdrs = + expandedHeaders(artifactExpander, separateModuleHeaders); - // For details about the different header types, see: - // http://clang.llvm.org/docs/Modules.html#header-declaration - content.append("module \"").append(cppModuleMap.getName()).append("\" {\n"); - content.append(" export *\n"); + // For details about the different header types, see: + // http://clang.llvm.org/docs/Modules.html#header-declaration + content.append("module \"").append(cppModuleMap.getName()).append("\" {\n"); + content.append(" export *\n"); - HashSet deduper = new HashSet<>(); - if (umbrellaHeader.isPresent()) { + HashSet deduper = new HashSet<>(); + if (umbrellaHeader.isPresent()) { + appendHeader( + content, + "", + umbrellaHeader.get().getExecPath(), + leadingPeriods, + /* canCompile= */ false, + deduper, + /*isUmbrellaHeader*/ true); + } else { + for (Artifact artifact : expandedHeaders(artifactExpander, publicHeaders)) { appendHeader( content, "", - umbrellaHeader.get().getExecPath(), + artifact.getExecPath(), leadingPeriods, - /*canCompile=*/ false, + /* canCompile= */ true, deduper, - /*isUmbrellaHeader*/ true); - } else { - for (Artifact artifact : expandedHeaders(artifactExpander, publicHeaders)) { - appendHeader( - content, - "", - artifact.getExecPath(), - leadingPeriods, - /*canCompile=*/ true, - deduper, - /*isUmbrellaHeader*/ false); - } - for (Artifact artifact : expandedHeaders(artifactExpander, privateHeaders)) { - appendHeader( - content, - "private", - artifact.getExecPath(), - leadingPeriods, - /*canCompile=*/ true, - deduper, - /*isUmbrellaHeader*/ false); - } - for (Artifact artifact : separateModuleHdrs) { - appendHeader( - content, - "", - artifact.getExecPath(), - leadingPeriods, - /*canCompile=*/ false, - deduper, - /*isUmbrellaHeader*/ false); - } - for (PathFragment additionalExportedHeader : additionalExportedHeaders) { - appendHeader( - content, - "", - additionalExportedHeader, - leadingPeriods, - /*canCompile*/ false, - deduper, - /*isUmbrellaHeader*/ false); - } + /*isUmbrellaHeader*/ false); } - for (CppModuleMap dep : dependencies) { - content.append(" use \"").append(dep.getName()).append("\"\n"); + for (Artifact artifact : expandedHeaders(artifactExpander, privateHeaders)) { + appendHeader( + content, + "private", + artifact.getExecPath(), + leadingPeriods, + /* canCompile= */ true, + deduper, + /*isUmbrellaHeader*/ false); + } + for (Artifact artifact : separateModuleHdrs) { + appendHeader( + content, + "", + artifact.getExecPath(), + leadingPeriods, + /* canCompile= */ false, + deduper, + /*isUmbrellaHeader*/ false); + } + for (PathFragment additionalExportedHeader : additionalExportedHeaders) { + appendHeader( + content, + "", + additionalExportedHeader, + leadingPeriods, + /*canCompile*/ false, + deduper, + /*isUmbrellaHeader*/ false); } + } + for (CppModuleMap dep : dependencies) { + content.append(" use \"").append(dep.getName()).append("\"\n"); + } - if (!Iterables.isEmpty(separateModuleHdrs)) { - String separateName = cppModuleMap.getName() + CppModuleMap.SEPARATE_MODULE_SUFFIX; - content.append(" use \"").append(separateName).append("\"\n"); - content.append("}\n"); - content.append("module \"").append(separateName).append("\" {\n"); - content.append(" export *\n"); - deduper = new HashSet<>(); - for (Artifact artifact : separateModuleHdrs) { - appendHeader( - content, - "", - artifact.getExecPath(), - leadingPeriods, - /*canCompile=*/ true, - deduper, - /*isUmbrellaHeader*/ false); - } - for (CppModuleMap dep : dependencies) { - content.append(" use \"").append(dep.getName()).append("\"\n"); - } + if (!Iterables.isEmpty(separateModuleHdrs)) { + String separateName = cppModuleMap.getName() + CppModuleMap.SEPARATE_MODULE_SUFFIX; + content.append(" use \"").append(separateName).append("\"\n"); + content.append("}\n"); + content.append("module \"").append(separateName).append("\" {\n"); + content.append(" export *\n"); + deduper = new HashSet<>(); + for (Artifact artifact : separateModuleHdrs) { + appendHeader( + content, + "", + artifact.getExecPath(), + leadingPeriods, + /* canCompile= */ true, + deduper, + /*isUmbrellaHeader*/ false); } - content.append("}"); + for (CppModuleMap dep : dependencies) { + content.append(" use \"").append(dep.getName()).append("\"\n"); + } + } + content.append("}"); - if (externDependencies) { - for (CppModuleMap dep : dependencies) { - content - .append("\nextern module \"") - .append(dep.getName()) - .append("\" \"") - .append(leadingPeriods) - .append(dep.getArtifact().getExecPathString()) - .append("\""); - } + if (externDependencies) { + for (CppModuleMap dep : dependencies) { + content + .append("\nextern module \"") + .append(dep.getName()) + .append("\" \"") + .append(leadingPeriods) + .append(dep.getArtifact().getExecPathString()) + .append("\""); } - content.flush(); } + content.flush(); }; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java index 950b0e87e93280..5e67c393b68f45 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java @@ -27,8 +27,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.PathFragment; -import java.io.IOException; -import java.io.OutputStream; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.HashSet; @@ -58,8 +56,7 @@ public UmbrellaHeaderAction( NestedSetBuilder.stableOrder() .addAll(Iterables.filter(publicHeaders, Artifact::isTreeArtifact)) .build(), - umbrellaHeader, - /*makeExecutable=*/ false); + umbrellaHeader); this.umbrellaHeader = umbrellaHeader; this.publicHeaders = ImmutableList.copyOf(publicHeaders); this.additionalExportedHeaders = ImmutableList.copyOf(additionalExportedHeaders); @@ -68,19 +65,16 @@ public UmbrellaHeaderAction( @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { final ArtifactExpander artifactExpander = ctx.getArtifactExpander(); - return new DeterministicWriter() { - @Override - public void writeOutputFile(OutputStream out) throws IOException { - StringBuilder content = new StringBuilder(); - HashSet deduper = new HashSet<>(); - for (Artifact artifact : expandedHeaders(artifactExpander, publicHeaders)) { - appendHeader(content, artifact.getExecPath(), deduper); - } - for (PathFragment additionalExportedHeader : additionalExportedHeaders) { - appendHeader(content, additionalExportedHeader, deduper); - } - out.write(content.toString().getBytes(StandardCharsets.ISO_8859_1)); + return out -> { + StringBuilder content = new StringBuilder(); + HashSet deduper = new HashSet<>(); + for (Artifact artifact : expandedHeaders(artifactExpander, publicHeaders)) { + appendHeader(content, artifact.getExecPath(), deduper); } + for (PathFragment additionalExportedHeader : additionalExportedHeaders) { + appendHeader(content, additionalExportedHeader, deduper); + } + out.write(content.toString().getBytes(StandardCharsets.ISO_8859_1)); }; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderAction.java index f7078a6d6b5791..e55331b1d05d3c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderAction.java @@ -73,7 +73,7 @@ public WriteBuildInfoHeaderAction( Artifact primaryOutput, boolean writeVolatileInfo, boolean writeStableInfo) { - super(ActionOwner.SYSTEM_ACTION_OWNER, inputs, primaryOutput, /*makeExecutable=*/ false); + super(ActionOwner.SYSTEM_ACTION_OWNER, inputs, primaryOutput); if (!inputs.isEmpty()) { // With non-empty inputs we should not generate both volatile and non-volatile data // in the same header file. diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java index 78b1c8b6012d68..c5d621b951ebd5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java @@ -379,8 +379,7 @@ private static final class QueryResultAction extends AbstractFileWriteAction { private final GenQueryResult result; private QueryResultAction(ActionOwner owner, Artifact output, GenQueryResult result) { - super( - owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, /*makeExecutable=*/ false); + super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output); this.result = result; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java index c13b6aec7262be..e92ab226ed87c4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java @@ -153,7 +153,7 @@ public WriteBuildInfoPropertiesAction( boolean includeVolatile, boolean includeNonVolatile, TimestampFormatter timestampFormatter) { - super(ActionOwner.SYSTEM_ACTION_OWNER, inputs, primaryOutput, /* makeExecutable= */ false); + super(ActionOwner.SYSTEM_ACTION_OWNER, inputs, primaryOutput); this.keyTranslations = keyTranslations; this.includeVolatile = includeVolatile; this.includeNonVolatile = includeNonVolatile; diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java index 7434d644147c4f..dfc9ba379d3b9d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java @@ -444,11 +444,7 @@ static final class CopyWithoutCachingAction extends AbstractFileWriteAction { private static final String GUID = "67513fa7-3824-493b-aeab-95a8b778ea07"; CopyWithoutCachingAction(ActionOwner owner, Artifact readFrom, Artifact writeTo) { - super( - owner, - NestedSetBuilder.create(Order.STABLE_ORDER, readFrom), - writeTo, - /* makeExecutable= */ false); + super(owner, NestedSetBuilder.create(Order.STABLE_ORDER, readFrom), writeTo); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteActionTest.java index d3dec8e49e2d0c..a2526e061e7f03 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteActionTest.java @@ -108,18 +108,24 @@ ActionExecutionContext createMockActionExecutionContext(FileWriteActionContext f private static class TestFileWriteAction extends AbstractFileWriteAction { private final DeterministicWriter deterministicWriter; private final boolean isRemotable; + private final boolean makeExecutable; TestFileWriteAction( DeterministicWriter deterministicWriter, boolean executable, boolean isRemotable) { super( ActionsTestUtil.NULL_ACTION_OWNER, - /*inputs=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), - /*output=*/ ActionsTestUtil.DUMMY_ARTIFACT, - executable); + /* inputs= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /* output= */ ActionsTestUtil.DUMMY_ARTIFACT); this.deterministicWriter = deterministicWriter; + this.makeExecutable = executable; this.isRemotable = isRemotable; } + @Override + public boolean makeExecutable() { + return makeExecutable; + } + @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { return deterministicWriter;