Support labels in new_{http,local}_repository's build_file attribute

Fixes #855. -- MOS_MIGRATED_REVID=118711400
bazelbuild · Mar 31, 2016 · c7009b3 · c7009b3
1 parent cdb49c5
commit c7009b3
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 25 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/NewGitRepositoryRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/NewGitRepositoryRule.java
@@ -56,9 +56,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
 
         <p>Either build_file or build_file_content must be specified.</p>
 
-        <p>This path is relative to the build's workspace. The file does not need to be named
-        BUILD, but can be something like BUILD.new-repo-name to distinguish it
-        from the workspace's actual BUILD files.</p>
+        <p>This attribute is a label relative to the main workspace. The file does not need to be
+        named BUILD, but can be (something like BUILD.new-repo-name may work well for
+        distinguishing it from the repository's actual BUILD files.</p>
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("build_file", STRING))
         /* <!-- #BLAZE_RULE(new_git_repository).ATTRIBUTE(build_file_content) -->

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/NewHttpArchiveRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/NewHttpArchiveRule.java
@@ -53,9 +53,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
 
          <p>Either build_file or build_file_content must be specified.</p>
 
-         <p>This path is relative to the build's workspace. The file does not need to be named
-         BUILD, but can be something like BUILD.new-repo-name to distinguish it
-         from the workspace's actual BUILD files.</p>
+         <p>This attribute is a label relative to the main workspace. The file does not need to be
+        named BUILD, but can be (something like BUILD.new-repo-name may work well for
+        distinguishing it from the repository's actual BUILD files.</p>
          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("build_file", STRING))
         /* <!-- #BLAZE_RULE(new_http_archive).ATTRIBUTE(build_file_content) -->

diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryRule.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryRule.java
@@ -43,9 +43,9 @@ public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
 
         <p>Either build_file or build_file_content must be specified.</p>
 
-        <p>This path is relative to the build's workspace. The file does not need to be named
-        BUILD, but can be (something like BUILD.new-repo-name may work well for distinguishing it
-        from the repository's actual BUILD files.</p>
+        <p>This attribute is a label relative to the main workspace. The file does not need to be
+        named BUILD, but can be (something like BUILD.new-repo-name may work well for
+        distinguishing it from the repository's actual BUILD files.</p>
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("build_file", STRING))
         /* <!-- #BLAZE_RULE(new_local_repository).ATTRIBUTE(build_file_content) -->

diff --git a/...in/java/com/google/devtools/build/lib/rules/repository/NewRepositoryBuildFileHandler.java b/...in/java/com/google/devtools/build/lib/rules/repository/NewRepositoryBuildFileHandler.java
@@ -14,12 +14,16 @@
 
 package com.google.devtools.build.lib.rules.repository;
 
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.cmdline.LabelValidator;
 import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
 import com.google.devtools.build.lib.skyframe.FileSymlinkException;
 import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.vfs.Path;
@@ -113,22 +117,53 @@ public void finishBuildFile(Path outputDirectory) throws RepositoryFunctionExcep
   private FileValue getBuildFileValue(Rule rule, Environment env)
       throws RepositoryFunctionException {
     AggregatingAttributeMapper mapper = AggregatingAttributeMapper.of(rule);
-    PathFragment buildFile = new PathFragment(mapper.get("build_file", Type.STRING));
-    Path buildFileTarget = workspacePath.getRelative(buildFile);
-    if (!buildFileTarget.exists()) {
-      throw new RepositoryFunctionException(
-          new EvalException(rule.getLocation(),
-              String.format("In %s the 'build_file' attribute does not specify an existing file "
-                  + "(%s does not exist)", rule, buildFileTarget)),
-          Transience.PERSISTENT);
-    }
-
+    String buildFileAttribute = mapper.get("build_file", Type.STRING);
     RootedPath rootedBuild;
-    if (buildFile.isAbsolute()) {
-      rootedBuild = RootedPath.toRootedPath(
-          buildFileTarget.getParentDirectory(), new PathFragment(buildFileTarget.getBaseName()));
+
+    if (LabelValidator.isAbsolute(buildFileAttribute)) {
+      try {
+        // Parse a label
+        Label label = Label.parseAbsolute(buildFileAttribute);
+        SkyKey pkgSkyKey = PackageLookupValue.key(label.getPackageIdentifier());
+        PackageLookupValue pkgLookupValue = (PackageLookupValue) env.getValue(pkgSkyKey);
+        if (pkgLookupValue == null) {
+          return null;
+        }
+        if (!pkgLookupValue.packageExists()) {
+          throw new RepositoryFunctionException(
+              new EvalException(rule.getLocation(),
+                  "Unable to load package for " + buildFileAttribute + ": not found."),
+              Transience.PERSISTENT);
+        }
+
+        // And now for the file
+        Path packageRoot = pkgLookupValue.getRoot();
+        rootedBuild = RootedPath.toRootedPath(packageRoot, label.toPathFragment());
+      } catch (LabelSyntaxException ex) {
+        throw new RepositoryFunctionException(
+            new EvalException(rule.getLocation(),
+                String.format("In %s the 'build_file' attribute does not specify a valid label: %s",
+                    rule, ex.getMessage())),
+            Transience.PERSISTENT);
+      }
     } else {
-      rootedBuild = RootedPath.toRootedPath(workspacePath, buildFile);
+      // TODO(dmarting): deprecate using a path for the build_file attribute.
+      PathFragment buildFile = new PathFragment(buildFileAttribute);
+      Path buildFileTarget = workspacePath.getRelative(buildFile);
+      if (!buildFileTarget.exists()) {
+        throw new RepositoryFunctionException(
+            new EvalException(rule.getLocation(),
+                String.format("In %s the 'build_file' attribute does not specify an existing file "
+                    + "(%s does not exist)", rule, buildFileTarget)),
+            Transience.PERSISTENT);
+      }
+
+      if (buildFile.isAbsolute()) {
+        rootedBuild = RootedPath.toRootedPath(
+            buildFileTarget.getParentDirectory(), new PathFragment(buildFileTarget.getBaseName()));
+      } else {
+        rootedBuild = RootedPath.toRootedPath(workspacePath, buildFile);
+      }
     }
     SkyKey buildFileKey = FileValue.key(rootedBuild);
     FileValue buildFileValue;
@@ -146,7 +181,7 @@ private FileValue getBuildFileValue(Rule rule, Environment env)
       }
     } catch (IOException | FileSymlinkException | InconsistentFilesystemException e) {
       throw new RepositoryFunctionException(
-          new IOException("Cannot lookup " + buildFile + ": " + e.getMessage()),
+          new IOException("Cannot lookup " + buildFileAttribute + ": " + e.getMessage()),
           Transience.TRANSIENT);
     }
 

diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh
@@ -191,6 +191,10 @@ function test_new_local_repository_with_build_file() {
   do_new_local_repository_test "build_file"
 }
 
+function test_new_local_repository_with_labeled_build_file() {
+  do_new_local_repository_test "build_file+label"
+}
+
 function test_new_local_repository_with_build_file_content() {
   do_new_local_repository_test "build_file_content"
 }
@@ -218,8 +222,13 @@ public class Mongoose {
 }
 EOF
 
-  if [ "$1" == "build_file" ] ; then
+  if [ "$1" == "build_file" -o "$1" == "build_file+label" ] ; then
     build_file=BUILD.carnivore
+    build_file_str="${build_file}"
+    if [ "$1" == "build_file+label" ]; then
+      build_file_str="//:${build_file}"
+      cat > BUILD
+    fi
     cat > WORKSPACE <<EOF
 new_local_repository(
     name = 'endangered',