diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index 6ed40c7b2d1046..c2c9fe435594cf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/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", @@ -348,15 +349,13 @@ java_library( ":repository/workspace_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//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/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/net/starlark/java/eval", - "//third_party:guava", "//third_party:jsr305", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java index 567f6f4a2f521f..e265e3c1685538 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java +++ b/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; @@ -160,7 +162,7 @@ public RepositoryDirectoryValue.Builder fetch( } fileHandler.finishFile(rule, outputDirectory, markerData); - env.getListener().post(resolve(rule, directories)); + env.getListener().post(resolve(rule)); return RepositoryDirectoryValue.builder() .setPath(outputDirectory) @@ -173,7 +175,7 @@ public Class getRuleDefinition() { return NewLocalRepositoryRule.class; } - private static ResolvedEvent resolve(Rule rule, BlazeDirectories directories) { + private static ResolvedEvent resolve(Rule rule) { String name = rule.getName(); Object pathObj = rule.getAttr("path"); ImmutableMap.Builder origAttr = @@ -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) { 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 index 211e7dfca1a4cc..c747bf37e38bcd 100644 --- 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 @@ -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.)

*/ - .add(attr("build_file", STRING)) + .add(attr("build_file", BuildType.NODEP_LABEL)) /* 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.)

*/ - .add(attr("workspace_file", STRING)) + .add(attr("workspace_file", BuildType.NODEP_LABEL)) /* The content for the WORKSPACE file for this repository. diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java index 49d18fae782c37..37c15b1c3fdf9e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java +++ b/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; @@ -73,13 +72,11 @@ public void finishFile(Rule rule, Path outputDirectory, Map mark */ private abstract static class BaseFileHandler { - private final Path workspacePath; private final String filename; private FileValue fileValue; private String fileContent; - private BaseFileHandler(Path workspacePath, String filename) { - this.workspacePath = workspacePath; + private BaseFileHandler(String filename) { this.filename = filename; } @@ -147,14 +144,7 @@ public void finishFile(Rule rule, Path outputDirectory, Map mark if (fileValue != null) { // Link x/FILENAME to /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 +157,37 @@ public void finishFile(Rule rule, Path outputDirectory, Map 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 +203,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); } @@ -284,7 +240,7 @@ private static void symlinkFile(FileValue fileValue, String filename, Path outpu public static class NewRepositoryWorkspaceFileHandler extends BaseFileHandler { public NewRepositoryWorkspaceFileHandler(Path workspacePath) { - super(workspacePath, "WORKSPACE"); + super("WORKSPACE"); } @Override @@ -310,7 +266,7 @@ protected String getDefaultContent(Rule rule) { public static class NewRepositoryBuildFileHandler extends BaseFileHandler { public NewRepositoryBuildFileHandler(Path workspacePath) { - super(workspacePath, "BUILD.bazel"); + super("BUILD.bazel"); } @Override diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh index 37debe6c24ded1..ea32f83b59b5a6 100755 --- a/src/test/shell/bazel/local_repository_test.sh +++ b/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) < $build_file < BUILD.carnivore < mutant.BUILD < BUILD.r < 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" < BUILD <