Skip to content

Commit d5ee3b5

Browse files
kchodorowvladmos
authored andcommitted
Repositories can only be accessed in projects that define them in their WORKSPACE file
This is prep for #1943 - hierarchical workspace loading. RELNOTES[INC]: Remote repositories must define any remote repositories they themselves use (e.g., if @x//:foo depends on @y//:bar, @y must be defined in @x's WORKSPACE file). PiperOrigin-RevId: 154295762
1 parent 4db102c commit d5ee3b5

File tree

8 files changed

+251
-3
lines changed

8 files changed

+251
-3
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
// Copyright 2017 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+
15+
package com.google.devtools.build.lib.rules.repository;
16+
17+
import com.google.auto.value.AutoValue;
18+
import com.google.devtools.build.lib.cmdline.Label;
19+
import com.google.devtools.build.lib.cmdline.RepositoryName;
20+
import com.google.devtools.build.lib.packages.Rule;
21+
import com.google.devtools.build.lib.skyframe.PackageLookupValue;
22+
import com.google.devtools.build.lib.skyframe.SkyFunctions;
23+
import com.google.devtools.build.lib.skyframe.WorkspaceFileValue;
24+
import com.google.devtools.build.lib.util.Preconditions;
25+
import com.google.devtools.build.lib.vfs.RootedPath;
26+
import com.google.devtools.build.skyframe.SkyFunction;
27+
import com.google.devtools.build.skyframe.SkyKey;
28+
import com.google.devtools.build.skyframe.SkyValue;
29+
import javax.annotation.Nullable;
30+
31+
/**
32+
* Used for repositories that are declared in other repositories' WORKSPACE files.
33+
*/
34+
public class RepositoryVisibilityFunction implements SkyFunction {
35+
public static SkyKey key(RepositoryName parent, RepositoryName child) {
36+
return SkyKey.create(SkyFunctions.REPOSITORY_DEPENDENCY, Key.create(parent, child));
37+
}
38+
39+
@Nullable
40+
@Override
41+
public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
42+
Key key = (Key) skyKey.argument();
43+
RepositoryDirectoryValue parentDir = (RepositoryDirectoryValue) env.getValue(
44+
RepositoryDirectoryValue.key(key.parent()));
45+
if (env.valuesMissing()) {
46+
return null;
47+
}
48+
49+
SkyKey parentWorkspaceKey;
50+
if (parentDir.repositoryExists()) {
51+
parentWorkspaceKey = WorkspaceFileValue.key(
52+
RootedPath.toRootedPath(parentDir.getPath(), Label.EXTERNAL_PACKAGE_FILE_NAME));
53+
} else {
54+
// This is kind of hacky: the main repository won't exist under output_base/external, so RDF
55+
// won't find it above. However, we know that the parent repository has to exist, so we'll
56+
// just assume it's the main repository if RDF couldn't find it.
57+
PackageLookupValue packageLookupValue = (PackageLookupValue) env.getValue(
58+
PackageLookupValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER));
59+
if (env.valuesMissing()) {
60+
return null;
61+
}
62+
Preconditions.checkState(packageLookupValue.packageExists());
63+
parentWorkspaceKey = WorkspaceFileValue.key(
64+
RootedPath.toRootedPath(packageLookupValue.getRoot(), Label.EXTERNAL_PACKAGE_FILE_NAME));
65+
}
66+
67+
// This just looks for the child repo name. It doesn't care if the name is used for a different
68+
// repository (either a different type or a different path/url/commit/whichever) than is
69+
// actually being used. For example, if someone has a copy of Boost on their system that
70+
// they're using but a library they're depending downloads Boost, we just want "everyone" to
71+
// use the local copy of Boost, not error out. This is the check that the library actually
72+
// declares a repository "dependency" on @boost.
73+
while (true) {
74+
WorkspaceFileValue parentWorkspace = (WorkspaceFileValue) env.getValue(parentWorkspaceKey);
75+
if (env.valuesMissing()) {
76+
return null;
77+
}
78+
79+
Rule child = parentWorkspace.getPackage().getRule(key.child().strippedName());
80+
if (child == null) {
81+
if (parentWorkspace.hasNext()) {
82+
parentWorkspaceKey = parentWorkspace.next();
83+
} else {
84+
break;
85+
}
86+
} else {
87+
return RepositoryVisibilityValue.OK;
88+
}
89+
}
90+
return RepositoryVisibilityValue.NOT_FOUND;
91+
}
92+
93+
@Nullable
94+
@Override
95+
public String extractTag(SkyKey skyKey) {
96+
return null;
97+
}
98+
99+
/**
100+
* Represents a parent repository and a child repository that we expect to be defined in the
101+
* parent's WORKSPACE file.
102+
*/
103+
@AutoValue
104+
public abstract static class Key {
105+
public static Key create(RepositoryName parent, RepositoryName child) {
106+
return new AutoValue_RepositoryVisibilityFunction_Key(parent, child);
107+
}
108+
109+
public abstract RepositoryName parent();
110+
public abstract RepositoryName child();
111+
}
112+
113+
/**
114+
* Returns if the repository definition was found or not.
115+
*/
116+
public static class RepositoryVisibilityValue implements SkyValue {
117+
private static RepositoryVisibilityValue OK = new RepositoryVisibilityValue();
118+
private static RepositoryVisibilityValue NOT_FOUND = new RepositoryVisibilityValue();
119+
120+
private RepositoryVisibilityValue() {
121+
}
122+
123+
public boolean ok() {
124+
return this == OK;
125+
}
126+
}
127+
}

src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java

+34
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.google.common.base.VerifyException;
2323
import com.google.common.collect.ImmutableList;
2424
import com.google.common.collect.ImmutableMap;
25+
import com.google.common.collect.ImmutableSet;
2526
import com.google.common.collect.Iterables;
2627
import com.google.common.collect.LinkedHashMultimap;
2728
import com.google.common.collect.LinkedListMultimap;
@@ -50,6 +51,7 @@
5051
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
5152
import com.google.devtools.build.lib.analysis.config.PatchTransition;
5253
import com.google.devtools.build.lib.cmdline.Label;
54+
import com.google.devtools.build.lib.cmdline.RepositoryName;
5355
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
5456
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
5557
import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -69,6 +71,8 @@
6971
import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
7072
import com.google.devtools.build.lib.packages.Target;
7173
import com.google.devtools.build.lib.packages.TargetUtils;
74+
import com.google.devtools.build.lib.rules.repository.RepositoryVisibilityFunction;
75+
import com.google.devtools.build.lib.rules.repository.RepositoryVisibilityFunction.RepositoryVisibilityValue;
7276
import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException;
7377
import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
7478
import com.google.devtools.build.lib.skyframe.SkyframeExecutor.BuildViewProvider;
@@ -91,6 +95,7 @@
9195
import java.util.LinkedHashMap;
9296
import java.util.List;
9397
import java.util.Map;
98+
import java.util.Map.Entry;
9499
import java.util.Objects;
95100
import java.util.Set;
96101
import java.util.concurrent.Semaphore;
@@ -336,6 +341,35 @@ static OrderedSetMultimap<Attribute, ConfiguredTarget> computeDependencies(
336341
throw new DependencyEvaluationException(e);
337342
}
338343

344+
RepositoryName ctgRepository = ctgValue.getLabel().getPackageIdentifier().getRepository();
345+
ImmutableSet.Builder<SkyKey> pairsToCheck = ImmutableSet.builder();
346+
for (Dependency dep : depValueNames.values()) {
347+
RepositoryName depRepository = dep.getLabel().getPackageIdentifier().getRepository();
348+
if (ctgRepository.equals(depRepository) || depRepository.isMain()) {
349+
continue;
350+
}
351+
pairsToCheck.add(RepositoryVisibilityFunction.key(ctgRepository, depRepository));
352+
}
353+
Map<SkyKey, SkyValue> pairs = env.getValues(pairsToCheck.build());
354+
if (env.valuesMissing()) {
355+
return null;
356+
}
357+
boolean hadError = false;
358+
for (Entry<SkyKey, SkyValue> entry : pairs.entrySet()) {
359+
if (!((RepositoryVisibilityValue) entry.getValue()).ok()) {
360+
RepositoryVisibilityFunction.Key key =
361+
(RepositoryVisibilityFunction.Key) entry.getKey().argument();
362+
String message = ctgValue.getLabel() + " has a dependency on "
363+
+ key.child() + " but does not define " + key.child() + " in its WORKSPACE";
364+
env.getListener().handle(Event.error(message));
365+
hadError = true;
366+
}
367+
}
368+
if (hadError) {
369+
throw new DependencyEvaluationException(
370+
new ConfiguredValueCreationException("Missing external repository definitions"));
371+
}
372+
339373
// Trim each dep's configuration so it only includes the fragments needed by its transitive
340374
// closure (only dynamic configurations support this).
341375
if (useDynamicConfigurations(ctgValue.getConfiguration())) {

src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java

+2
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,8 @@ public final class SkyFunctions {
109109
SkyFunctionName.create("ACTION_TEMPLATE_EXPANSION");
110110
public static final SkyFunctionName LOCAL_REPOSITORY_LOOKUP =
111111
SkyFunctionName.create("LOCAL_REPOSITORY_LOOKUP");
112+
public static final SkyFunctionName REPOSITORY_DEPENDENCY =
113+
SkyFunctionName.create("REPOSITORY_DEPENDENCY");
112114

113115
public static Predicate<SkyKey> isSkyFunction(final SkyFunctionName functionName) {
114116
return new Predicate<SkyKey>() {

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

+2
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@
105105
import com.google.devtools.build.lib.pkgcache.TestFilter;
106106
import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
107107
import com.google.devtools.build.lib.profiler.AutoProfiler;
108+
import com.google.devtools.build.lib.rules.repository.RepositoryVisibilityFunction;
108109
import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
109110
import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.FileDirtinessChecker;
110111
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
@@ -446,6 +447,7 @@ private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions(
446447
SkyFunctions.ACTION_TEMPLATE_EXPANSION,
447448
new ActionTemplateExpansionFunction(removeActionsAfterEvaluation));
448449
map.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction());
450+
map.put(SkyFunctions.REPOSITORY_DEPENDENCY, new RepositoryVisibilityFunction());
449451
map.putAll(extraSkyFunctions);
450452
return map.build();
451453
}

src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java

+3-1
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,9 @@ public void testCcLibraryExternalIncludesNotWarned() throws Exception {
551551
new ModifiedFileSet.Builder().modify(PathFragment.create("WORKSPACE")).build(),
552552
rootDirectory);
553553
FileSystemUtils.createDirectoryAndParents(scratch.resolve("/foo/bar"));
554-
scratch.file("/foo/WORKSPACE", "workspace(name = 'pkg')");
554+
scratch.file("/foo/WORKSPACE",
555+
"workspace(name = 'pkg')",
556+
"local_repository(name = 'bazel_tools', path = '/whatever')");
555557
scratch.file(
556558
"/foo/bar/BUILD",
557559
"cc_library(name = 'lib',",

src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java

+3-1
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,9 @@ public void outputDirectoryForProtoCompileAction_externalRepos() throws Exceptio
149149
scratch.file(
150150
"x/BUILD", "cc_proto_library(name = 'foo_cc_proto', deps = ['@bla//foo:bar_proto'])");
151151

152-
scratch.file("/bla/WORKSPACE");
152+
scratch.file("/bla/WORKSPACE",
153+
"local_repository(name = 'com_google_protobuf', path = '/whatever')",
154+
"local_repository(name = 'com_google_protobuf_cc', path = '/whatever')");
153155
// Create the rule '@bla//foo:bar_proto'.
154156
scratch.file(
155157
"/bla/foo/BUILD",

src/test/shell/bazel/external_correctness_test.sh

+4-1
Original file line numberDiff line numberDiff line change
@@ -176,12 +176,15 @@ function test_refs_btwn_repos() {
176176
REMOTE1=$TEST_TMPDIR/remote1
177177
REMOTE2=$TEST_TMPDIR/remote2
178178
mkdir -p $REMOTE1 $REMOTE2
179-
touch $REMOTE1/WORKSPACE $REMOTE2/WORKSPACE
179+
touch $REMOTE1/WORKSPACE
180180
cat > $REMOTE1/input <<EOF
181181
1.0
182182
EOF
183183
cat > $REMOTE1/BUILD <<EOF
184184
exports_files(['input'])
185+
EOF
186+
cat > $REMOTE2/WORKSPACE <<EOF
187+
local_repository(name = "remote1", path = "/whatever")
185188
EOF
186189
cat > $REMOTE2/BUILD <<EOF
187190
genrule(

src/test/shell/bazel/workspace_test.sh

+76
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,9 @@ EOF
190190
touch foo/baz
191191
cat > bar/WORKSPACE <<EOF
192192
workspace(name = "bar")
193+
194+
# Needs to be defined, since @foo is referenced from the genrule.
195+
local_repository(name = "foo", path = "/whatever")
193196
EOF
194197
cat > bar/BUILD <<EOF
195198
genrule(
@@ -243,4 +246,77 @@ EOF
243246
assert_contains "override" bazel-genfiles/external/o/gen.out
244247
}
245248

249+
function test_direct_deps() {
250+
REPO1="$PWD/repo1"
251+
REPO2="$PWD/repo2"
252+
mkdir -p "$REPO1" "$REPO2"
253+
254+
# repo1 has dependencies on the main repo and repo2.
255+
cat > "$REPO1/WORKSPACE" <<EOF
256+
workspace(name = "repo1")
257+
258+
local_repository(name = "repo2", path = "/whatever")
259+
# Definition of main_repo purposely omitted to make build fail.
260+
EOF
261+
cat > "$REPO1/BUILD" << EOF
262+
genrule(
263+
name = "bar",
264+
srcs = ["@repo2//:baz", "@main_repo//:qux"],
265+
outs = ["bar.out"],
266+
cmd = "echo \$(SRCS) > \$@",
267+
visibility = ["//visibility:public"],
268+
)
269+
EOF
270+
271+
# repo2 has no dependencies.
272+
touch "$REPO2/WORKSPACE"
273+
cat > "$REPO2/BUILD" << EOF
274+
genrule(
275+
name = "baz",
276+
outs = ["baz.out"],
277+
cmd = "echo 'baz' > \$@",
278+
visibility = ["//visibility:public"],
279+
)
280+
EOF
281+
282+
# The main repo has dependencies on repo1.
283+
cat > WORKSPACE << EOF
284+
workspace(name = "main_repo")
285+
286+
local_repository(name = "repo1", path = "$REPO1")
287+
# TODO: move this to repo1/WORKSPACE once hierarchical workspaces work.
288+
local_repository(name = "repo2", path = "$REPO2")
289+
EOF
290+
cat > BUILD <<EOF
291+
exports_files(["qux"])
292+
genrule(
293+
name = "foo",
294+
srcs = ["@repo1//:bar"],
295+
outs = ["foo.out"],
296+
cmd = "echo 'hi' > \$@",
297+
)
298+
EOF
299+
touch qux
300+
301+
bazel build //:foo &> $TEST_log && fail "Expected missing main_repo"
302+
expect_log "@repo1//:bar has a dependency on @main_repo but does not define @main_repo in its WORKSPACE"
303+
304+
cat > "$REPO1/WORKSPACE" <<EOF
305+
workspace(name = "repo1")
306+
307+
local_repository(name = "main_repo", path = "/whatever")
308+
# Definition of repo2 purposely omitted to make build fail.
309+
EOF
310+
bazel build //:foo &> $TEST_log && fail "Expected missing repo2"
311+
expect_log "@repo1//:bar has a dependency on @repo2 but does not define @repo2 in its WORKSPACE"
312+
313+
cat > "$REPO1/WORKSPACE" <<EOF
314+
workspace(name = "repo1")
315+
316+
local_repository(name = "main_repo", path = "/whatever")
317+
local_repository(name = "repo2", path = "/whatever")
318+
EOF
319+
bazel build //:foo &> $TEST_log || fail "Expected success"
320+
}
321+
246322
run_suite "workspace tests"

0 commit comments

Comments
 (0)