Skip to content

Commit

Permalink
Make the attr new_local_repository.build_file label-typed
Browse files Browse the repository at this point in the history 
The attribute is string-typed to support a legacy use case where a path instead of a label could be passed. This causes us to parse a passed label as a canonical label, forgoing repo mapping altogether.

Fixes #19963.

RELNOTES[INC]: The attribute `new_local_repository.build_file` no longer accepts a path; a label must be passed instead.

Closes #19992.

PiperOrigin-RevId: 579188677
Change-Id: I2e2ef7c6423d5dbba1022ab4b3a2537202a9e724
  • Loading branch information
@Wyverald @copybara-github
Wyverald authored and copybara-github committed Nov 3, 2023
1 parent e9f0296 commit 76d71d9
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 117 deletions.
5 changes: 2 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
],
)
Expand Down
26 changes: 8 additions & 18 deletions src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -173,7 +175,7 @@ public Class<? extends RuleDefinition> 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<String, Object> origAttr =
Expand All @@ -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) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
108 changes: 32 additions & 76 deletions src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -73,13 +72,11 @@ public void finishFile(Rule rule, Path outputDirectory, Map<String, String> 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;
}

Expand Down Expand Up @@ -147,14 +144,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) {
Expand All @@ -167,75 +157,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 {
Expand All @@ -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);
}

Expand All @@ -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
Expand All @@ -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
Expand Down
25 changes: 9 additions & 16 deletions src/test/shell/bazel/local_repository_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -462,7 +453,7 @@ EOF
new_local_repository(
name = "bar",
path = "$bar",
build_file = "BUILD",
build_file = "//:BUILD",
)
EOF
touch BUILD
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 76d71d9

Please sign in to comment.