Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history 
Co-authored-by: Xùdōng Yáng <[email protected]>
  • Loading branch information
@fmeum @Wyverald
fmeum and Wyverald committed Sep 26, 2024
1 parent 8681209 commit a0d26f2
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 36 deletions.
8 changes: 2 additions & 6 deletions .../com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1351,13 +1351,9 @@ during the analysis phase and thus cannot depends on a target result (the \
"<code>string</code>, <code>Label</code> or <code>path</code> from which to create"
+ " a path from.")
})
public StarlarkPath path(Object path) throws EvalException, InterruptedException {
return getPath(path);
}

protected StarlarkPath getPath(Object path) throws EvalException, InterruptedException {
public StarlarkPath getPath(Object path) throws EvalException, InterruptedException {
return switch (path) {
case String ignored -> new StarlarkPath(this, workingDirectory.getRelative(path.toString()));
case String s -> new StarlarkPath(this, workingDirectory.getRelative(s));
case Label label -> getPathFromLabel(label);
case StarlarkPath starlarkPath -> starlarkPath;
// This can never happen because we check it in the Starlark interpreter.
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ public final class BuildLanguageOptions extends OptionsBase {
help =
"If true, then methods on <code>repository_ctx</code> that are passed a Label will no"
+ " longer automatically watch the file under that label for changes even if"
+ " <code>watch = \"no\"</code> and <code>repository_ctx.path</code> no longer causes"
+ " <code>watch = \"no\"</code>, and <code>repository_ctx.path</code> no longer causes"
+ " the returned path to be watched. Use <code>repository_ctx.watch</code> instead.")
public boolean incompatibleNoImplicitWatchLabel;

Expand Down
60 changes: 31 additions & 29 deletions ...om/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,16 +253,16 @@ public void testWhich() throws Exception {
@Test
public void testFile() throws Exception {
setUpContextForRule("test");
context.createFile(context.path("foobar"), "", true, true, thread);
context.createFile(context.path("foo/bar"), "foobar", true, true, thread);
context.createFile(context.path("bar/foo/bar"), "", true, true, thread);
context.createFile(context.getPath("foobar"), "", true, true, thread);
context.createFile(context.getPath("foo/bar"), "foobar", true, true, thread);
context.createFile(context.getPath("bar/foo/bar"), "", true, true, thread);

testOutputFile(outputDirectory.getChild("foobar"), "");
testOutputFile(outputDirectory.getRelative("foo/bar"), "foobar");
testOutputFile(outputDirectory.getRelative("bar/foo/bar"), "");

try {
context.createFile(context.path("/absolute"), "", true, true, thread);
context.createFile(context.getPath("/absolute"), "", true, true, thread);
fail("Expected error on creating path outside of the repository directory");
} catch (RepositoryFunctionException ex) {
assertThat(ex)
Expand All @@ -271,7 +271,7 @@ public void testFile() throws Exception {
.isEqualTo("Cannot write outside of the repository directory for path /absolute");
}
try {
context.createFile(context.path("../somepath"), "", true, true, thread);
context.createFile(context.getPath("../somepath"), "", true, true, thread);
fail("Expected error on creating path outside of the repository directory");
} catch (RepositoryFunctionException ex) {
assertThat(ex)
Expand All @@ -280,7 +280,7 @@ public void testFile() throws Exception {
.isEqualTo("Cannot write outside of the repository directory for path /somepath");
}
try {
context.createFile(context.path("foo/../../somepath"), "", true, true, thread);
context.createFile(context.getPath("foo/../../somepath"), "", true, true, thread);
fail("Expected error on creating path outside of the repository directory");
} catch (RepositoryFunctionException ex) {
assertThat(ex)
Expand All @@ -294,14 +294,16 @@ public void testFile() throws Exception {
public void testDelete() throws Exception {
setUpContextForRule("testDelete");
Path bar = outputDirectory.getRelative("foo/bar");
StarlarkPath barPath = context.path(bar.getPathString());
Object path1 = bar.getPathString();
StarlarkPath barPath = context.getPath(path1);
context.createFile(barPath, "content", true, true, thread);
assertThat(context.delete(barPath, thread)).isTrue();

assertThat(context.delete(barPath, thread)).isFalse();

Path tempFile = scratch.file("/abcde/b", "123");
assertThat(context.delete(context.path(tempFile.getPathString()), thread)).isTrue();
Object path = tempFile.getPathString();
assertThat(context.delete(context.getPath(path), thread)).isTrue();

Path innerDir = scratch.dir("/some/inner");
scratch.dir("/some/inner/deeper");
Expand Down Expand Up @@ -331,30 +333,30 @@ public void testDelete() throws Exception {
@Test
public void testRead() throws Exception {
setUpContextForRule("test");
context.createFile(context.path("foo/bar"), "foobar", true, true, thread);
context.createFile(context.getPath("foo/bar"), "foobar", true, true, thread);

String content = context.readFile(context.path("foo/bar"), "auto", thread);
String content = context.readFile(context.getPath("foo/bar"), "auto", thread);
assertThat(content).isEqualTo("foobar");
}

@Test
public void testPatch() throws Exception {
setUpContextForRule("test");
StarlarkPath foo = context.path("foo");
StarlarkPath foo = context.getPath("foo");
context.createFile(foo, "line one\n", false, true, thread);
StarlarkPath patchFile = context.path("my.patch");
StarlarkPath patchFile = context.getPath("my.patch");
context.createFile(
context.path("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread);
context.getPath("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread);
context.patch(patchFile, StarlarkInt.of(0), "auto", thread);
testOutputFile(foo.getPath(), "line one\nline two\n");
}

@Test
public void testCannotFindFileToPatch() throws Exception {
setUpContextForRule("test");
StarlarkPath patchFile = context.path("my.patch");
StarlarkPath patchFile = context.getPath("my.patch");
context.createFile(
context.path("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread);
context.getPath("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread);
try {
context.patch(patchFile, StarlarkInt.of(0), "auto", thread);
fail("Expected RepositoryFunctionException");
Expand All @@ -371,9 +373,9 @@ public void testCannotFindFileToPatch() throws Exception {
@Test
public void testPatchOutsideOfExternalRepository() throws Exception {
setUpContextForRule("test");
StarlarkPath patchFile = context.path("my.patch");
StarlarkPath patchFile = context.getPath("my.patch");
context.createFile(
context.path("my.patch"),
context.getPath("my.patch"),
"--- ../other_root/foo\n" + "+++ ../other_root/foo\n" + ONE_LINE_PATCH,
false,
true,
Expand All @@ -394,11 +396,11 @@ public void testPatchOutsideOfExternalRepository() throws Exception {
@Test
public void testPatchErrorWasThrown() throws Exception {
setUpContextForRule("test");
StarlarkPath foo = context.path("foo");
StarlarkPath patchFile = context.path("my.patch");
StarlarkPath foo = context.getPath("foo");
StarlarkPath patchFile = context.getPath("my.patch");
context.createFile(foo, "line three\n", false, true, thread);
context.createFile(
context.path("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread);
context.getPath("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread);
try {
context.patch(patchFile, StarlarkInt.of(0), "auto", thread);
fail("Expected RepositoryFunctionException");
Expand Down Expand Up @@ -475,12 +477,12 @@ public void testRemoteExec() throws Exception {
@Test
public void testSymlink() throws Exception {
setUpContextForRule("test");
context.createFile(context.path("foo"), "foobar", true, true, thread);
context.createFile(context.getPath("foo"), "foobar", true, true, thread);

context.symlink(context.path("foo"), context.path("bar"), thread);
context.symlink(context.getPath("foo"), context.getPath("bar"), thread);
testOutputFile(outputDirectory.getChild("bar"), "foobar");

assertThat(context.path("bar").realpath()).isEqualTo(context.path("foo"));
assertThat(context.getPath("bar").realpath()).isEqualTo(context.getPath("foo"));
}

private static void testOutputFile(Path path, String content) throws IOException {
Expand All @@ -497,11 +499,11 @@ public void testDirectoryListing() throws Exception {
scratch.file("/my/folder/a");
scratch.file("/my/folder/b");
scratch.file("/my/folder/c");
assertThat(context.path("/my/folder").readdir("no"))
assertThat(context.getPath("/my/folder").readdir("no"))
.containsExactly(
context.path("/my/folder/a"),
context.path("/my/folder/b"),
context.path("/my/folder/c"));
context.getPath("/my/folder/a"),
context.getPath("/my/folder/b"),
context.getPath("/my/folder/c"));
}

@Test
Expand All @@ -514,7 +516,7 @@ public void testWorkspaceRoot() throws Exception {
public void testNoIncompatibleNoImplicitWatchLabel() throws Exception {
setUpContextForRule("test");
scratch.file(root.getRelative("foo").getPathString());
context.path(fakeFileLabel);
context.getPath(fakeFileLabel);
context.readFile(fakeFileLabel, "no", thread);
assertThat(context.getRecordedFileInputs()).isNotEmpty();
}
Expand All @@ -527,7 +529,7 @@ public void testIncompatibleNoImplicitWatchLabel() throws Exception {
.setBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_WATCH_LABEL, true)
.build());
scratch.file(root.getRelative("foo").getPathString());
context.path(fakeFileLabel);
context.getPath(fakeFileLabel);
context.readFile(fakeFileLabel, "no", thread);
assertThat(context.getRecordedFileInputs()).isEmpty();
}
Expand Down

0 comments on commit a0d26f2

Please sign in to comment.