Change in bazel[master]: Make the runfiles symlinks avaliable from skylark.

[Jason Michalski (Gerrit)]

Jason Michalski has uploaded a new change for review.

https://bazel-review.googlesource.com/4080

Change subject: Make the runfiles symlinks avaliable from skylark.
......................................................................

Make the runfiles symlinks avaliable from skylark.

This change allows skylark rules to access the symlinks and
root_symlinks fromt the default and data runfiles. Packaging rules such
as tar_pkg will be able to include symlinked runfiles afer this change.

Fixes: #1109
Change-Id: I221e4cabe8d3f7b555f4c3f0ae108a12474a2db5
---
M src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
M
src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
2 files changed, 76 insertions(+), 2 deletions(-)



diff --git
a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
index 0de1b2d..c0088fa 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
@@ -129,6 +129,12 @@
// equals to the third one if they are not the same instance (which they
almost never are)
//
// Goodnight, prince(ss)?, and sweet dreams.
+ @Immutable
+ @SkylarkModule(
+ name = "symlinkentry",
+ category = SkylarkModuleCategory.NONE,
+ doc = "An interface for a runfile symlink."
+ )
private static final class SymlinkEntry {
private final PathFragment path;
private final Artifact artifact;
@@ -138,10 +144,20 @@
this.artifact = artifact;
}

+ @SkylarkCallable(
+ name = "path",
+ doc = "Returns the symlink path",
+ structField = true
+ )
public PathFragment getPath() {
return path;
}

+ @SkylarkCallable(
+ name = "file",
+ doc = "Returns the file referenced by this symlink",
+ structField = true
+ )
public Artifact getArtifact() {
return artifact;
}
@@ -562,6 +578,7 @@
/**
* Returns the root symlinks.
*/
+ @SkylarkCallable(name = "root_symlinks", doc = "Returns the set of root
symlinks", structField = true)
public NestedSet<SymlinkEntry> getRootSymlinks() {
return rootSymlinks;
}
diff --git
a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 5e5fb7d..49982a1 100644
---
a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++
b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -951,6 +951,29 @@
scratch.file("test/__init__.py");
scratch.file(
"test/rule.bzl",
+ "def symlink_impl(ctx):",
+ " symlinks = {",
+ " 'symlink_' + f.short_path: f",
+ " for f in getattr(ctx.attr.symlink, 'files', [])",
+ " }",
+ " root_symlinks = {",
+ " 'root_symlink_' + f.short_path: f",
+ " for f in getattr(ctx.attr.root_symlink, 'files', [])",
+ " }",
+ " runfiles = ctx.runfiles(",
+ " symlinks=symlinks,",
+ " root_symlinks=root_symlinks,",
+ " )",
+ " return struct(",
+ " runfiles=runfiles",
+ " )",
+ "symlink_rule = rule(",
+ " implementation = symlink_impl,",
+ " attrs = {",
+ " 'symlink': attr.label(allow_files=True),",
+ " 'root_symlink': attr.label(allow_files=True)",
+ " },",
+ ")",
"def _impl(ctx):",
" return",
"skylark_rule = rule(",
@@ -961,11 +984,15 @@
")");
scratch.file(
"test/BUILD",
- "load('/test/rule', 'skylark_rule')",
+ "load('/test/rule', 'skylark_rule', 'symlink_rule')",
"py_library(name = 'lib', srcs = ['a.py', 'b.py'])",
"skylark_rule(name = 'foo', dep = ':lib')",
"py_library(name = 'lib_with_init', srcs =
['a.py', 'b.py', '__init__.py'])",
- "skylark_rule(name = 'foo_with_init', dep = ':lib_with_init')");
+ "skylark_rule(name = 'foo_with_init', dep = ':lib_with_init')",
+ "symlink_rule(name = 'lib_with_symlink', symlink = ':a.py')",
+ "skylark_rule(name = 'foo_with_symlink', dep
= ':lib_with_symlink')",
+ "symlink_rule(name = 'lib_with_root_symlink', root_symlink
= ':a.py')",
+ "skylark_rule(name = 'foo_with_root_symlink', dep
= ':lib_with_root_symlink')");

SkylarkRuleContext ruleContext = createRuleContext("//test:foo");
Object filenames =
@@ -988,6 +1015,36 @@
assertThat(noEmptyFilenames).isInstanceOf(SkylarkList.class);
SkylarkList noEmptyFilenamesList = (SkylarkList) noEmptyFilenames;
assertThat(noEmptyFilenamesList).containsExactly().inOrder();
+
+ SkylarkRuleContext ruleWithSymlinkContext =
createRuleContext("//test:foo_with_symlink");
+ Object symlinkPaths =
+ evalRuleContextCode(
+ ruleWithSymlinkContext, "[str(s.path) for s in
ruleContext.attr.dep.data_runfiles.symlinks]");
+ assertThat(symlinkPaths).isInstanceOf(SkylarkList.class);
+ SkylarkList symlinkPathsList = (SkylarkList) symlinkPaths;
+
assertThat(symlinkPathsList).containsExactly("symlink_test/a.py").inOrder();
+
+ Object symlinkFilenames =
+ evalRuleContextCode(
+ ruleWithSymlinkContext, "[s.file.short_path for s in
ruleContext.attr.dep.data_runfiles.symlinks]");
+ assertThat(symlinkFilenames).isInstanceOf(SkylarkList.class);
+ SkylarkList symlinkFilenamesList = (SkylarkList) symlinkFilenames;
+
assertThat(symlinkFilenamesList).containsExactly("test/a.py").inOrder();
+
+ SkylarkRuleContext ruleWithRootSymlinkContext =
createRuleContext("//test:foo_with_root_symlink");
+ Object rootSymlinkPaths =
+ evalRuleContextCode(
+ ruleWithRootSymlinkContext, "[str(s.path) for s in
ruleContext.attr.dep.data_runfiles.root_symlinks]");
+ assertThat(rootSymlinkPaths).isInstanceOf(SkylarkList.class);
+ SkylarkList rootSymlinkPathsList = (SkylarkList) rootSymlinkPaths;
+
assertThat(rootSymlinkPathsList).containsExactly("root_symlink_test/a.py").inOrder();
+
+ Object rootSymlinkFilenames =
+ evalRuleContextCode(
+ ruleWithRootSymlinkContext, "[s.file.short_path for s in
ruleContext.attr.dep.data_runfiles.root_symlinks]");
+ assertThat(rootSymlinkFilenames).isInstanceOf(SkylarkList.class);
+ SkylarkList rootSymlinkFilenamesList = (SkylarkList)
rootSymlinkFilenames;
+
assertThat(rootSymlinkFilenamesList).containsExactly("test/a.py").inOrder();
}

@Test

--
To view, visit https://bazel-review.googlesource.com/4080
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I221e4cabe8d3f7b555f4c3f0ae108a12474a2db5
Gerrit-PatchSet: 1
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Jason Michalski <arm...@armooo.net>

[Kristina Chodorow (Gerrit)]

Kristina Chodorow has posted comments on this change.

Change subject: Make the runfiles symlinks avaliable from skylark.
......................................................................

Patch Set 1:

(5 comments)

https://bazel-review.googlesource.com/#/c/4080/1/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
File src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java:

PS1, Line 147: SkylarkCallable
Your indentation is off here and below.


PS1, Line 152: PathFragment
PathFragment is not a Skylark type. Add a method to return the String
version of the PF.


PS1, Line 581: root symlinks
"Root" is kind of ambiguous. Specify that the symlinks' paths are relative
to the .runfiles directory.


https://bazel-review.googlesource.com/#/c/4080/1/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
File
src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java:

PS1, Line 954: "def symlink_impl(ctx):",
Move the new tests to their own methods.


PS1, Line 993: "skylark_rule(name = 'foo_with_symlink', dep
= ':lib_with_symlink')",
No need to wrap these in skylark_rule.

--
To view, visit https://bazel-review.googlesource.com/4080
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I221e4cabe8d3f7b555f4c3f0ae108a12474a2db5
Gerrit-PatchSet: 1
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Jason Michalski <arm...@armooo.net>

Gerrit-Reviewer: Klaus Aehlig <aeh...@google.com>
Gerrit-Reviewer: Kristina Chodorow <kcho...@google.com>
Gerrit-HasComments: Yes

[Jason Michalski (Gerrit)]

Jason Michalski has uploaded a new patch set (#2).

Change subject: Make the runfiles symlinks avaliable from skylark.
......................................................................

Make the runfiles symlinks avaliable from skylark.

This change allows skylark rules to access the symlinks and
root_symlinks fromt the default and data runfiles. Packaging rules such
as tar_pkg will be able to include symlinked runfiles afer this change.

Fixes: #1109
Change-Id: I221e4cabe8d3f7b555f4c3f0ae108a12474a2db5
---
M src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
M
src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java

2 files changed, 130 insertions(+), 0 deletions(-)

--
To view, visit https://bazel-review.googlesource.com/4080
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I221e4cabe8d3f7b555f4c3f0ae108a12474a2db5
Gerrit-PatchSet: 2

Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Jason Michalski <arm...@armooo.net>

[Jason Michalski (Gerrit)]

Jason Michalski has posted comments on this change.

Change subject: Make the runfiles symlinks avaliable from skylark.
......................................................................

Patch Set 1:

(5 comments)

Thanks for the quick review.

https://bazel-review.googlesource.com/#/c/4080/1/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
File src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java:

PS1, Line 147: SkylarkCallable
> Your indentation is off here and below.

Done

PS1, Line 152: PathFragment
> PathFragment is not a Skylark type. Add a method to return the String
> vers

Done

PS1, Line 581: root symlinks
> "Root" is kind of ambiguous. Specify that the symlinks' paths are
> relative

Done

https://bazel-review.googlesource.com/#/c/4080/1/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
File
src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java:

PS1, Line 954: "def symlink_impl(ctx):",
> Move the new tests to their own methods.

Done


PS1, Line 993: "skylark_rule(name = 'foo_with_symlink', dep
= ':lib_with_symlink')",

> No need to wrap these in skylark_rule.

It was not obvious how to test that the runfiles were correctly set without
the extra rule to use with evalRuleContextCode. Is there an example of how
to do this? The next patch set 2 also uses an extra rule for to test this.

--
To view, visit https://bazel-review.googlesource.com/4080
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I221e4cabe8d3f7b555f4c3f0ae108a12474a2db5
Gerrit-PatchSet: 1
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Jason Michalski <arm...@armooo.net>

Gerrit-Reviewer: Jason Michalski <arm...@armooo.net>

Gerrit-Reviewer: Klaus Aehlig <aeh...@google.com>
Gerrit-Reviewer: Kristina Chodorow <kcho...@google.com>

Gerrit-HasComments: Yes

[Kristina Chodorow (Gerrit)]

Kristina Chodorow has posted comments on this change.

Change subject: Make the runfiles symlinks avaliable from skylark.
......................................................................

Patch Set 1:

(1 comment)

> Patch Set 1:

> (5 comments)

> Thanks for the quick review.

Yeah, so... unfortunately this adds new syntax to Skylark. To prevent API
creep, we have an internal process for design review of new Skylark
syntax. Congratulations, you're the first external contributor to hit this!

Dimitry (added as a reviewer) is working on putting the instructions for
design review up on bazel.io, but basically you send an email to the
bazel-dev mailing list with:

1. Motivation for the change (a GitHub issue link is acceptable)
2. An example of the usage of the proposed API/language feature
3. Description of the proposed change

But you might want to wait until Dimitry gets back to you, since there
might be other requirements, too (I'm not sure).

Thanks for fixing this and sorry for the inconvenience!

https://bazel-review.googlesource.com/#/c/4080/1/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
File
src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java:

Line 993: "skylark_rule(name = 'foo_with_symlink', dep
= ':lib_with_symlink')",

> It was not obvious how to test that the runfiles were correctly set
> without

I think you can do something like
https://github.com/bazelbuild/bazel/blob/master/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java#L650.

--
To view, visit https://bazel-review.googlesource.com/4080
To unsubscribe, visit https://bazel-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I221e4cabe8d3f7b555f4c3f0ae108a12474a2db5
Gerrit-PatchSet: 1
Gerrit-Project: bazel
Gerrit-Branch: master
Gerrit-Owner: Jason Michalski <arm...@armooo.net>