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 925dfcc790291a..49b53d7d43bde8 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -54,6 +54,7 @@ java_library( "LocalHostResourceManagerDarwin.java", "LocalHostResourceFallback.java", "MiddlemanType.java", + "RemoteFileStatus.java", "ResourceManager.java", "ResourceSet.java", "ResourceSetOrBuilder.java", @@ -263,6 +264,7 @@ java_library( "FileStateType.java", "FileStateValue.java", "FileValue.java", + "RemoteFileStatus.java", "cache/MetadataDigestUtils.java", ], deps = [ diff --git a/src/main/java/com/google/devtools/build/lib/actions/RemoteFileStatus.java b/src/main/java/com/google/devtools/build/lib/actions/RemoteFileStatus.java new file mode 100644 index 00000000000000..73184b55e7cc30 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/RemoteFileStatus.java @@ -0,0 +1,28 @@ +// 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.actions; + +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.vfs.FileStatusWithDigest; + +/** + * A FileStatus that exists remotely and provides remote metadata. + * + *

Filesystem may return implementation of this interface if the files are stored remotely. When + * checking outputs of actions, Skyframe will inject the metadata returned by {@link + * #getRemoteMetadata()} if the output file has {@link RemoteFileStatus}. + */ +public interface RemoteFileStatus extends FileStatusWithDigest { + RemoteFileArtifactValue getRemoteMetadata(); +} 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 bc5b6aba384cce..add5b366d436f6 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 @@ -40,6 +40,7 @@ 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.actions.RemoteFileStatus; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.inmemoryfs.FileInfo; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryContentInfo; @@ -128,41 +129,7 @@ void flush() throws IOException { PathFragment path = execRoot.getRelative(entry.getKey()); Artifact output = entry.getValue(); - if (maybeInjectMetadataForSymlink(path, output)) { - continue; - } - - if (output.isTreeArtifact()) { - if (remoteOutputTree.exists(path)) { - SpecialArtifact parent = (SpecialArtifact) output; - TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent); - - // TODO: Check directory content on the local fs to support mixed tree. - TreeArtifactValue.visitTree( - remoteOutputTree.getPath(path), - (parentRelativePath, type) -> { - if (type == Dirent.Type.DIRECTORY) { - return; - } - RemoteFileInfo remoteFile = - remoteOutputTree.getRemoteFileInfo( - path.getRelative(parentRelativePath), /* followSymlinks= */ true); - if (remoteFile != null) { - TreeFileArtifact child = - TreeFileArtifact.createTreeOutput(parent, parentRelativePath); - tree.putChild(child, createRemoteMetadata(remoteFile)); - } - }); - - metadataInjector.injectTree(parent, tree.build()); - } - } else { - RemoteFileInfo remoteFile = - remoteOutputTree.getRemoteFileInfo(path, /* followSymlinks= */ true); - if (remoteFile != null) { - metadataInjector.injectFile(output, createRemoteMetadata(remoteFile)); - } - } + maybeInjectMetadataForSymlink(path, output); } } @@ -183,26 +150,24 @@ void flush() throws IOException { * the output should be materialized as a symlink to the original location, which avoids * fetching multiple copies when multiple symlinks to the same artifact are created in the * same build. - * - * @return Whether the metadata was injected. */ - private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact output) + private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact output) throws IOException { if (output.isSymlink()) { - return false; + return; } Path outputTreePath = remoteOutputTree.getPath(linkPath); if (!outputTreePath.exists(Symlinks.NOFOLLOW)) { - return false; + return; } PathFragment targetPath; try { targetPath = outputTreePath.readSymbolicLink(); } catch (NotASymlinkException e) { - return false; + return; } checkState( @@ -212,7 +177,7 @@ private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact ou if (output.isTreeArtifact()) { TreeArtifactValue metadata = getRemoteTreeMetadata(targetPath); if (metadata == null) { - return false; + return; } SpecialArtifact parent = (SpecialArtifact) output; @@ -233,7 +198,7 @@ private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact ou } else { RemoteFileArtifactValue metadata = getRemoteMetadata(targetPath); if (metadata == null) { - return false; + return; } RemoteFileArtifactValue injectedMetadata = @@ -247,8 +212,6 @@ private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact ou metadataInjector.injectFile(output, injectedMetadata); } - - return true; } private RemoteFileArtifactValue createRemoteMetadata(RemoteFileInfo remoteFile) { @@ -498,7 +461,12 @@ protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOEx } private static FileStatus statFromRemoteMetadata(RemoteFileArtifactValue m) { - return new FileStatus() { + return new RemoteFileStatus() { + @Override + public byte[] getDigest() { + return m.getDigest(); + } + @Override public boolean isFile() { return m.getType().isFile(); @@ -538,6 +506,11 @@ public long getLastChangeTime() { public long getNodeId() { throw new UnsupportedOperationException("Cannot get node id for " + m); } + + @Override + public RemoteFileArtifactValue getRemoteMetadata() { + return m; + } }; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 77104d996b1ee3..171c664f439ead 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -47,6 +47,7 @@ import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.actions.RemoteFileStatus; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.XattrProvider; @@ -656,13 +657,19 @@ private static FileArtifactValue fileArtifactValueFromStat( return FileArtifactValue.MISSING_FILE_MARKER; } + if (stat.isDirectory()) { + return FileArtifactValue.createForDirectoryWithMtime(stat.getLastModifiedTime()); + } + + if (stat instanceof RemoteFileStatus) { + return ((RemoteFileStatus) stat).getRemoteMetadata(); + } + FileStateValue fileStateValue = FileStateValue.createWithStatNoFollow( rootedPath, stat, digestWillBeInjected, xattrProvider, tsgm); - return stat.isDirectory() - ? FileArtifactValue.createForDirectoryWithMtime(stat.getLastModifiedTime()) - : FileArtifactValue.createForNormalFile( + return FileArtifactValue.createForNormalFile( fileStateValue.getDigest(), fileStateValue.getContentsProxy(), stat.getSize()); } 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 ade581f83941c3..b5089d85cb8c69 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 @@ -439,6 +439,36 @@ public void downloadToplevel_treeArtifacts() throws Exception { assertValidOutputFile("foo/file-3", "file-3\n"); } + @Test + public void treeOutputsFromLocalFileSystem_works() throws Exception { + // Test that tree artifact generated locally can be consumed by other actions. + // See https://github.com/bazelbuild/bazel/issues/16789 + + // Disable remote execution so tree outputs are generated locally + addOptions("--modify_execution_info=OutputDir=+no-remote-exec"); + setDownloadToplevel(); + writeOutputDirRule(); + write( + "BUILD", + "load(':output_dir.bzl', 'output_dir')", + "output_dir(", + " name = 'foo',", + " manifest = ':manifest',", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'cat $(location :foo)/file-1 > $@ && echo bar >> $@',", + ")"); + write("manifest", "file-1"); + + buildTarget("//:foobar"); + waitDownloads(); + + assertValidOutputFile("out/foobar.txt", "file-1\nbar\n"); + } + @Test public void incrementalBuild_deleteOutputsInUnwritableParentDirectory() throws Exception { write( @@ -695,6 +725,7 @@ protected void writeOutputDirRule() throws IOException { "def _output_dir_impl(ctx):", " output_dir = ctx.actions.declare_directory(ctx.attr.name)", " ctx.actions.run_shell(", + " mnemonic = 'OutputDir',", " inputs = [ctx.file.manifest],", " outputs = [output_dir],", " arguments = [ctx.file.manifest.path, output_dir.path],",