Make the attr new_local_repository.build_file label-typed

src/main/java/com/google/devtools/build/lib/rules/BUILD

@@ -315,6 +315,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
         "//src/main/java/com/google/devtools/build/lib/bazel:resolved_event",
         "//src/main/java/com/google/devtools/build/lib/bugreport",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_value",
@@ -350,6 +351,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator",
         "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function",
         "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",

src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java

@@ -21,7 +21,9 @@
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.bazel.ResolvedEvent;
 import com.google.devtools.build.lib.bugreport.BugReport;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.vfs.FileSystem;
@@ -186,22 +188,10 @@ private static ResolvedEvent resolve(Rule rule, BlazeDirectories directories) {
             .append(", path = ")
             .append(Starlark.repr(pathObj));
 
-    Object buildFileObj = rule.getAttr("build_file");
-    if ((buildFileObj instanceof String) && !((String) buildFileObj).isEmpty()) {
-      // Build files might refer to an embedded file (as they to for "local_jdk"), so we have to
-      // describe the argument in a portable way.
-      origAttr.put("build_file", buildFileObj);
-      String buildFileArg;
-      PathFragment pathFragment = PathFragment.create((String) buildFileObj);
-      PathFragment embeddedDir = directories.getEmbeddedBinariesRoot().asFragment();
-      if (pathFragment.isAbsolute() && pathFragment.startsWith(embeddedDir)) {
-        buildFileArg =
-            "__embedded_dir__ + \"/\" + "
-                + Starlark.repr(pathFragment.relativeTo(embeddedDir).toString());
-      } else {
-        buildFileArg = Starlark.repr(buildFileObj.toString());
-      }
-      repr.append(", build_file = ").append(buildFileArg);
+    Label buildFile = (Label) rule.getAttr("build_file", BuildType.NODEP_LABEL);
+    if (buildFile != null) {
+      origAttr.put("build_file", buildFile);
+      repr.append(", build_file = ").append(Starlark.repr(buildFile));
     } else {
       Object buildFileContentObj = rule.getAttr("build_file_content");
       if (buildFileContentObj != null) {

src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryRule.java

@@ -19,6 +19,7 @@
 
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
+import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
 
@@ -46,7 +47,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
         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))
+        .add(attr("build_file", BuildType.NODEP_LABEL))
         /* <!-- #BLAZE_RULE(new_local_repository).ATTRIBUTE(build_file_content) -->
         The content for the BUILD file for this repository.
 
@@ -62,7 +63,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
         named WORKSPACE, but can be. (Something like WORKSPACE.new-repo-name may work well for
         distinguishing it from the repository's actual WORKSPACE files.)</p>
          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr("workspace_file", STRING))
+        .add(attr("workspace_file", BuildType.NODEP_LABEL))
         /* <!-- #BLAZE_RULE(new_local_repository).ATTRIBUTE(workspace_file_content) -->
         The content for the WORKSPACE file for this repository.
 

src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java

@@ -16,14 +16,13 @@
 
 import com.google.devtools.build.lib.actions.FileValue;
 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.BuildType;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
+import com.google.devtools.build.lib.skyframe.PackageLookupFunction;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue;
 import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
@@ -147,14 +146,7 @@ public void finishFile(Rule rule, Path outputDirectory, Map<String, String> mark
       if (fileValue != null) {
         // Link x/FILENAME to <build_root>/x.FILENAME.
         symlinkFile(fileValue, filename, outputDirectory);
-        String fileAttribute = getFileAttributeValue(rule);
-        String fileKey;
-        if (LabelValidator.isAbsolute(fileAttribute)) {
-          fileKey = getFileAttributeAsLabel(rule).toString();
-        } else {
-          // TODO(pcloudy): Don't add absolute path into markerData once it's not supported
-          fileKey = fileValue.realRootedPath().asPath().getPathString();
-        }
+        String fileKey = getFileAttributeAsLabel(rule).toString();
         try {
           markerData.put("FILE:" + fileKey, RepositoryFunction.fileValueToMarkerValue(fileValue));
         } catch (IOException e) {
@@ -167,75 +159,37 @@ public void finishFile(Rule rule, Path outputDirectory, Map<String, String> mark
       }
     }
 
-    private String getFileAttributeValue(Rule rule) throws RepositoryFunctionException {
-      WorkspaceAttributeMapper mapper = WorkspaceAttributeMapper.of(rule);
-      String fileAttribute;
+    private Label getFileAttributeAsLabel(Rule rule) throws RepositoryFunctionException {
       try {
-        fileAttribute = mapper.get(getFileAttrName(), Type.STRING);
+        return WorkspaceAttributeMapper.of(rule).get(getFileAttrName(), BuildType.NODEP_LABEL);
       } catch (EvalException e) {
         throw new RepositoryFunctionException(e, Transience.PERSISTENT);
       }
-      return fileAttribute;
-    }
-
-    private Label getFileAttributeAsLabel(Rule rule) throws RepositoryFunctionException {
-      Label label;
-      try {
-        // Parse a label
-        label = Label.parseCanonical(getFileAttributeValue(rule));
-      } catch (LabelSyntaxException ex) {
-        throw new RepositoryFunctionException(
-            Starlark.errorf(
-                "the '%s' attribute does not specify a valid label: %s",
-                getFileAttrName(), ex.getMessage()),
-            Transience.PERSISTENT);
-      }
-      return label;
     }
 
     @Nullable
     private FileValue getFileValue(Rule rule, Environment env)
         throws RepositoryFunctionException, InterruptedException {
-      String fileAttribute = getFileAttributeValue(rule);
-      RootedPath rootedFile;
-
-      if (LabelValidator.isAbsolute(fileAttribute)) {
-        Label label = getFileAttributeAsLabel(rule);
-        SkyKey pkgSkyKey = PackageLookupValue.key(label.getPackageIdentifier());
-        PackageLookupValue pkgLookupValue = (PackageLookupValue) env.getValue(pkgSkyKey);
-        if (pkgLookupValue == null) {
-          return null;
-        }
-        if (!pkgLookupValue.packageExists()) {
-          throw new RepositoryFunctionException(
-              Starlark.errorf("Unable to load package for %s: not found.", fileAttribute),
-              Transience.PERSISTENT);
-        }
-
-        // And now for the file
-        Root packageRoot = pkgLookupValue.getRoot();
-        rootedFile = RootedPath.toRootedPath(packageRoot, label.toPathFragment());
-      } else {
-        // TODO(dmarting): deprecate using a path for the workspace_file attribute.
-        PathFragment file = PathFragment.create(fileAttribute);
-        Path fileTarget = workspacePath.getRelative(file);
-        if (!fileTarget.exists()) {
-          throw new RepositoryFunctionException(
-              Starlark.errorf(
-                  "the '%s' attribute does not specify an existing file (%s does not exist)",
-                  getFileAttrName(), fileTarget),
-              Transience.PERSISTENT);
-        }
-
-        if (file.isAbsolute()) {
-          rootedFile =
-              RootedPath.toRootedPath(
-                  Root.fromPath(fileTarget.getParentDirectory()),
-                  PathFragment.create(fileTarget.getBaseName()));
-        } else {
-          rootedFile = RootedPath.toRootedPath(Root.fromPath(workspacePath), file);
+      Label label = getFileAttributeAsLabel(rule);
+      SkyKey pkgSkyKey = PackageLookupValue.key(label.getPackageIdentifier());
+      PackageLookupValue pkgLookupValue = (PackageLookupValue) env.getValue(pkgSkyKey);
+      if (pkgLookupValue == null) {
+        return null;
+      }
+      if (!pkgLookupValue.packageExists()) {
+        String message = pkgLookupValue.getErrorMsg();
+        if (pkgLookupValue == PackageLookupValue.NO_BUILD_FILE_VALUE) {
+          message = PackageLookupFunction.explainNoBuildFileValue(label.getPackageIdentifier(),
+              env);
         }
+        throw new RepositoryFunctionException(
+            Starlark.errorf("Unable to load package for %s: %s", label, message),
+            Transience.PERSISTENT);
       }
+
+      // And now for the file
+      Root packageRoot = pkgLookupValue.getRoot();
+      RootedPath rootedFile = RootedPath.toRootedPath(packageRoot, label.toPathFragment());
       SkyKey fileKey = FileValue.key(rootedFile);
       FileValue fileValue;
       try {
@@ -251,13 +205,17 @@ private FileValue getFileValue(Rule rule, Environment env)
         }
       } catch (IOException e) {
         throw new RepositoryFunctionException(
-            new IOException("Cannot lookup " + fileAttribute + ": " + e.getMessage()),
+            new IOException("Cannot lookup " + label + ": " + e.getMessage()),
             Transience.TRANSIENT);
       }
 
       if (!fileValue.isFile() || fileValue.isSpecialFile()) {
         throw new RepositoryFunctionException(
-            Starlark.errorf("%s is not a regular file", rootedFile.asPath()),
+            Starlark.errorf(
+                "%s is not a regular file; if you're using a relative or absolute path for "
+                    + "`build_file` in your `new_local_repository` rule, please switch to using a "
+                    + "label instead",
+                rootedFile.asPath()),
             Transience.PERSISTENT);
       }
 

src/test/shell/bazel/local_repository_test.sh

@@ -192,10 +192,6 @@ 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"
 }
@@ -224,22 +220,17 @@ public class Mongoose {
 }
 EOF
 
-  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
+  if [ "$1" == "build_file" ] ; then
+    touch BUILD
     cat >> $(create_workspace_with_default_repos WORKSPACE) <<EOF
 new_local_repository(
     name = 'endangered',
     path = '$project_dir',
-    build_file = '$build_file',
+    build_file = '//:BUILD.carnivore',
 )
 EOF
 
-    cat > $build_file <<EOF
+    cat > BUILD.carnivore <<EOF
 java_library(
     name = "mongoose",
     srcs = ["carnivore/Mongoose.java"],
@@ -462,7 +453,7 @@ EOF
 new_local_repository(
     name = "bar",
     path = "$bar",
-    build_file = "BUILD",
+    build_file = "//:BUILD",
 )
 EOF
   touch BUILD
@@ -567,14 +558,15 @@ function test_overlaid_build_file() {
 new_local_repository(
     name = "mutant",
     path = "$mutant",
-    build_file = "mutant.BUILD"
+    build_file = "//:mutant.BUILD"
 )
 
 bind(
     name = "best-turtle",
     actual = "@mutant//:turtle",
 )
 EOF
+  touch BUILD
   cat > mutant.BUILD <<EOF
 genrule(
     name = "turtle",
@@ -1088,10 +1080,11 @@ EOF
 new_local_repository(
     name="r",
     path="$r",
-    build_file="BUILD.r"
+    build_file="//:BUILD.r"
 )
 EOF
 
+  touch BUILD
   cat > BUILD.r <<EOF
 cc_library(name = "a", srcs = ["a.cc"])
 EOF

src/test/shell/bazel/new_local_repo_test.sh

@@ -62,6 +62,25 @@ if "$is_windows"; then
   export MSYS2_ARG_CONV_EXCL="*"
 fi
 
+function test_build_file_label_repo_mapping() {
+  mkdir subdir
+  cat > WORKSPACE <<'eof'
+workspace(name='myws')
+# add a `load` to force a new workspace chunk, adding "myws" to the mapping
+load('@bazel_tools//tools/build_defs/repo:http.bzl', 'http_archive')
+new_local_repository(
+  name = "heh",
+  path = "subdir",
+  build_file = "@myws//:thing",
+)
+eof
+  touch BUILD
+  echo 'filegroup(name="a-ma-bob")' > thing
+  write_default_lockfile MODULE.bazel.lock
+
+  bazel build @heh//:a-ma-bob &> $TEST_log || fail "don't fail!"
+}
+
 # Regression test for GitHub issue #6351, see
 # https://github.com/bazelbuild/bazel/issues/6351#issuecomment-465488344
 function test_glob_in_synthesized_build_file() {
@@ -127,9 +146,10 @@ function test_recursive_glob_in_new_local_repository() {
 new_local_repository(
     name = "myext",
     path = "../B",
-    build_file = "BUILD.myext",
+    build_file = "//:BUILD.myext",
 )
 eof
+  touch "$pkg/A/BUILD.bazel"
   cat >"$pkg/A/BUILD.myext" <<eof
 filegroup(name = "all_files", srcs = glob(["**"]))
 eof

src/test/shell/bazel/runfiles_test.sh

@@ -84,7 +84,7 @@ function test_legacy_runfiles_change() {
 new_local_repository(
     name = "bar",
     path = ".",
-    build_file = "BUILD",
+    build_file = "//:BUILD",
 )
 EOF
   cat > BUILD <<EOF