diff options
3 files changed, 113 insertions, 3 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java index 41f71d3082..80e190b6ac 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java @@ -18,6 +18,7 @@ import com.google.common.base.Function; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.skyframe.CycleInfo; @@ -35,17 +36,30 @@ public class SkylarkModuleCycleReporter implements CyclesReporter.SingleCycleRep private static final Predicate<SkyKey> IS_PACKAGE_SKY_KEY = SkyFunctions.isSkyFunction(SkyFunctions.PACKAGE); + private static final Predicate<SkyKey> IS_PACKAGE_LOOKUP = + SkyFunctions.isSkyFunction(SkyFunctions.PACKAGE_LOOKUP); + + private static final Predicate<SkyKey> IS_WORKSPACE_FILE = + SkyFunctions.isSkyFunction(SkyFunctions.WORKSPACE_FILE); + + private static final Predicate<SkyKey> IS_REPOSITORY_DIRECTORY = + SkyFunctions.isSkyFunction(SkyFunctions.REPOSITORY_DIRECTORY); + + private static final Predicate<SkyKey> IS_AST_FILE_LOOKUP = + SkyFunctions.isSkyFunction(SkyFunctions.AST_FILE_LOOKUP); + @Override public boolean maybeReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo, boolean alreadyReported, EventHandler eventHandler) { ImmutableList<SkyKey> pathToCycle = cycleInfo.getPathToCycle(); + ImmutableList<SkyKey> cycle = cycleInfo.getCycle(); if (pathToCycle.isEmpty()) { return false; } - SkyKey lastPathElement = cycleInfo.getPathToCycle().get(pathToCycle.size() - 1); + SkyKey lastPathElement = pathToCycle.get(pathToCycle.size() - 1); if (alreadyReported) { return true; - } else if (Iterables.all(cycleInfo.getCycle(), IS_SKYLARK_MODULE_SKY_KEY) + } else if (Iterables.all(cycle, IS_SKYLARK_MODULE_SKY_KEY) // The last element of the path to the cycle has to be a PackageFunction. && IS_PACKAGE_SKY_KEY.apply(lastPathElement)) { StringBuilder cycleMessage = @@ -63,11 +77,23 @@ public class SkylarkModuleCycleReporter implements CyclesReporter.SingleCycleRep .importLabel.toString(); } }); - // TODO(bazel-team): it would be nice to pass the Location of the load Statement in the // BUILD file. eventHandler.handle(Event.error(null, cycleMessage.toString())); return true; + } else if (Iterables.any(cycle, IS_PACKAGE_LOOKUP) && Iterables.any(cycle, IS_WORKSPACE_FILE) + && (IS_REPOSITORY_DIRECTORY.apply(lastPathElement) + || IS_PACKAGE_SKY_KEY.apply(lastPathElement))) { + // We have a cycle in the workspace file, report as such. + Label fileLabel = + (Label) Iterables.getLast(Iterables.filter(cycle, IS_AST_FILE_LOOKUP)).argument(); + String repositoryName = fileLabel.getPackageIdentifier().getRepository().strippedName(); + eventHandler.handle(Event.error(null, + "Failed to load Skylark extension '" + fileLabel.toString() + "'.\n" + + "It usually happens when the repository is not defined prior to being used.\n" + + "Maybe repository '" + repositoryName + + "' was defined later in your WORKSPACE file?")); + return true; } return false; } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java index 8f692515b6..a7ee7213bb 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.bazel.repository.skylark; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -24,6 +25,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.config.ConfigurationFactory; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.util.MockCcSupport; import com.google.devtools.build.lib.packages.util.MockToolsConfig; import com.google.devtools.build.lib.rules.repository.LocalRepositoryFunction; @@ -250,4 +252,52 @@ public class SkylarkRepositoryIntegrationTest extends BuildViewTestCase { Object path = target.getTarget().getAssociatedRule().getAttributeContainer().getAttr("path"); assertThat(path).isEqualTo("foobar"); } + + @Test + public void testCycleErrorWhenCallingRandomTarget() throws Exception { + reporter.removeHandler(failFastHandler); + scratch.file("/repo2/data.txt", "data"); + scratch.file("/repo2/BUILD", "exports_files_(['data.txt'])"); + scratch.file("/repo2/def.bzl", "def macro():", " print('bleh')"); + scratch.file("/repo2/WORKSPACE"); + scratch.overwriteFile( + rootDirectory.getRelative("WORKSPACE").getPathString(), + "load('@foo//:def.bzl', 'repo')", + "repo(name='foobar')", + "local_repository(name='foo', path='/repo2')"); + try { + invalidatePackages(); + getTarget("@foobar//:data.txt"); + fail(); + } catch (BuildFileContainsErrorsException e) { + // This is expected + } + assertDoesNotContainEvent("cycle"); + assertContainsEvent("Maybe repository 'foo' was defined later in your WORKSPACE file?"); + assertContainsEvent("Failed to load Skylark extension '@foo//:def.bzl'."); + } + + @Test + public void testCycleErrorWhenCallingCycleTarget() throws Exception { + reporter.removeHandler(failFastHandler); + scratch.file("/repo2/data.txt", "data"); + scratch.file("/repo2/BUILD", "exports_files_(['data.txt'])"); + scratch.file("/repo2/def.bzl", "def macro():", " print('bleh')"); + scratch.file("/repo2/WORKSPACE"); + scratch.overwriteFile( + rootDirectory.getRelative("WORKSPACE").getPathString(), + "load('@foo//:def.bzl', 'repo')", + "repo(name='foobar')", + "local_repository(name='foo', path='/repo2')"); + try { + invalidatePackages(); + getTarget("@foo//:data.txt"); + fail(); + } catch (BuildFileContainsErrorsException e) { + // This is expected + } + assertDoesNotContainEvent("cycle"); + assertContainsEvent("Maybe repository 'foo' was defined later in your WORKSPACE file?"); + assertContainsEvent("Failed to load Skylark extension '@foo//:def.bzl'."); + } } diff --git a/src/test/shell/bazel/skylark_repository_test.sh b/src/test/shell/bazel/skylark_repository_test.sh index 283bf6bc39..734a56caa2 100755 --- a/src/test/shell/bazel/skylark_repository_test.sh +++ b/src/test/shell/bazel/skylark_repository_test.sh @@ -216,6 +216,40 @@ EOF expect_log "Tra-la!" } +# Test cycle when loading a repository with a load statement in the WORKSPACE file that is not +# yet defined. +function test_cycle_load_repository() { + create_new_workspace + repo2=$new_workspace_dir + + echo "Tra-la!" > data.txt + cat <<'EOF' >BUILD +exports_files(["data.txt"]) +EOF + + cat <<'EOF' >ext.bzl +def macro(): + print('bleh') +EOF + + cat >WORKSPACE + + cd ${WORKSPACE_DIR} + cat > WORKSPACE <<EOF +load("@foo//:ext.bzl", "macro") +macro() +local_repository(name='foo', path='$repo2') +EOF + + local exitCode=0 + bazel build @foo//:data.txt >& $TEST_log || exitCode=$? + [ $exitCode != 0 ] || fail "building @foo//:data.txt succeed while expected failure" + + expect_not_log "PACKAGE" + expect_log "Failed to load Skylark extension '@foo//:ext.bzl'" + expect_log "Maybe repository 'foo' was defined later in your WORKSPACE file?" +} + function test_skylark_local_repository() { create_new_workspace repo2=$new_workspace_dir |