Skip to content

Commit ceaac96

Browse files
coeuvrephilwo
coeuvre
authored and
philwo
committed
remote: set executable bit of an input file based on its real value
The "always mark" was introduced by 3e3b71a which was a workaround for #4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. #12818. Fixes #12818. Closes #12820. PiperOrigin-RevId: 351940694
1 parent c13cc8e commit ceaac96

File tree

12 files changed

+115
-28
lines changed

12 files changed

+115
-28
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,7 @@ public boolean dataIsShareable() {
551551
}
552552

553553
/** Metadata for remotely stored files. */
554-
public static final class RemoteFileArtifactValue extends FileArtifactValue {
554+
public static class RemoteFileArtifactValue extends FileArtifactValue {
555555
private final byte[] digest;
556556
private final long size;
557557
private final int locationIndex;

‎src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
4747
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
4848
import com.google.devtools.build.lib.actions.ExecException;
49-
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
5049
import com.google.devtools.build.lib.actions.UserExecException;
5150
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
5251
import com.google.devtools.build.lib.concurrent.ThreadSafety;
@@ -56,6 +55,7 @@
5655
import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.DirectoryMetadata;
5756
import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.FileMetadata;
5857
import com.google.devtools.build.lib.remote.RemoteCache.ActionResultMetadata.SymlinkMetadata;
58+
import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue;
5959
import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
6060
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
6161
import com.google.devtools.build.lib.remote.options.RemoteOptions;
@@ -647,12 +647,13 @@ private void injectRemoteArtifact(
647647
for (FileMetadata file : directory.files()) {
648648
TreeFileArtifact child =
649649
TreeFileArtifact.createTreeOutput(parent, file.path().relativeTo(parent.getPath()));
650-
RemoteFileArtifactValue value =
651-
new RemoteFileArtifactValue(
650+
RemoteActionFileArtifactValue value =
651+
new RemoteActionFileArtifactValue(
652652
DigestUtil.toBinaryDigest(file.digest()),
653653
file.digest().getSizeBytes(),
654654
/*locationIndex=*/ 1,
655-
actionId);
655+
actionId,
656+
file.isExecutable());
656657
tree.putChild(child, value);
657658
}
658659
metadataInjector.injectTree(parent, tree.build());
@@ -665,11 +666,12 @@ private void injectRemoteArtifact(
665666
}
666667
metadataInjector.injectFile(
667668
output,
668-
new RemoteFileArtifactValue(
669+
new RemoteActionFileArtifactValue(
669670
DigestUtil.toBinaryDigest(outputMetadata.digest()),
670671
outputMetadata.digest().getSizeBytes(),
671672
/*locationIndex=*/ 1,
672-
actionId));
673+
actionId,
674+
outputMetadata.isExecutable()));
673675
}
674676
}
675677

‎src/main/java/com/google/devtools/build/lib/remote/common/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ java_library(
1414
name = "common",
1515
srcs = glob(["*.java"]),
1616
deps = [
17+
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
1718
"//src/main/java/com/google/devtools/build/lib/vfs",
1819
"//third_party:guava",
1920
"//third_party/protobuf:protobuf_java",

‎src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionFileArtifactValue.java

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Copyright 2020 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.remote.common;
15+
16+
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
17+
18+
/**
19+
* A {@link RemoteFileArtifactValue} with additional data only available when using Remote Execution
20+
* API (e.g. {@code isExecutable}).
21+
*/
22+
public class RemoteActionFileArtifactValue extends RemoteFileArtifactValue {
23+
24+
private final boolean isExecutable;
25+
26+
public RemoteActionFileArtifactValue(
27+
byte[] digest, long size, int locationIndex, String actionId, boolean isExecutable) {
28+
super(digest, size, locationIndex, actionId);
29+
this.isExecutable = isExecutable;
30+
}
31+
32+
public boolean isExecutable() {
33+
return isExecutable;
34+
}
35+
}

‎src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ java_library(
1818
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
1919
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
2020
"//src/main/java/com/google/devtools/build/lib/profiler",
21+
"//src/main/java/com/google/devtools/build/lib/remote/common",
2122
"//src/main/java/com/google/devtools/build/lib/remote/util",
2223
"//src/main/java/com/google/devtools/build/lib/vfs",
2324
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",

‎src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,14 @@ static class FileNode extends Node {
7272
private final Path path;
7373
private final ByteString data;
7474
private final Digest digest;
75+
private final boolean isExecutable;
7576

76-
FileNode(String pathSegment, Path path, Digest digest) {
77+
FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
7778
super(pathSegment);
7879
this.path = Preconditions.checkNotNull(path, "path");
7980
this.data = null;
8081
this.digest = Preconditions.checkNotNull(digest, "digest");
82+
this.isExecutable = isExecutable;
8183
}
8284

8385
FileNode(String pathSegment, ByteString data, Digest digest) {
@@ -86,6 +88,7 @@ static class FileNode extends Node {
8688
this.data = Preconditions.checkNotNull(data, "data");
8789
;
8890
this.digest = Preconditions.checkNotNull(digest, "digest");
91+
this.isExecutable = false;
8992
}
9093

9194
Digest getDigest() {
@@ -100,6 +103,10 @@ ByteString getBytes() {
100103
return data;
101104
}
102105

106+
public boolean isExecutable() {
107+
return isExecutable;
108+
}
109+
103110
@Override
104111
public int hashCode() {
105112
return Objects.hash(super.hashCode(), path, data, digest);
@@ -166,8 +173,8 @@ boolean isEmpty() {
166173
}
167174

168175
/**
169-
* Traverses the {@link ActionInputsTree} in a depth first search manner. The children are visited
170-
* in lexographical order.
176+
* Traverses the {@link DirectoryTree} in a depth first search manner. The children are visited in
177+
* lexographical order.
171178
*/
172179
void visit(Visitor visitor) {
173180
Preconditions.checkNotNull(visitor, "visitor");

‎src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.devtools.build.lib.actions.FileArtifactValue;
2121
import com.google.devtools.build.lib.actions.MetadataProvider;
2222
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
23+
import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue;
2324
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode;
2425
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode;
2526
import com.google.devtools.build.lib.remote.util.DigestUtil;
@@ -101,7 +102,7 @@ private static int buildFromPaths(
101102
throw new IOException(String.format("Input '%s' is not a file.", input));
102103
}
103104
Digest d = digestUtil.compute(input);
104-
currDir.addChild(new FileNode(path.getBaseName(), input, d));
105+
currDir.addChild(new FileNode(path.getBaseName(), input, d, input.isExecutable()));
105106
return 1;
106107
});
107108
}
@@ -139,9 +140,16 @@ private static int buildFromActionInputs(
139140
switch (metadata.getType()) {
140141
case REGULAR_FILE:
141142
Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize());
142-
currDir.addChild(
143-
new FileNode(
144-
path.getBaseName(), ActionInputHelper.toInputPath(input, execRoot), d));
143+
Path inputPath = ActionInputHelper.toInputPath(input, execRoot);
144+
145+
boolean isExecutable;
146+
if (metadata instanceof RemoteActionFileArtifactValue) {
147+
isExecutable = ((RemoteActionFileArtifactValue) metadata).isExecutable();
148+
} else {
149+
isExecutable = inputPath.isExecutable();
150+
}
151+
152+
currDir.addChild(new FileNode(path.getBaseName(), inputPath, d, isExecutable));
145153
return 1;
146154

147155
case DIRECTORY:

‎src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ private static FileNode buildProto(DirectoryTree.FileNode file) {
198198
return FileNode.newBuilder()
199199
.setName(file.getPathSegment())
200200
.setDigest(file.getDigest())
201-
.setIsExecutable(true)
201+
.setIsExecutable(file.isExecutable())
202202
.build();
203203
}
204204

‎src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ public void virtualActionInputShouldWork() throws Exception {
7070
assertThat(directoriesAtDepth(1, tree)).isEmpty();
7171

7272
FileNode expectedFooNode =
73-
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
73+
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
7474
FileNode expectedBarNode =
7575
new FileNode("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"));
7676
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
@@ -117,13 +117,19 @@ public void directoryInputShouldBeExpanded() throws Exception {
117117
assertThat(directoriesAtDepth(3, tree)).isEmpty();
118118

119119
FileNode expectedFooNode =
120-
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"));
120+
new FileNode("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo"), false);
121121
FileNode expectedBarNode =
122122
new FileNode(
123-
"bar.cc", execRoot.getRelative(bar.getExecPath()), digestUtil.computeAsUtf8("bar"));
123+
"bar.cc",
124+
execRoot.getRelative(bar.getExecPath()),
125+
digestUtil.computeAsUtf8("bar"),
126+
false);
124127
FileNode expectedBuzzNode =
125128
new FileNode(
126-
"buzz.cc", execRoot.getRelative(buzz.getExecPath()), digestUtil.computeAsUtf8("buzz"));
129+
"buzz.cc",
130+
execRoot.getRelative(buzz.getExecPath()),
131+
digestUtil.computeAsUtf8("buzz"),
132+
false);
127133
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
128134
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode);
129135
assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBarNode);

‎src/test/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeTest.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,10 @@ public void buildingATreeOfFilesShouldWork() throws Exception {
7474
assertThat(directoriesAtDepth(1, tree)).containsExactly("fizz");
7575
assertThat(directoriesAtDepth(2, tree)).isEmpty();
7676

77-
FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo"));
78-
FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"));
79-
FileNode expectedBuzzNode = new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"));
77+
FileNode expectedFooNode = new FileNode("foo.cc", foo, digestUtil.computeAsUtf8("foo"), false);
78+
FileNode expectedBarNode = new FileNode("bar.cc", bar, digestUtil.computeAsUtf8("bar"), false);
79+
FileNode expectedBuzzNode =
80+
new FileNode("buzz.cc", buzz, digestUtil.computeAsUtf8("buzz"), false);
8081
assertThat(fileNodesAtDepth(tree, 0)).isEmpty();
8182
assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode);
8283
assertThat(fileNodesAtDepth(tree, 2)).containsExactly(expectedBuzzNode);

‎src/test/java/com/google/devtools/build/lib/remote/merkletree/MerkleTreeTest.java

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -87,13 +87,13 @@ public void buildMerkleTree() throws IOException {
8787

8888
Directory fizzDir =
8989
Directory.newBuilder()
90-
.addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz")))
91-
.addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz")))
90+
.addFiles(newFileNode("buzz.cc", digestUtil.computeAsUtf8("buzz"), false))
91+
.addFiles(newFileNode("fizzbuzz.cc", digestUtil.computeAsUtf8("fizzbuzz"), false))
9292
.build();
9393
Directory srcsDir =
9494
Directory.newBuilder()
95-
.addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar")))
96-
.addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo")))
95+
.addFiles(newFileNode("bar.cc", digestUtil.computeAsUtf8("bar"), false))
96+
.addFiles(newFileNode("foo.cc", digestUtil.computeAsUtf8("foo"), false))
9797
.addDirectories(
9898
DirectoryNode.newBuilder().setName("fizz").setDigest(digestUtil.compute(fizzDir)))
9999
.build();
@@ -153,7 +153,11 @@ private Artifact addFile(
153153
return a;
154154
}
155155

156-
private static FileNode newFileNode(String name, Digest digest) {
157-
return FileNode.newBuilder().setName(name).setDigest(digest).setIsExecutable(true).build();
156+
private static FileNode newFileNode(String name, Digest digest, boolean isExecutable) {
157+
return FileNode.newBuilder()
158+
.setName(name)
159+
.setDigest(digest)
160+
.setIsExecutable(isExecutable)
161+
.build();
158162
}
159163
}

‎src/test/shell/bazel/remote/remote_execution_test.sh

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2159,6 +2159,28 @@ EOF
21592159
@local_foo//:all
21602160
}
21612161

2162+
function test_remote_input_files_executable_bit() {
2163+
# Test that input files uploaded to remote executor have the same executable bit with local files. #12818
2164+
touch WORKSPACE
2165+
cat > BUILD <<'EOF'
2166+
genrule(
2167+
name = "test",
2168+
srcs = ["foo.txt", "bar.sh"],
2169+
outs = ["out.txt"],
2170+
cmd = "ls -l $(SRCS); touch $@",
2171+
)
2172+
EOF
2173+
touch foo.txt bar.sh
2174+
chmod a+x bar.sh
2175+
2176+
bazel build \
2177+
--remote_executor=grpc://localhost:${worker_port} \
2178+
//:test >& $TEST_log || fail "Failed to build //:test"
2179+
2180+
expect_log "-rwxr--r-- .* bar.sh"
2181+
expect_log "-rw-r--r-- .* foo.txt"
2182+
}
2183+
21622184
function test_exclusive_tag() {
21632185
# Test that the exclusive tag works with the remote cache.
21642186
mkdir -p a

0 commit comments

Comments
 (0)