diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index ea010f7a542104..ee221e3da92764 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -457,6 +457,7 @@ java_library( deps = [ ":artifacts", ":has_digest", + "//src/main/java/com/google/devtools/build/lib/analysis:actions/deterministic_writer", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:filesystem_keys", diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index 01be1c2c711034..df47be1a30d306 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -21,12 +21,16 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.common.hash.HashFunction; +import com.google.common.hash.HashingOutputStream; import com.google.common.io.BaseEncoding; +import com.google.common.io.CountingOutputStream; +import com.google.devtools.build.lib.analysis.actions.DeterministicWriter; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.HashCodes; +import com.google.devtools.build.lib.util.StreamWriter; import com.google.devtools.build.lib.vfs.DigestUtils; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.Path; @@ -36,8 +40,10 @@ import com.google.devtools.build.lib.vfs.XattrProvider; import com.google.devtools.build.skyframe.SkyValue; import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.io.OutputStream; import java.time.Instant; import java.util.Arrays; import java.util.Objects; @@ -67,7 +73,7 @@ */ @Immutable @ThreadSafe -public abstract class FileArtifactValue implements SkyValue, HasDigest { +public abstract class FileArtifactValue implements SkyValue, HasDigest, StreamWriter { /** * The type of the underlying file system object. If it is a regular file, then it is guaranteed * to have a digest. Otherwise it does not have a digest. @@ -148,11 +154,30 @@ public InputStream getInputStream() { throw new UnsupportedOperationException(); } + /** + * Writes the inline file contents to the given {@link OutputStream}. + * + * @throws UnsupportedOperationException if the file contents are not inline. + */ + public void writeTo(OutputStream out) throws IOException { + try (var in = getInputStream()) { + in.transferTo(out); + } + } + /** Returns whether the file contents exist remotely. */ public boolean isRemote() { return false; } + /** + * Returns whether the file contents are materialized lazily, for example because they exist + * remotely. + */ + public final boolean isLazy() { + return isRemote() || isInline(); + } + /** Returns the location index for remote files. For non-remote files, returns 0. */ public int getLocationIndex() { return 0; @@ -398,6 +423,20 @@ public static FileArtifactValue createForRemoteFileWithMaterializationData( digest, size, locationIndex, expirationTime); } + public static FileArtifactValue createForFileWriteActionOutput( + DeterministicWriter writer, HashFunction hashFunction) throws IOException { + long size; + byte[] digest; + try (CountingOutputStream countingOut = + new CountingOutputStream(OutputStream.nullOutputStream()); + HashingOutputStream hashingOut = new HashingOutputStream(hashFunction, countingOut)) { + writer.writeOutputFile(hashingOut); + size = countingOut.getCount(); + digest = hashingOut.hash().asBytes(); + } + return new FileWriteOutputArtifactValue(writer, size, digest); + } + public static FileArtifactValue createFromExistingWithResolvedPath( FileArtifactValue delegate, PathFragment resolvedPath) { return new ResolvedSymlinkArtifactValue(delegate, resolvedPath); @@ -1004,6 +1043,105 @@ public boolean wasModifiedSinceDigest(Path path) { } } + private static final class FileWriteOutputArtifactValue extends FileArtifactValue { + private final DeterministicWriter writer; + private final long size; + private final byte[] digest; + @Nullable private FileContentsProxy proxy; + + private FileWriteOutputArtifactValue(DeterministicWriter writer, long size, byte[] digest) { + this.writer = writer; + this.size = size; + this.digest = digest; + } + + @Override + public FileStateType getType() { + return FileStateType.REGULAR_FILE; + } + + @Override + public byte[] getDigest() { + return digest; + } + + @Override + public long getSize() { + return size; + } + + @Override + public boolean isInline() { + return true; + } + + @Override + public InputStream getInputStream() { + // TODO: Avoid materializing the full content in memory by using a variant of + // Piped{Input,Output}Stream that works well with virtual threads. + var out = new ByteArrayOutputStream(Math.clamp(getSize(), 0, Integer.MAX_VALUE)); + try { + writeTo(out); + } catch (IOException e) { + // writer is not expected to throw if out doesn't. + throw new IllegalStateException(e); + } + return new ByteArrayInputStream(out.toByteArray()); + } + + @Override + public void writeTo(OutputStream out) throws IOException { + writer.writeOutputFile(out); + } + + @Override + public long getModifiedTime() { + throw new UnsupportedOperationException(); + } + + /** + * Sets the {@link FileContentsProxy} if the output backed by this metadata is materialized + * later. + */ + @Override + public void setContentsProxy(FileContentsProxy proxy) { + this.proxy = proxy; + } + + /** + * Returns a non-null {@link FileContentsProxy} if this metadata is backed by a local file, e.g. + * the file is materialized after action execution. + */ + @Override + @Nullable + public FileContentsProxy getContentsProxy() { + return proxy; + } + + @Override + public boolean wasModifiedSinceDigest(Path path) { + return false; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof FileWriteOutputArtifactValue that)) { + return false; + } + return Objects.equals(writer, that.writer) + && size == that.size + && Arrays.equals(digest, that.digest); + } + + @Override + public int hashCode() { + return HashCodes.hashObjects(writer, size, Arrays.hashCode(digest)); + } + } + /** Metadata for files whose contents are available in memory. */ private static final class InlineFileArtifactValue extends FileArtifactValue { private final byte[] data; diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java b/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java index e292666e0abb40..6accda1c16813d 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java @@ -114,19 +114,6 @@ protected byte[] writeTo(Path target) throws IOException { return digest; } - /** - * Gets a {@link ByteString} representation of the fake file. Used to avoid copying if the fake - * file is internally represented as a {@link ByteString}. - * - *

Prefer {@link #writeTo} to this method to avoid materializing the entire file in memory. The - * return value should not be retained. - */ - public ByteString getBytes() throws IOException { - ByteString.Output out = ByteString.newOutput(); - writeTo(out); - return out.toByteString(); - } - /** * Returns the metadata for this input if available. Null otherwise. * diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index fa8112519e115f..4e64465e337aa7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -1663,6 +1663,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/unsafe:string", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/lib/util:string", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/DeterministicWriter.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/DeterministicWriter.java index 301860b9812f19..0323cb2d8a9ccb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/DeterministicWriter.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/DeterministicWriter.java @@ -22,6 +22,9 @@ * invocation of writeOutputFile(). */ public interface DeterministicWriter { + /** + * @throws IOException only if out throws an IOException + */ void writeOutputFile(OutputStream out) throws IOException; /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionContext.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionContext.java index e780716e979fbd..4bc456899903d4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionContext.java @@ -56,4 +56,12 @@ default ImmutableList writeOutputToFile( isRemotable, Iterables.getOnlyElement(action.getOutputs())); } + + /** + * Returns whether the {@link DeterministicWriter} may be retained after {@link + * #writeOutputToFile} returns. + */ + default boolean mayRetainWriter() { + return false; + } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/LocalTemplateExpansionStrategy.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/LocalTemplateExpansionStrategy.java index 93396ad2923fb9..cbac36de6a826c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/LocalTemplateExpansionStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/LocalTemplateExpansionStrategy.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.analysis.actions; -import static java.nio.charset.StandardCharsets.ISO_8859_1; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.AbstractAction; @@ -25,6 +24,7 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.server.FailureDetails.Execution; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.unsafe.StringUnsafe; import com.google.devtools.build.lib.util.StringUtilities; import java.io.IOException; import java.util.List; @@ -44,18 +44,43 @@ public ImmutableList expandTemplate( TemplateExpansionContext.TemplateMetadata templateMetadata) throws InterruptedException, ExecException { try { + // If writeOutputToFile may retain the writer, make sure that it doesn't capture the + // expanded template string. Since expansion may throw and the writer must not, expand the + // template once before calling writeOutputToFile. It is assumed to be deterministic, so if + // it doesn't throw once, it won't throw again. final String expandedTemplate = getExpandedTemplateUnsafe( templateMetadata.template(), templateMetadata.substitutions(), ctx.getPathResolver()); - DeterministicWriter deterministicWriter = - out -> out.write(expandedTemplate.getBytes(ISO_8859_1)); - return ctx.getContext(FileWriteActionContext.class) - .writeOutputToFile( - action, - ctx, - deterministicWriter, - templateMetadata.makeExecutable(), - /* isRemotable= */ true); + FileWriteActionContext fileWriteActionContext = ctx.getContext(FileWriteActionContext.class); + DeterministicWriter deterministicWriter; + if (fileWriteActionContext.mayRetainWriter()) { + ArtifactPathResolver pathResolver = ctx.getPathResolver(); + deterministicWriter = + out -> { + try { + out.write( + StringUnsafe.getInternalStringBytes( + getExpandedTemplateUnsafe( + templateMetadata.template(), + templateMetadata.substitutions(), + pathResolver))); + } catch (EvalException e) { + throw new IllegalStateException( + "Template expansion is not deterministic, first succeeded and then failed with: " + + e.getMessage(), + e); + } + }; + } else { + deterministicWriter = + out -> out.write(StringUnsafe.getInternalStringBytes(expandedTemplate)); + } + return fileWriteActionContext.writeOutputToFile( + action, + ctx, + deterministicWriter, + templateMetadata.makeExecutable(), + /* isRemotable= */ true); } catch (IOException | EvalException e) { throw new EnvironmentalExecException( e, diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index 156a5ae39d161b..26e92ed9687f9f 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -141,10 +141,14 @@ java_library( java_library( name = "file_write_strategy", - srcs = ["FileWriteStrategy.java"], + srcs = [ + "EagerFileWriteStrategy.java", + "LazyFileWriteStrategy.java", + ], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/analysis:actions/deterministic_writer", "//src/main/java/com/google/devtools/build/lib/analysis:actions/file_write_action_context", "//src/main/java/com/google/devtools/build/lib/profiler", diff --git a/src/main/java/com/google/devtools/build/lib/exec/FileWriteStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/EagerFileWriteStrategy.java similarity index 95% rename from src/main/java/com/google/devtools/build/lib/exec/FileWriteStrategy.java rename to src/main/java/com/google/devtools/build/lib/exec/EagerFileWriteStrategy.java index dce4893d9345df..f7dc585a9284ad 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/FileWriteStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/EagerFileWriteStrategy.java @@ -35,9 +35,10 @@ /** * A strategy for executing an {@link - * com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction}. + * com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction} that writes the file to + * disk eagerly. */ -public final class FileWriteStrategy implements FileWriteActionContext { +public class EagerFileWriteStrategy implements FileWriteActionContext { private static final Duration MIN_LOGGING = Duration.ofMillis(100); @Override diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java index 811567aea62c74..d73b7bfb06454d 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java @@ -562,6 +562,29 @@ public boolean usingLocalTestJobs() { + " code 39.") public int remoteRetryOnTransientCacheError; + /** An enum for specifying different file write strategies. */ + public enum FileWriteStrategy { + EAGER, + LAZY; + + /** Converts to {@link FileWriteStrategy}. */ + public static class Converter extends EnumConverter { + public Converter() { + super(FileWriteStrategy.class, "file write strategy"); + } + } + } + + @Option( + name = "file_write_strategy", + defaultValue = "eager", + converter = FileWriteStrategy.Converter.class, + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.EXECUTION}, + help = + "If set to 'eager', Bazel will write the output of file write actions to disk. If set to 'lazy', their output will be kept in-memory if possible and is only written out if needed or requested, depending on the value of --remote_download_outputs.") + public FileWriteStrategy fileWriteStrategy; + /** An enum for specifying different formats of test output. */ public enum TestOutputFormat { SUMMARY, // Provide summary output only. diff --git a/src/main/java/com/google/devtools/build/lib/exec/LazyFileWriteStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/LazyFileWriteStrategy.java new file mode 100644 index 00000000000000..15d2f0be2e7179 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/exec/LazyFileWriteStrategy.java @@ -0,0 +1,83 @@ +// Copyright 2025 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.exec; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.AbstractAction; +import com.google.devtools.build.lib.actions.ActionExecutionContext; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.EnvironmentalExecException; +import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.RunningActionEvent; +import com.google.devtools.build.lib.actions.SpawnResult; +import com.google.devtools.build.lib.analysis.actions.DeterministicWriter; +import com.google.devtools.build.lib.profiler.AutoProfiler; +import com.google.devtools.build.lib.profiler.GoogleAutoProfilerUtils; +import com.google.devtools.build.lib.server.FailureDetails; +import java.io.IOException; +import java.time.Duration; + +/** + * A strategy for executing an {@link + * com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction} that avoids writing the + * file to disk if possible. + */ +public final class LazyFileWriteStrategy extends EagerFileWriteStrategy { + private static final Duration MIN_LOGGING = Duration.ofMillis(100); + + @Override + public ImmutableList writeOutputToFile( + AbstractAction action, + ActionExecutionContext actionExecutionContext, + DeterministicWriter deterministicWriter, + boolean makeExecutable, + boolean isRemotable, + Artifact output) + throws ExecException { + if (!isRemotable || actionExecutionContext.getActionFileSystem() == null) { + return super.writeOutputToFile( + action, actionExecutionContext, deterministicWriter, makeExecutable, isRemotable, output); + } + actionExecutionContext.getEventHandler().post(new RunningActionEvent(action, "local")); + try (AutoProfiler p = + GoogleAutoProfilerUtils.logged("hashing output of " + action.prettyPrint(), MIN_LOGGING)) { + // TODO: Bazel currently marks all output files as executable after local execution and stages + // all files as executable for remote execution, so we don't keep track of the executable + // bit yet. + try { + actionExecutionContext + .getOutputMetadataStore() + .injectFile( + output, + FileArtifactValue.createForFileWriteActionOutput( + deterministicWriter, + actionExecutionContext + .getActionFileSystem() + .getDigestFunction() + .getHashFunction())); + } catch (IOException e) { + throw new EnvironmentalExecException( + e, FailureDetails.Execution.Code.FILE_WRITE_IO_EXCEPTION); + } + } + return ImmutableList.of(); + } + + @Override + public boolean mayRetainWriter() { + return true; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index a330456005cbd2..b5a0df2f394d3f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -26,6 +26,7 @@ import static java.util.Objects.requireNonNull; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.common.flogger.GoogleLogger; @@ -58,6 +59,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import io.reactivex.rxjava3.core.Completable; import java.io.IOException; +import java.io.OutputStream; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -562,33 +564,53 @@ private Completable downloadFileNoCheckRx( Path finalPath = path; - Completable download = - usingTempPath( - (tempPath, alreadyDeleted) -> - toCompletable( - () -> - doDownloadFile( - action, - reporter, - tempPath, - finalPath.relativeTo(execRoot), - metadata, - priority, - reason), - directExecutor()) - .doOnComplete( - () -> { - finalizeDownload( - metadata, tempPath, finalPath, dirsWithOutputPermissions); - alreadyDeleted.set(true); - }) - .doOnError( - error -> { - if (error instanceof CacheNotFoundException cacheNotFoundException) { - reporter.post( - new LostInputsEvent(cacheNotFoundException.getMissingDigest())); - } - })); + Completable download; + if (metadata.isInline()) { + download = + usingTempPath( + (tempPath, alreadyDeleted) -> { + try { + tempPath.getParentDirectory().createDirectoryAndParents(); + try (OutputStream out = tempPath.getOutputStream()) { + metadata.writeTo(out); + } + finalizeDownload(metadata, tempPath, finalPath, dirsWithOutputPermissions); + alreadyDeleted.set(true); + return Completable.complete(); + } catch (IOException e) { + return Completable.error(e); + } + }); + } else { + Preconditions.checkState(metadata.isRemote()); + download = + usingTempPath( + (tempPath, alreadyDeleted) -> + toCompletable( + () -> + doDownloadFile( + action, + reporter, + tempPath, + finalPath.relativeTo(execRoot), + metadata, + priority, + reason), + directExecutor()) + .doOnComplete( + () -> { + finalizeDownload( + metadata, tempPath, finalPath, dirsWithOutputPermissions); + alreadyDeleted.set(true); + }) + .doOnError( + error -> { + if (error instanceof CacheNotFoundException cacheNotFoundException) { + reporter.post( + new LostInputsEvent(cacheNotFoundException.getMissingDigest())); + } + })); + } return downloadCache.executeIfNot( finalPath, @@ -719,7 +741,7 @@ public void finalizeAction(Action action, OutputMetadataStore outputMetadataStor } var metadata = outputMetadataStore.getOutputMetadata(output); - if (!metadata.isRemote()) { + if (!metadata.isLazy()) { continue; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index f70600344b8191..cc053621b9af9f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -285,12 +285,8 @@ protected FileSystem getLocalFileSystem() { /** Returns whether a path is stored remotely. Follows symlinks. */ boolean isRemote(Path path) throws IOException { - return isRemote(path.asFragment()); - } - - private boolean isRemote(PathFragment path) throws IOException { // Files in the local filesystem are non-remote by definition, so stat only in-memory sources. - var status = statInternal(path, FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY); + var status = statInternal(path.asFragment(), FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY); return status instanceof FileStatusWithMetadata fileStatusWithMetadata && fileStatusWithMetadata.getMetadata().isRemote(); } @@ -369,7 +365,15 @@ protected boolean delete(PathFragment path) throws IOException { @Override protected InputStream getInputStream(PathFragment path) throws IOException { - downloadFileIfRemote(path); + var inMemoryStatus = statInternal(path, FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY); + if (inMemoryStatus instanceof FileStatusWithMetadata fileStatusWithMetadata) { + if (fileStatusWithMetadata.getMetadata().isInline()) { + return fileStatusWithMetadata.getMetadata().getInputStream(); + } + if (fileStatusWithMetadata.getMetadata().isRemote()) { + downloadRemoteFile(path); + } + } // TODO(tjgq): Consider only falling back to the local filesystem for source (non-output) files. // See getMetadata() for why this isn't currently possible. return localFs.getPath(path).getInputStream(); @@ -748,10 +752,7 @@ FileArtifactValue getInputMetadata(ActionInput input) { return inputArtifactData.getInputMetadata(input); } - private void downloadFileIfRemote(PathFragment path) throws IOException { - if (!isRemote(path)) { - return; - } + private void downloadRemoteFile(PathFragment path) throws IOException { PathFragment execPath = path.relativeTo(execRoot); try { ActionInput input = getInput(execPath.getPathString()); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java index 251791bdab499d..a2f5eee3b231d5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -74,7 +74,7 @@ protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOExc @Override protected boolean canDownloadFile(Path path, FileArtifactValue metadata) { - return metadata.isRemote(); + return metadata.isLazy(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java index fc90073279294b..d810835d6da694 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java @@ -34,7 +34,6 @@ import com.google.common.collect.Iterables; import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.ListenableFuture; -import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; @@ -49,6 +48,7 @@ import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult; +import com.google.devtools.build.lib.util.StreamWriter; import com.google.devtools.build.lib.vfs.Path; import com.google.protobuf.ByteString; import com.google.protobuf.Message; @@ -177,14 +177,14 @@ public void ensureInputsPresent( } } - private static final class VirtualActionInputBlob implements Blob { - private VirtualActionInput virtualActionInput; + private static final class StreamWriterBlob implements Blob { + private StreamWriter streamWriter; // Can be large compared to the retained size of the VirtualActionInput and thus shouldn't be // kept in memory for an extended period of time. private volatile ByteString data; - VirtualActionInputBlob(VirtualActionInput virtualActionInput) { - this.virtualActionInput = Preconditions.checkNotNull(virtualActionInput); + StreamWriterBlob(StreamWriter streamWriter) { + this.streamWriter = Preconditions.checkNotNull(streamWriter); } @Override @@ -192,7 +192,7 @@ public InputStream get() throws IOException { if (data == null) { synchronized (this) { if (data == null) { - data = Preconditions.checkNotNull(virtualActionInput, "used after close()").getBytes(); + data = Preconditions.checkNotNull(streamWriter, "used after close()").getBytes(); } } } @@ -201,7 +201,7 @@ public InputStream get() throws IOException { @Override public void close() { - virtualActionInput = null; + streamWriter = null; data = null; } } @@ -220,9 +220,8 @@ private ListenableFuture uploadBlob( ContentSource file = merkleTree.getFileByDigest(digest); if (file != null) { return switch (file) { - case ContentSource.VirtualActionInputSource(VirtualActionInput virtualActionInput) -> - remoteCacheClient.uploadBlob( - context, digest, new VirtualActionInputBlob(virtualActionInput)); + case ContentSource.InMemorySource(StreamWriter streamWriter) -> + remoteCacheClient.uploadBlob(context, digest, new StreamWriterBlob(streamWriter)); case ContentSource.PathSource(Path path) -> { try { if (remotePathChecker.isRemote(context, path)) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java index f5f44b3d729f8d..60f361bfdb486d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java @@ -279,7 +279,6 @@ public boolean shouldDownloadOutput(ActionInput output) { return shouldDownloadOutput(output.getExecPath()); } - /** Returns whether an {@link ActionInput} with the given path should be downloaded. */ public boolean shouldDownloadOutput(PathFragment execPath) { return outputsMode == RemoteOutputsMode.ALL || pathsToDownload.contains(execPath) @@ -288,8 +287,8 @@ public boolean shouldDownloadOutput(PathFragment execPath) { @Override public boolean shouldTrustArtifact(ActionInput file, FileArtifactValue metadata) { - // Local metadata is always trusted. - if (!metadata.isRemote()) { + // Eager metadata is always trusted. + if (!metadata.isLazy()) { return true; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD b/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD index 00f5b343ae3cf4..027857705eabb1 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD @@ -22,6 +22,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/remote:scrubber", "//src/main/java/com/google/devtools/build/lib/remote/util:digest_utils", + "//src/main/java/com/google/devtools/build/lib/util:stream_writer", "//src/main/java/com/google/devtools/build/lib/util:string_encoding", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java index c24763b7d0f8a8..56169f74750fcc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java @@ -16,7 +16,7 @@ import build.bazel.remote.execution.v2.Digest; import com.google.common.base.Preconditions; import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.actions.cache.VirtualActionInput; +import com.google.devtools.build.lib.util.StreamWriter; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.errorprone.annotations.CanIgnoreReturnValue; @@ -73,7 +73,7 @@ public boolean equals(Object o) { static class FileNode extends Node { private final Path path; - private final VirtualActionInput virtualActionInput; + private final StreamWriter data; private final Digest digest; private final boolean isExecutable; private final boolean toolInput; @@ -97,7 +97,7 @@ static FileNode createExecutable( static FileNode createExecutable( String pathSegment, - VirtualActionInput virtualActionInput, + StreamWriter virtualActionInput, Digest digest, boolean toolInput) { return new FileNode( @@ -108,7 +108,7 @@ private FileNode( String pathSegment, Path path, Digest digest, boolean isExecutable, boolean toolInput) { super(pathSegment); this.path = Preconditions.checkNotNull(path, "path"); - this.virtualActionInput = null; + this.data = null; this.digest = Preconditions.checkNotNull(digest, "digest"); this.isExecutable = isExecutable; this.toolInput = toolInput; @@ -116,13 +116,13 @@ private FileNode( private FileNode( String pathSegment, - VirtualActionInput input, + StreamWriter input, Digest digest, boolean isExecutable, boolean toolInput) { super(pathSegment); this.path = null; - this.virtualActionInput = Preconditions.checkNotNull(input, "data"); + this.data = Preconditions.checkNotNull(input, "data"); this.digest = Preconditions.checkNotNull(digest, "digest"); this.isExecutable = isExecutable; this.toolInput = toolInput; @@ -136,8 +136,8 @@ Path getPath() { return path; } - VirtualActionInput getVirtualActionInput() { - return virtualActionInput; + StreamWriter getData() { + return data; } public boolean isExecutable() { @@ -151,7 +151,7 @@ boolean isToolInput() { @Override public int hashCode() { return Objects.hash( - super.hashCode(), path, virtualActionInput, digest, toolInput, isExecutable); + super.hashCode(), path, data, digest, toolInput, isExecutable); } @Override @@ -159,7 +159,7 @@ public boolean equals(Object o) { if (o instanceof FileNode other) { return super.equals(other) && Objects.equals(path, other.path) - && Objects.equals(virtualActionInput, other.virtualActionInput) + && Objects.equals(data, other.data) && Objects.equals(digest, other.digest) && toolInput == other.toolInput && isExecutable == other.isExecutable; diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java index 38f46a177a3004..8e915e03d92f82 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java @@ -169,11 +169,21 @@ private static int buildFromActionInputs( switch (metadata.getType()) { case REGULAR_FILE -> { Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); - Path inputPath = artifactPathResolver.toPath(input); - boolean childAdded = - currDir.addChild( - FileNode.createExecutable( - path.getBaseName(), inputPath, d, toolInputs.contains(path))); + boolean childAdded; + if (metadata.isInline()) { + childAdded = + currDir.addChild( + FileNode.createExecutable( + path.getBaseName(), metadata, d, toolInputs.contains(path))); + } else { + childAdded = + currDir.addChild( + FileNode.createExecutable( + path.getBaseName(), + artifactPathResolver.toPath(input), + d, + toolInputs.contains(path))); + } return childAdded ? 1 : 0; } case DIRECTORY -> { diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java index db6f3b90c276e4..669c569023a3dc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java @@ -32,13 +32,13 @@ import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.InputMetadataProvider; -import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.remote.Scrubber.SpawnScrubber; +import com.google.devtools.build.lib.remote.merkletree.MerkleTree.ContentSource.InMemorySource; import com.google.devtools.build.lib.remote.merkletree.MerkleTree.ContentSource.PathSource; -import com.google.devtools.build.lib.remote.merkletree.MerkleTree.ContentSource.VirtualActionInputSource; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.util.StreamWriter; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; @@ -63,8 +63,7 @@ public sealed interface ContentSource { record PathSource(Path path) implements ContentSource {} /** Content provided by a virtual action input. */ - record VirtualActionInputSource(VirtualActionInput virtualActionInput) - implements ContentSource {} + record InMemorySource(StreamWriter streamWriter) implements ContentSource {} } private interface MerkleTreeDirectoryVisitor { @@ -172,7 +171,7 @@ private Map getDigestFileMap() { if (file.getPath() != null) { newDigestMap.put(file.getDigest(), file.getPath()); } else { - newDigestMap.put(file.getDigest(), file.getVirtualActionInput()); + newDigestMap.put(file.getDigest(), file.getData()); } } }); @@ -190,8 +189,7 @@ public Directory getDirectoryByDigest(Digest digest) { public ContentSource getFileByDigest(Digest digest) { return switch (getDigestFileMap().get(digest)) { case Path path -> new PathSource(path); - case VirtualActionInput virtualActionInput -> - new VirtualActionInputSource(virtualActionInput); + case StreamWriter streamWriter -> new InMemorySource(streamWriter); case null -> null; default -> throw new IllegalStateException("Unexpected value: " + getDigestFileMap().get(digest)); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index d1f0e1ba531640..218410f4af30a0 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -1091,11 +1091,13 @@ private static BlazeCommandResult fullyValidateTarget( Path executablePath = executable.getPath(); try { - if (!executablePath.exists() || !executablePath.isExecutable()) { + if (!executablePath.exists()) { return reportAndCreateFailureResult( - env, - "Non-existent or non-executable " + executablePath, - Code.TARGET_BUILT_BUT_PATH_NOT_EXECUTABLE); + env, "Non-existent " + executablePath, Code.TARGET_BUILT_BUT_PATH_NOT_EXECUTABLE); + } + if (!executablePath.isExecutable()) { + return reportAndCreateFailureResult( + env, "Non-executable " + executablePath, Code.TARGET_BUILT_BUT_PATH_NOT_EXECUTABLE); } } catch (IOException e) { return reportAndCreateFailureResult( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index d48071791a8f8e..cdf22a4d104a30 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java @@ -524,11 +524,11 @@ private boolean artifactIsDirtyWithDirectSystemCalls( FileArtifactValue fileMetadata = ActionOutputMetadataStore.fileArtifactValueFromArtifact( file, null, xattrProviderOverrider.getXattrProvider(syscallCache), tsgm); - boolean isTrustedRemoteValue = + boolean isTrustedLazyValue = fileMetadata.getType() == FileStateType.NONEXISTENT - && lastKnownData.isRemote() + && lastKnownData.isLazy() && outputChecker.shouldTrustArtifact(file, lastKnownData); - if (!isTrustedRemoteValue && fileMetadata.couldBeModifiedSince(lastKnownData)) { + if (!isTrustedLazyValue && fileMetadata.couldBeModifiedSince(lastKnownData)) { modifiedOutputsReceiver.reportModifiedOutputFile( fileMetadata.getType() != FileStateType.NONEXISTENT ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java index 6535cc94671ee9..d4e75e7e6a29cb 100644 --- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java +++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java @@ -22,8 +22,9 @@ import com.google.devtools.build.lib.analysis.test.TestActionContext; import com.google.devtools.build.lib.analysis.test.TestStrategy; import com.google.devtools.build.lib.buildtool.BuildRequest; +import com.google.devtools.build.lib.exec.EagerFileWriteStrategy; import com.google.devtools.build.lib.exec.ExecutionOptions; -import com.google.devtools.build.lib.exec.FileWriteStrategy; +import com.google.devtools.build.lib.exec.LazyFileWriteStrategy; import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; import com.google.devtools.build.lib.exec.RunfilesTreeUpdater; import com.google.devtools.build.lib.exec.SpawnRunner; @@ -69,7 +70,13 @@ public void registerActionContexts( registryBuilder.register(TestActionContext.class, testStrategy, "standalone"); registryBuilder.register( TestActionContext.class, new ExclusiveTestStrategy(testStrategy), "exclusive"); - registryBuilder.register(FileWriteActionContext.class, new FileWriteStrategy(), "local"); + registryBuilder.register( + FileWriteActionContext.class, + switch (executionOptions.fileWriteStrategy) { + case LAZY -> new LazyFileWriteStrategy(); + case EAGER -> new EagerFileWriteStrategy(); + }, + "local"); registryBuilder.register( TemplateExpansionContext.class, new LocalTemplateExpansionStrategy(), "local"); } diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD index e1651d83476157..fe328ea6be02f8 100644 --- a/src/main/java/com/google/devtools/build/lib/util/BUILD +++ b/src/main/java/com/google/devtools/build/lib/util/BUILD @@ -271,6 +271,9 @@ java_library( srcs = [ "StreamWriter.java", ], + deps = [ + "@com_google_protobuf//:protobuf_java", + ], ) java_library( diff --git a/src/main/java/com/google/devtools/build/lib/util/StreamWriter.java b/src/main/java/com/google/devtools/build/lib/util/StreamWriter.java index 2a501d323ae36b..e1e87c37cfa4d4 100644 --- a/src/main/java/com/google/devtools/build/lib/util/StreamWriter.java +++ b/src/main/java/com/google/devtools/build/lib/util/StreamWriter.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.util; +import com.google.protobuf.ByteString; import java.io.IOException; import java.io.OutputStream; @@ -27,4 +28,22 @@ public interface StreamWriter { * @throws IOException only if out throws an IOException */ void writeTo(OutputStream out) throws IOException; + + /** + * Gets a {@link ByteString} representation of the content to write. Used to avoid copying if the + * stream is internally represented as a {@link ByteString}. + * + *

Prefer {@link #writeTo} to this method to avoid materializing the entire file in memory. The + * return value should not be retained. + */ + default ByteString getBytes() { + ByteString.Output out = ByteString.newOutput(); + try { + writeTo(out); + } catch (IOException e) { + // ByteString.Output doesn't throw IOExceptions. + throw new IllegalStateException(e); + } + return out.toByteString(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java index 0e4623b297b035..391ed95d11928c 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java @@ -37,7 +37,7 @@ import com.google.devtools.build.lib.buildtool.util.TestRuleModule; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.EventHandler; -import com.google.devtools.build.lib.exec.FileWriteStrategy; +import com.google.devtools.build.lib.exec.EagerFileWriteStrategy; import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; import com.google.devtools.build.lib.packages.AttributeTransitionData; import com.google.devtools.build.lib.packages.ImplicitOutputsFunction; @@ -195,7 +195,7 @@ public void registerActionContexts( CommandEnvironment env, BuildRequest buildRequest) { // we need an implementation of FileWriteActionContext to get our file writes to succeed - registryBuilder.register(FileWriteActionContext.class, new FileWriteStrategy()); + registryBuilder.register(FileWriteActionContext.class, new EagerFileWriteStrategy()); // we need something to consume FileWriteActionContext or the registration will have no effect registryBuilder.restrictTo(FileWriteActionContext.class, "local"); } diff --git a/src/test/java/com/google/devtools/build/lib/exec/BUILD b/src/test/java/com/google/devtools/build/lib/exec/BUILD index bd7978c336e65c..8532fa6d9e4e4c 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/BUILD @@ -46,6 +46,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", "//src/main/java/com/google/devtools/build/lib/bazel/rules/python", + "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/cmdline", @@ -71,6 +72,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:symlink_tree_helper", "//src/main/java/com/google/devtools/build/lib/exec:symlink_tree_strategy", "//src/main/java/com/google/devtools/build/lib/exec:test_log_helper", + "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/shell", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", diff --git a/src/test/java/com/google/devtools/build/lib/exec/FileWriteStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/EagerFileWriteStrategyTest.java similarity index 95% rename from src/test/java/com/google/devtools/build/lib/exec/FileWriteStrategyTest.java rename to src/test/java/com/google/devtools/build/lib/exec/EagerFileWriteStrategyTest.java index 31abcda73ac388..e5e207d1ccd6a3 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/FileWriteStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/EagerFileWriteStrategyTest.java @@ -48,12 +48,12 @@ import org.junit.Test; import org.junit.runner.RunWith; -/** Tests for {@link FileWriteStrategy}. */ +/** Tests for {@link EagerFileWriteStrategy}. */ @RunWith(TestParameterInjector.class) -public final class FileWriteStrategyTest { +public final class EagerFileWriteStrategyTest { private static final IOException INJECTED_EXCEPTION = new IOException("oh no!"); - private final FileWriteStrategy fileWriteStrategy = new FileWriteStrategy(); + private final EagerFileWriteStrategy eagerFileWriteStrategy = new EagerFileWriteStrategy(); private final SpiedFileSystem fileSystem = SpiedFileSystem.createInMemorySpy(); private final Scratch scratch = new Scratch(fileSystem); @@ -73,7 +73,7 @@ public void writeOutputToFile_writesCorrectOutput( AbstractAction action = createAction("file"); var unused = - fileWriteStrategy.writeOutputToFile( + eagerFileWriteStrategy.writeOutputToFile( action, createActionExecutionContext(), out -> out.write(content.getBytes(UTF_8)), @@ -124,7 +124,7 @@ public void writeOutputToFile_errorInWriter_returnsFailure(@TestParameter Failur assertThrows( EnvironmentalExecException.class, () -> { - fileWriteStrategy.writeOutputToFile( + eagerFileWriteStrategy.writeOutputToFile( action, createActionExecutionContext(), failureMode, diff --git a/src/test/java/com/google/devtools/build/lib/exec/LazyFileWriteStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/LazyFileWriteStrategyTest.java new file mode 100644 index 00000000000000..2bef191fc922ca --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/exec/LazyFileWriteStrategyTest.java @@ -0,0 +1,190 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.exec; + +import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSortedSet; +import com.google.devtools.build.lib.actions.AbstractAction; +import com.google.devtools.build.lib.actions.ActionExecutionContext; +import com.google.devtools.build.lib.actions.ActionExecutionException; +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputMap; +import com.google.devtools.build.lib.actions.ActionInputPrefetcher; +import com.google.devtools.build.lib.actions.ActionKeyContext; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactRoot; +import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; +import com.google.devtools.build.lib.actions.DiscoveredModulesPruner; +import com.google.devtools.build.lib.actions.EnvironmentalExecException; +import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.Executor; +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.ThreadStateReceiver; +import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.actions.util.ActionsTestUtil.FakeInputMetadataHandlerBase; +import com.google.devtools.build.lib.actions.util.ActionsTestUtil.NullAction; +import com.google.devtools.build.lib.actions.util.DummyExecutor; +import com.google.devtools.build.lib.bugreport.BugReporter; +import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.remote.RemoteActionFileSystem; +import com.google.devtools.build.lib.remote.RemoteActionInputFetcher; +import com.google.devtools.build.lib.server.FailureDetails.Execution.Code; +import com.google.devtools.build.lib.testutil.Scratch; +import com.google.devtools.build.lib.util.DetailedExitCode; +import com.google.devtools.build.lib.util.ExitCode; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.SyscallCache; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; +import javax.annotation.Nullable; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** Tests for {@link LazyFileWriteStrategy}. */ +@RunWith(TestParameterInjector.class) +public final class LazyFileWriteStrategyTest { + private static final IOException INJECTED_EXCEPTION = new IOException("oh no!"); + + private final LazyFileWriteStrategy lazyFileWriteStrategy = new LazyFileWriteStrategy(); + + private final FileSystem fileSystem = new InMemoryFileSystem(DigestHashFunction.SHA256); + private final Scratch scratch = new Scratch(fileSystem); + private Path execRoot; + private ArtifactRoot outputRoot; + private RemoteActionFileSystem actionFileSystem; + private FakeInputMetadataHandlerBase metadataHandler; + private AbstractAction action; + + @Before + public void createOutputRoot() throws IOException { + execRoot = scratch.dir("/execroot"); + outputRoot = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "bazel-out"); + outputRoot.getRoot().asPath().createDirectory(); + metadataHandler = + new FakeInputMetadataHandlerBase() { + private Map injectedMetadata = new HashMap<>(); + + @Override + public void injectFile(Artifact output, FileArtifactValue metadata) { + injectedMetadata.put(output, metadata); + } + + @Nullable + @Override + public FileArtifactValue getOutputMetadata(ActionInput input) { + return injectedMetadata.get(input); + } + }; + action = createAction("file"); + actionFileSystem = + new RemoteActionFileSystem( + fileSystem, + execRoot.asFragment(), + outputRoot.getExecPathString(), + new ActionInputMap(BugReporter.defaultInstance(), 0), + action.getOutputs(), + metadataHandler, + mock(RemoteActionInputFetcher.class)); + actionFileSystem.createDirectoryAndParents( + execRoot + .getRelative(action.getPrimaryOutput().getExecPath().getParentDirectory()) + .asFragment()); + } + + @Test + public void writeOutputToFile_writesCorrectOutput( + @TestParameter({"", "hello", "hello there"}) String content) throws Exception { + var unused = + lazyFileWriteStrategy.writeOutputToFile( + action, + createActionExecutionContext(actionFileSystem, metadataHandler), + out -> out.write(content.getBytes(UTF_8)), + /* makeExecutable= */ false, + /* isRemotable= */ true); + + FileArtifactValue metadata = metadataHandler.getOutputMetadata(action.getPrimaryOutput()); + assertThat(metadata.isInline()).isTrue(); + assertThat(metadata.getInputStream().readAllBytes()).isEqualTo(content.getBytes(UTF_8)); + } + + @Test + public void writeOutputToFile_errorInWriter_returnsFailure() throws Exception { + AbstractAction action = createAction("file"); + + ExecException e = + assertThrows( + EnvironmentalExecException.class, + () -> + lazyFileWriteStrategy.writeOutputToFile( + action, + createActionExecutionContext(actionFileSystem, metadataHandler), + out -> { + throw INJECTED_EXCEPTION; + }, + /* makeExecutable= */ false, + /* isRemotable= */ true)); + + assertThat(e).hasCauseThat().isSameInstanceAs(INJECTED_EXCEPTION); + var detailExitCode = getDetailExitCode(e); + assertThat(detailExitCode.getExitCode()).isEqualTo(ExitCode.LOCAL_ENVIRONMENTAL_ERROR); + assertThat(detailExitCode.getFailureDetail().getExecution().getCode()) + .isEqualTo(Code.FILE_WRITE_IO_EXCEPTION); + } + + private DetailedExitCode getDetailExitCode(ExecException e) { + return ActionExecutionException.fromExecException(e, new NullAction()).getDetailedExitCode(); + } + + private ActionExecutionContext createActionExecutionContext( + FileSystem actionFileSystem, FakeInputMetadataHandlerBase metadataHandler) { + Executor executor = new DummyExecutor(fileSystem, execRoot); + return new ActionExecutionContext( + executor, + /* inputMetadataProvider= */ metadataHandler, + ActionInputPrefetcher.NONE, + new ActionKeyContext(), + /* outputMetadataStore= */ metadataHandler, + /* rewindingEnabled= */ false, + ActionExecutionContext.LostInputsCheck.NONE, + /* fileOutErr= */ null, + NullEventHandler.INSTANCE, + /* clientEnv= */ ImmutableMap.of(), + /* topLevelFilesets= */ ImmutableMap.of(), + treeArtifact -> ImmutableSortedSet.of(), + /* actionFileSystem= */ actionFileSystem, + /* skyframeDepsResult= */ null, + DiscoveredModulesPruner.DEFAULT, + SyscallCache.NO_CACHE, + ThreadStateReceiver.NULL_INSTANCE); + } + + private AbstractAction createAction(String outputRelativePath) { + return new NullAction( + ActionsTestUtil.createArtifactWithRootRelativePath( + outputRoot, PathFragment.create(outputRelativePath))); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java index 7cd1a44c8ce9ee..a66e9557d2720c 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java @@ -29,7 +29,7 @@ import com.google.devtools.build.lib.exec.BinTools; import com.google.devtools.build.lib.exec.BlazeExecutor; import com.google.devtools.build.lib.exec.ExecutionOptions; -import com.google.devtools.build.lib.exec.FileWriteStrategy; +import com.google.devtools.build.lib.exec.EagerFileWriteStrategy; import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; import com.google.devtools.build.lib.exec.SpawnStrategyRegistry; import com.google.devtools.build.lib.exec.SpawnStrategyResolver; @@ -69,7 +69,7 @@ public TestExecutorBuilder( public TestExecutorBuilder(FileSystem fileSystem, Path execRoot, BinTools binTools) { this.fileSystem = fileSystem; this.execRoot = execRoot; - addContext(FileWriteActionContext.class, new FileWriteStrategy()); + addContext(FileWriteActionContext.class, new EagerFileWriteStrategy()); addContext(TemplateExpansionContext.class, new LocalTemplateExpansionStrategy()); addContext( SymlinkTreeActionContext.class, diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 0afa13bb3aca7a..1b9c95c82eb06b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -212,6 +212,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", + "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util:command", @@ -223,6 +224,7 @@ java_library( "//third_party:guava", "//third_party:junit4", "//third_party:truth", + "@maven//:com_google_testparameterinjector_test_parameter_injector", ], ) @@ -252,6 +254,7 @@ java_test( "//third_party:guava", "//third_party:junit4", "//third_party:truth", + "@maven//:com_google_testparameterinjector_test_parameter_injector", ], ) diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 04294c160cf0c0..c44bc21bff58e0 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java @@ -39,6 +39,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.io.IOException; import org.junit.After; import org.junit.Test; @@ -46,7 +47,7 @@ import org.junit.runners.JUnit4; /** Integration tests for Build without the Bytes. */ -@RunWith(JUnit4.class) +@RunWith(TestParameterInjector.class) public class BuildWithoutTheBytesIntegrationTest extends BuildWithoutTheBytesIntegrationTestBase { private WorkerInstance worker; @@ -190,6 +191,8 @@ def _substitute_username_impl(ctx): ) """); + addOptions("--file_write_strategy=eager"); + buildTarget("//a:substitute-buchgr"); // The genrule //a:generate-template should run remotely and //a:substitute-buchgr should be a diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java index f0b7f0216e9f14..43910599e416e6 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java @@ -31,13 +31,16 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.analysis.TargetCompleteEvent; import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase; +import com.google.devtools.build.lib.exec.ExecutionOptions.FileWriteStrategy; import com.google.devtools.build.lib.skyframe.ActionExecutionValue; import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.util.CommandBuilder; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.io.RecordingOutErr; +import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.testing.junit.testparameterinjector.TestParameter; import java.io.IOException; import java.util.ArrayList; import java.util.List; @@ -1029,6 +1032,54 @@ public void downloadToplevel_symlinkToDirectory() throws Exception { assertValidOutputFile("foo-link/file-3", "3"); } + @Test + public void downloadToplevel_symlinkToWrittenFile( + @TestParameter FileWriteStrategy fileWriteStrategy, + @TestParameter({"expand_template", "write_file"}) String fileWriteRule) + throws Exception { + setDownloadToplevel(); + writeSymlinkRule(); + writeFileWriteRules(); + write( + "BUILD", + """ + load(':symlink.bzl', 'symlink') + load('//rules:%1$s.bzl', '%1$s') + %1$s( + name = 'foo', + content = 'hello', + # assertValidOutputFile checks for the executable bit. + executable = True, + ) + symlink( + name = 'foo-link', + target_artifact = ':foo', + ) + """ + .formatted(fileWriteRule)); + + addOptions("--file_write_strategy=" + fileWriteStrategy); + + buildTarget("//:foo-link"); + + assertSymlink("foo-link", getOutputPath("foo").asFragment()); + assertValidOutputFile("foo-link", "hello"); + + // Delete link, re-plant symlink + getOutputPath("foo-link").delete(); + buildTarget("//:foo-link"); + + assertSymlink("foo-link", getOutputPath("foo").asFragment()); + assertValidOutputFile("foo-link", "hello"); + + // Delete target, re-download it + getOutputPath("foo").delete(); + buildTarget("//:foo-link"); + + assertSymlink("foo-link", getOutputPath("foo").asFragment()); + assertValidOutputFile("foo-link", "hello"); + } + @Test public void downloadToplevel_unresolvedSymlink() throws Exception { // Dangling symlink would require developer mode to be enabled in the CI environment. @@ -1055,6 +1106,89 @@ public void downloadToplevel_unresolvedSymlink() throws Exception { assertSymlink("foo-link", PathFragment.create("/some/path")); } + @Test + public void downloadMinimal_fileWrite( + @TestParameter boolean isExecutable, + @TestParameter FileWriteStrategy fileWriteStrategy, + @TestParameter({"expand_template", "write_file"}) String fileWriteRule) + throws Exception { + writeFileWriteRules(); + writeSymlinkRule(); + // Remote execution stages all files as executable. + write( + "BUILD", + """ + load('//rules:%1$s.bzl', '%1$s') + %1$s( + name = 'foo', + content = 'hello', + executable = %2$s, + ) + genrule( + name = 'gen', + srcs = [':foo'], + outs = ['out/gen.txt'], + cmd = \""" + %3$s + cat $(location :foo) $(location :foo) > $@ + \""", + ) + """ + .formatted( + fileWriteRule, + isExecutable ? "True" : "False", + OS.getCurrent() == OS.WINDOWS + ? "" + : """ + [ -x $(location :foo) ] || { echo "unexpectedly not executable"; exit 1; } + """)); + + addOptions("--file_write_strategy=" + fileWriteStrategy); + + buildTarget("//:gen"); + if (fileWriteStrategy == FileWriteStrategy.LAZY) { + assertOutputsDoNotExist("//:foo"); + } else { + assertValidOutputFile("foo", "hello"); + } + assertOutputsDoNotExist("//:gen"); + } + + @Test + public void downloadToplevel_fileWrite( + @TestParameter boolean isExecutable, + @TestParameter FileWriteStrategy fileWriteStrategy, + @TestParameter({"expand_template", "write_file"}) String fileWriteRule) + throws Exception { + setDownloadToplevel(); + writeFileWriteRules(); + write( + "BUILD", + """ + load('//rules:%1$s.bzl', '%1$s') + %1$s( + name = 'foo', + content = 'hello', + executable = %2$s, + ) + """ + .formatted(fileWriteRule, isExecutable ? "True" : "False")); + + addOptions("--file_write_strategy=" + fileWriteStrategy); + + buildTarget("//:foo"); + + assertOnlyOutputContent("//:foo", "foo", "hello"); + // TODO: Bazel doesn't honor the executable bit. + assertThat(getOutputPath("foo").isExecutable()).isTrue(); + + // Delete file, re-create it + getOutputPath("foo-link").delete(); + buildTarget("//:foo"); + + assertOnlyOutputContent("//:foo", "foo", "hello"); + } + @Test public void treeOutputsFromLocalFileSystem_works() throws Exception { // Test that tree artifact generated locally can be consumed by other actions. @@ -1372,6 +1506,57 @@ public void incrementalBuild_fileOutputIsPrefetched_noRuns() throws Exception { assertThat(metadata.getContentsProxy()).isNotNull(); } + @Test + public void incrementalBuild_writeFileOutputIsPrefetched_noRuns( + @TestParameter({"expand_template", "write_file"}) String fileWriteRule) throws Exception { + // We need to download the intermediate output + if (!hasAccessToRemoteOutputs()) { + return; + } + + // Arrange: Prepare workspace and run a clean build + writeFileWriteRules(); + write( + "BUILD", + """ + load('//rules:%1$s.bzl', '%1$s') + %1$s( + name = 'foo', + content = 'foo', + executable = True, + ) + genrule( + name = 'foobar', + srcs = [':foo'], + outs = ['out/foobar.txt'], + cmd = 'cat $(location :foo) > $@ && echo bar >> $@', + tags = ['no-remote'], + ) + """ + .formatted(fileWriteRule)); + + addOptions("--file_write_strategy=lazy"); + + buildTarget("//:foobar"); + assertValidOutputFile("foo", "foo"); + assertValidOutputFile("out/foobar.txt", "foobar\n"); + assertThat(getOnlyElement(getMetadata("//:foo").values()).isLazy()).isTrue(); + + // Act: Do an incremental build without any modifications + ActionEventCollector actionEventCollector = new ActionEventCollector(); + getRuntimeWrapper().registerSubscriber(actionEventCollector); + buildTarget("//:foobar"); + + // Assert: remote file metadata has contents proxy and action node is not marked as dirty. + assertValidOutputFile("foo", "foo"); + assertValidOutputFile("out/foobar.txt", "foobar\n"); + assertThat(actionEventCollector.getActionExecutedEvents()).isEmpty(); + assertThat(actionEventCollector.getCachedActionEvents()).isEmpty(); + var metadata = getOnlyElement(getMetadata("//:foo").values()); + assertThat(metadata.isLazy()).isTrue(); + assertThat(metadata.getContentsProxy()).isNotNull(); + } + @Test public void incrementalBuild_treeOutputIsPrefetched_noRuns() throws Exception { // We need to download the intermediate output @@ -1995,6 +2180,66 @@ protected void writeCopyAspectRule(boolean aggregate) throws IOException { write("rules.bzl", lines.build().toArray(new String[0])); } + protected void writeFileWriteRules() throws IOException { + write( + "rules/write_file.bzl", + """ + def _write_file_impl(ctx): + out = ctx.actions.declare_file(ctx.label.name) + ctx.actions.write(output = out, content = ctx.attr.content, is_executable = ctx.attr.executable) + return DefaultInfo(files = depset([out])) + + write_file = rule( + implementation = _write_file_impl, + attrs = { + "content": attr.string(), + "executable": attr.bool(default = False), + }, + ) + """); + write( + "rules/expand_template.bzl", + """ + def identity(x): + return x + + def _expand_template_impl(ctx): + out = ctx.actions.declare_file(ctx.label.name) + substitutions = ctx.actions.template_dict() + substitutions.add_joined( + "%CONTENT%", + depset([ctx.attr.content]), + join_with = "", + map_each = identity) + ctx.actions.expand_template( + output = out, + template = ctx.file._template, + computed_substitutions = substitutions, + is_executable = ctx.attr.executable, + ) + return DefaultInfo(files = depset([out])) + + expand_template = rule( + implementation = _expand_template_impl, + attrs = { + "content": attr.string(), + "executable": attr.bool(default = False), + "_template": attr.label( + default = "template.txt", + allow_single_file = True, + ), + }, + ) + """); + write( + "rules/BUILD", + """ + exports_files(["template.txt"]) + """); + FileSystemUtils.writeContentAsLatin1( + getWorkspace().getRelative("rules/template.txt"), "%CONTENT%"); + } + protected static class ActionEventCollector { private final List actionExecutedEvents = new ArrayList<>(); private final List cachedActionEvents = new ArrayList<>(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index 4af76aa711eb24..3ada6ece663fc5 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -53,6 +53,7 @@ import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.testing.vfs.SpiedFileSystem; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigest; @@ -273,6 +274,23 @@ public void getInputStream_fromLocalFilesystem_forOutputFile() throws Exception assertThat(contents).isEqualTo("local contents"); } + @Test + public void testGetInputStream_fromInlineMetadata() throws Exception { + // arrange + ActionInputMap inputs = new ActionInputMap(1); + Artifact artifact = createInlineArtifact("inline-file", "inline contents", inputs); + FileSystem actionFs = createActionFileSystem(inputs); + + // act + Path actionFsPath = actionFs.getPath(artifact.getPath().asFragment()); + String contents = FileSystemUtils.readContent(actionFsPath, UTF_8); + + // assert + assertThat(actionFsPath.getFileSystem()).isSameInstanceAs(actionFs); + assertThat(contents).isEqualTo("inline contents"); + verifyNoMoreInteractions(inputFetcher); + } + @Test public void getInput_fromInputArtifactData_forLocalArtifact() throws Exception { ActionInputMap inputs = new ActionInputMap(1); @@ -1430,6 +1448,17 @@ private TreeArtifactValue createLocalTreeArtifactValue( return builder.build(); } + /** Returns an inline artifact and puts its metadata into the action input map. */ + private Artifact createInlineArtifact(String pathFragment, String content, ActionInputMap inputs) + throws IOException { + Artifact a = ActionsTestUtil.createArtifact(outputRoot, pathFragment); + FileArtifactValue f = + FileArtifactValue.createForFileWriteActionOutput( + out -> out.write(content.getBytes(UTF_8)), DigestHashFunction.SHA256.getHashFunction()); + inputs.putWithNoDepOwner(a, f); + return a; + } + private byte[] getDigest(String content) { return fs.getDigestFunction().getHashFunction().hashString(content, UTF_8).asBytes(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 68bbde3cddabb2..b9cee597ab9027 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -1402,11 +1402,30 @@ private FileArtifactValue createRemoteMetadata(String contents, Instant expirati hash.asBytes(), data.length, -1, expirationTime); } + private FileArtifactValue createInlineMetadata(String contents) throws IOException { + return FileArtifactValue.createForFileWriteActionOutput( + out -> out.write(contents.getBytes()), fs.getDigestFunction().getHashFunction()); + } + + enum LazyMetadataType { + INLINE, + REMOTE; + } + + private FileArtifactValue createMetadata(LazyMetadataType type, String contents) + throws IOException { + return switch (type) { + case INLINE -> createInlineMetadata(contents); + case REMOTE -> createRemoteMetadata(contents); + }; + } + @Test - public void testRemoteAndLocalArtifacts(@TestParameter boolean setContentsProxy) + public void testLazyAndLocalArtifacts( + @TestParameter LazyMetadataType metadataType, @TestParameter boolean setContentsProxy) throws Exception { - // Test that injected remote artifacts are trusted by the FileSystemValueChecker if it is - // configured to trust remote artifacts, and that local files always take precedence over remote + // Test that injected lazy artifacts are trusted by the FileSystemValueChecker if it is + // configured to trust lazy artifacts, and that local files always take precedence over remote // files if they are different. SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 1); @@ -1414,10 +1433,10 @@ public void testRemoteAndLocalArtifacts(@TestParameter boolean setContentsProxy) Artifact out1 = createDerivedArtifact("foo"); Artifact out2 = createDerivedArtifact("bar"); Map metadataToInject = new HashMap<>(); - var out1Metadata = createRemoteMetadata("foo-content"); + var out1Metadata = createMetadata(metadataType, "foo-content"); metadataToInject.put(actionKey1, actionValueWithMetadata(out1, out1Metadata)); metadataToInject.put( - actionKey2, actionValueWithMetadata(out2, createRemoteMetadata("bar-content"))); + actionKey2, actionValueWithMetadata(out2, createMetadata(metadataType, "bar-content"))); differencer.inject(metadataToInject); EvaluationContext evaluationContext = @@ -1469,9 +1488,10 @@ public void testRemoteAndLocalArtifacts(@TestParameter boolean setContentsProxy) } @Test - public void testRemoteAndLocalArtifacts_identicalContent(@TestParameter boolean setContentsProxy) + public void testLazyAndLocalArtifacts_identicalContent( + @TestParameter LazyMetadataType metadataType, @TestParameter boolean setContentsProxy) throws Exception { - // Test that if injected remote artifacts and local files are identical, the generating actions + // Test that if injected lazy artifacts and local files are identical, the generating actions // are not marked as dirty if it has contents proxy. SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 1); @@ -1479,10 +1499,10 @@ public void testRemoteAndLocalArtifacts_identicalContent(@TestParameter boolean Artifact out1 = createDerivedArtifact("foo"); Artifact out2 = createDerivedArtifact("bar"); Map metadataToInject = new HashMap<>(); - var out1Metadata = createRemoteMetadata("foo-content"); + var out1Metadata = createMetadata(metadataType, "foo-content"); metadataToInject.put(actionKey1, actionValueWithMetadata(out1, out1Metadata)); metadataToInject.put( - actionKey2, actionValueWithMetadata(out2, createRemoteMetadata("bar-content"))); + actionKey2, actionValueWithMetadata(out2, createMetadata(metadataType, "bar-content"))); differencer.inject(metadataToInject); EvaluationContext evaluationContext =