Skip to content

Commit

Permalink
remote: set executable bit of an input file based on its real value.
Browse files Browse the repository at this point in the history
When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider.

The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
  • Loading branch information
coeuvre committed Jan 13, 2021
1 parent 28fa193 commit 859e1c4
Show file tree
Hide file tree
Showing 11 changed files with 94 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ public boolean dataIsShareable() {
}

/** Metadata for remotely stored files. */
public static final class RemoteFileArtifactValue extends FileArtifactValue {
public static class RemoteFileArtifactValue extends FileArtifactValue {
private final byte[] digest;
private final long size;
private final int locationIndex;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.DirectoryMetadata;
import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.FileMetadata;
import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.SymlinkMetadata;
import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
import com.google.devtools.build.lib.remote.options.RemoteOptions;
Expand Down Expand Up @@ -665,11 +666,12 @@ private void injectRemoteArtifact(
}
metadataInjector.injectFile(
output,
new RemoteFileArtifactValue(
new RemoteActionFileArtifactValue(
DigestUtil.toBinaryDigest(outputMetadata.digest()),
outputMetadata.digest().getSizeBytes(),
/*locationIndex=*/ 1,
actionId));
actionId,
outputMetadata.isExecutable()));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ java_library(
name = "common",
srcs = glob(["*.java"]),
deps = [
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//third_party:guava",
"//third_party/protobuf:protobuf_java",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2020 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.remote.common;

import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;

/** A {@link RemoteFileArtifactValue} with more information added */
public class RemoteActionFileArtifactValue extends RemoteFileArtifactValue {

private final boolean isExecutable;

public RemoteActionFileArtifactValue(
byte[] digest, long size, int locationIndex, String actionId, boolean isExecutable) {
super(digest, size, locationIndex, actionId);
this.isExecutable = isExecutable;
}

public boolean isExecutable() {
return isExecutable;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,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/profiler",
"//src/main/java/com/google/devtools/build/lib/remote/common",
"//src/main/java/com/google/devtools/build/lib/remote/util",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.protobuf.ByteString;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -72,19 +73,22 @@ static class FileNode extends Node {
private final Path path;
private final ByteString data;
private final Digest digest;
private final boolean isExecutable;

FileNode(String pathSegment, Path path, Digest digest) {
FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
super(pathSegment);
this.path = Preconditions.checkNotNull(path, "path");
this.data = null;
this.digest = Preconditions.checkNotNull(digest, "digest");
this.isExecutable = isExecutable;
}

FileNode(String pathSegment, ByteString data, Digest digest) {
super(pathSegment);
this.path = null;
this.data = Preconditions.checkNotNull(data, "data");
this.digest = Preconditions.checkNotNull(digest, "digest");
this.isExecutable = false;
}

Digest getDigest() {
Expand All @@ -99,6 +103,10 @@ ByteString getBytes() {
return data;
}

public boolean isExecutable() {
return isExecutable;
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), path, data, digest);
Expand Down Expand Up @@ -165,7 +173,7 @@ boolean isEmpty() {
}

/**
* Traverses the {@link ActionInputsTree} in a depth first search manner. The children are visited
* Traverses the {@link DirectoryTree} in a depth first search manner. The children are visited
* in lexographical order.
*/
void visit(Visitor visitor) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue;
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode;
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode;
import com.google.devtools.build.lib.remote.util.DigestUtil;
Expand Down Expand Up @@ -101,7 +102,7 @@ private static int buildFromPaths(
throw new IOException(String.format("Input '%s' is not a file.", input));
}
Digest d = digestUtil.compute(input);
currDir.addChild(new FileNode(path.getBaseName(), input, d));
currDir.addChild(new FileNode(path.getBaseName(), input, d, input.isExecutable()));
return 1;
});
}
Expand Down Expand Up @@ -139,9 +140,16 @@ private static int buildFromActionInputs(
switch (metadata.getType()) {
case REGULAR_FILE:
Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
currDir.addChild(
new FileNode(
path.getBaseName(), ActionInputHelper.toInputPath(input, execRoot), d));
Path inputPath = ActionInputHelper.toInputPath(input, execRoot);

boolean isExecutable;
if (metadata instanceof RemoteActionFileArtifactValue) {
isExecutable = ((RemoteActionFileArtifactValue) metadata).isExecutable();
} else {
isExecutable = inputPath.isExecutable();
}

currDir.addChild(new FileNode(path.getBaseName(), inputPath, d, isExecutable));
return 1;

case DIRECTORY:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ private static FileNode buildProto(DirectoryTree.FileNode file) {
return FileNode.newBuilder()
.setName(file.getPathSegment())
.setDigest(file.getDigest())
.setIsExecutable(true)
.setIsExecutable(file.isExecutable())
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void virtualActionInputShouldWork() throws Exception {
assertThat(directoriesAtDepth(1, tree)).isEmpty();

FileNode expectedFooNode =
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
FileNode expectedBarNode =
new FileNode("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"));
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
Expand Down Expand Up @@ -117,13 +117,13 @@ public void directoryInputShouldBeExpanded() throws Exception {
assertThat(directoriesAtDepth(3, tree)).isEmpty();

FileNode expectedFooNode =
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
FileNode expectedBarNode =
new FileNode(
"bar.cc", execRoot.getRelative(bar.getExecPath()), digestUtil.computeAsUtf8("bar"));
"bar.cc", execRoot.getRelative(bar.getExecPath()), digestUtil.computeAsUtf8("bar"), false);
FileNode expectedBuzzNode =
new FileNode(
"buzz.cc", execRoot.getRelative(buzz.getExecPath()), digestUtil.computeAsUtf8("buzz"));
"buzz.cc", execRoot.getRelative(buzz.getExecPath()), digestUtil.computeAsUtf8("buzz"), false);
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode);
assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBarNode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,10 @@ public void buildingATreeOfFilesShouldWork() throws Exception {
assertThat(directoriesAtDepth(1, tree)).containsExactly("fizz");
assertThat(directoriesAtDepth(2, tree)).isEmpty();

FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo"));
FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"));
FileNode expectedBuzzNode = new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"));
FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo"), false);
FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"), false);
FileNode expectedBuzzNode =
new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"), false);
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode);
assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBuzzNode);
Expand Down
24 changes: 24 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2159,6 +2159,28 @@ EOF
@local_foo//:all
}

function test_remote_input_files_executable_bit() {
# Test that input files uploaded to remote executor have the same executable bit with local files. #12818
touch WORKSPACE
cat > BUILD <<'EOF'
genrule(
name = "test",
srcs = ["foo.txt", "bar.exe"],
outs = ["out.txt"],
cmd = "ls -l $(SRCS); touch $@",
)
EOF
touch foo.txt bar.exe
chmod a+x bar.exe

bazel build \
--remote_executor=grpc://localhost:${worker_port} \
//:test >& $TEST_log || fail "Failed to build //:test"

expect_log "-rwxr--r-- .* bar.exe"
expect_log "-rw-r--r-- .* foo.txt"
}

function test_exclusive_tag() {
# Test that the exclusive tag works with the remote cache.
mkdir -p a
Expand Down Expand Up @@ -2374,3 +2396,5 @@ end_of_record"
}

run_suite "Remote execution and remote cache tests"

}

0 comments on commit 859e1c4

Please sign in to comment.