Allow artifact -> artifact symlink actions from Starlark #10695

Yannic · 2020-02-03T11:10:07Z

Updates #7514

lberki · 2020-02-04T07:35:17Z

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

+  private void registerSymlinkAction(
+      Artifact output, Artifact input, String progressMessage) throws EvalException {
+    if (output.isSymlink()) {
+      throw Starlark.errorf("output of symlink action must be created by declare_file()");


nit: "symlinks pointing to artifacts must be created by declare_file()?"

If you want a dangling symlink, it has to created by declare_symlink(), doesn't it?

lberki · 2020-02-04T07:38:56Z

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

+    // TODO(yannic): This should be configurable.
+    String progressMessage = "Creating symlink " + outputArtifact.getRootRelativePathString();
+
+    if (target instanceof String) {


@laurentlb , is this kind of behavior change based on the runtime type of the argument okay?

It seems to be reasonable on the surface, but there doesn't seem to be that many instances of this in our code and maybe explicitly specifying which one we want like actions.symlink(output, file=<file>) / actions.symlink(output, symlink=<string>) would be better?

TBH, I actually like having actions.symlink(output, file=<file>) / actions.symlink(output, symlink=<string>) more than overloading target based on the runtime type.

Same here, but I want this to be idiomatic, that's why I asked for @laurentlb 's opinion!

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java

brandjon · 2020-02-06T18:13:24Z

Some thoughts:

My understanding is that all symlink functionality in Starlark is currently experimental. Let's not remove any experimental designations in this PR; that requires a separate discussion.
It seems the API we're exposing to Starlark is that a symlink is a special kind of File, created by declare_symlink() instead of declare_file() or other means, and that symlink() requires a symlink File. This should be clearly documented. (I also think the wording around output could be made clearer, but we can iterate on documentation separately. Maybe the param should be renamed to link for symmetry with man 1 ln?)
We seem to be in agreement that direct overloading on path string vs File is confusing, and that a nice alternative to fully splitting symlink() into two methods would be to pass in mutually exclusive keyword args. I like the names target_path=... and target_file=... for these params.

laurentlb · 2020-02-07T13:59:21Z

overloading on path string vs File is confusing

I agree. Maybe we need to resolve the situation about the experimental dangling symlink feature. Do we want to support it? Is it going away?

lberki · 2020-02-07T14:46:27Z

We definitely want to support it in the long term. For now, however, the fully supported one is the artifact -> artifact symlink.

Yannic · 2020-02-08T14:20:09Z

Some thoughts:

My understanding is that all symlink functionality in Starlark is currently experimental. Let's not remove any experimental designations in this PR; that requires a separate discussion.

"Normal" symlink functionality (i.e. artifact -> artifact) is currently non-existent in Starlark, only unresolved symlinks are available behind --experimental_allow_unresolved_symlinks. I don't think guarding artifact -> artifact symlinks with a flag is necessary (and definitely not with --experimental_allow_unresolved_symlinks, that would be really confusing to users).

It seems the API we're exposing to Starlark is that a symlink is a special kind of File, created by declare_symlink() instead of declare_file() or other means, and that symlink() requires a symlink File. This should be clearly documented. (I also think the wording around output could be made clearer, but we can iterate on documentation separately. Maybe the param should be renamed to link for symmetry with man 1 ln?)

See 1. for declare_file() vs declare_symlink(). I agree that documentation should be improved as part of this PR :).

We seem to be in agreement that direct overloading on path string vs File is confusing, and that a nice alternative to fully splitting symlink() into two methods would be to pass in mutually exclusive keyword args. I like the names target_path=... and target_file=... for these params.

Seems like we all agree that overloading is not the way to go.

lberki · 2020-02-10T08:14:41Z

Let's go with target_path= and target_file= then. ctx.actions.symlink() is nice enough that I don't want to uglify it and rename it to something like symlink_file().

I just realized that this is inconsistent with the symlink function in repository contexts which have from= and to= attributes, but given that those are very confusing (in fact, I was sure that from denotes the symlink and to denotes the target until I read the documentation), so I'd much rather keep the output/target names, unless @laurentlb or @brandjon strongly disagree.

brandjon · 2020-02-12T17:49:36Z

"Normal" symlink functionality (i.e. artifact -> artifact) is currently non-existent in Starlark, only unresolved symlinks are available behind --experimental_allow_unresolved_symlinks. I don't think guarding artifact -> artifact symlinks with a flag is necessary

If it's experimental it needs to be behind a flag.

I'd much rather keep the output/target names

Any thoughts on link instead of output? I don't have a strong opinion, and output might be better after all if we're playing up the symmetry between normal file artifacts and symlink artifacts in Starlark API.

Updates bazelbuild#7514

Yannic · 2020-02-18T14:26:49Z

src/test/shell/bazel/bazel_unresolved_symlink_test.sh

+EOF
+
+  # bazel build //a:a && fail "build succeeded"
+  # [[ "$?" == 1 ]] || fail "unexpected exit code"


I can't quite figure out why this isn't failing.
Looking at this comment, it seems like the action should fail.
Am I missing something here, or is that comment out-of date (or invalid for Bazel)?

Hehe. That's because Bazel makes every output file executable (which is not how I would like it to be, but it would be quite difficult to change now :( ). Probably this is where:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java#L737

If you want to test this functionality, symlink to a source file instead of one that was built by Bazel itself.

Makes sense, but not exactly what I wanted to hear :/.

I'll use your workaround and symlink to a source file for the test.

Yannic · 2020-02-18T14:31:01Z

This is ready for review now.

@brandjon I decided to stick with output to be consistent with ctx.actions.write, ctx.actions.run and ctx.actions.run_shell. I've also added an "experimental" warning to target_path (which is the only experimental part of this API).

lberki

Sorry for the delay. Feel free to nag me more in the future :)

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

lberki · 2020-03-09T10:06:21Z

src/test/shell/bazel/bazel_unresolved_symlink_test.sh

+EOF
+
+  # bazel build //a:a && fail "build succeeded"
+  # [[ "$?" == 1 ]] || fail "unexpected exit code"


Hehe. That's because Bazel makes every output file executable (which is not how I would like it to be, but it would be quite difficult to change now :( ). Probably this is where:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java#L737

If you want to test this functionality, symlink to a source file instead of one that was built by Bazel itself.

Yannic

Thanks! ptal

Yannic · 2020-03-09T13:50:06Z

src/test/shell/bazel/bazel_unresolved_symlink_test.sh

+EOF
+
+  # bazel build //a:a && fail "build succeeded"
+  # [[ "$?" == 1 ]] || fail "unexpected exit code"


Makes sense, but not exactly what I wanted to hear :/.

I'll use your workaround and symlink to a source file for the test.

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

lberki

Looks reasonable. Let me see if this can still be imported.

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

lberki · 2020-04-01T15:12:50Z

Update: internal change is being reviewed by @brandjon

Yannic · 2020-04-13T23:37:49Z

Any updates on this?

brandjon · 2020-04-15T22:24:30Z

Hi Yannic, I had a few comments from the internal review, but first I want to step back and make sure I understand the big picture. I'm particularly cautious because we seem to have skipped a Starlark API review for this change and for lberki@'s earlier dangling symlink feature (which up until now has been marked experimental).

Questions for @lberki:

There's an old doc, Symlinks in Bazel Remote Caching and Execution, that says dangling symlinks are explicitly not supported in remote execution. Add support for unresolved symbolic links #9075 and 2fc2e2b say different things about RBE compatibility but I don't know the nuances. What's the current status of this feature and remote execution, and does it block releasing any symlink functionality as non-experimental?
To confirm your earlier post, you agree that the new functionality added in this PR is ok to be marked non-experimental, right?

For @Yannic and @lberki: I want to make sure we're on the same page. Does this describe the API?

declare_file() creates a regular artifact (isSymlink() == false). But the corresponding physical file may or may not be a symlink, depending on whether it was generated by symlink() or another action. So users would use declare_file() when they want something guaranteed to exist.
declare_symlink() creates a symlink artifact (isSymlink() == true). The corresponding physical file must be a symlink. It may or may not resolve to a path that exists, and that path may or may not be in the output tree. This functionality remains experimental for now.

I'll reply with my comments from the internal review shortly.

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

src/test/shell/bazel/bazel_symlink_test.sh

brandjon

If we're distinguishing between artifacts created by declare_file and declare_symlink, do we have a way to expose the difference to users? Like a myfile.is_symlink bool (similar to is_source)?

Yannic

I think you got the API right (modulo small caveat: symlink() with ``isSymlink() == false` may actually copy on Windows).

If we're distinguishing between artifacts created by declare_file and declare_symlink, do we have a way to expose the difference to users? Like a myfile.is_symlink bool (similar to is_source)?

.is_symlink isn't available on FileAPI yet, but it seems not too complicated to add it (Artifact already known whether it's a symlink or not). Should we add it as part of this change?

src/test/shell/bazel/bazel_symlink_test.sh

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

src/test/shell/bazel/bazel_symlink_test.sh

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

src/test/shell/bazel/bazel_symlink_test.sh

brandjon

.is_symlink isn't available on FileAPI yet, but it seems not too complicated to add it (Artifact already known whether it's a symlink or not). Should we add it as part of this change?

No, this PR is already large enough and iterated enough times. :) Happy to accept this in a follow-up, or leave an open FR.

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java

src/test/shell/bazel/bazel_symlink_test.sh

brandjon · 2020-04-22T20:32:32Z

@lberki, when you get a chance please weigh in on these three concerns:

Yannic

.is_symlink isn't available on FileAPI yet, but it seems not too complicated to add it (Artifact already known whether it's a symlink or not). Should we add it as part of this change?

No, this PR is already large enough and iterated enough times. :) Happy to accept this in a follow-up, or leave an open FR.

Filed #11209

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java

src/test/shell/bazel/bazel_symlink_test.sh

brandjon

LGTM, I'll get my answers from @lberki on the internal merge. Thanks!

lberki · 2020-04-27T15:37:48Z

re: #10695 (comment), we are on the same page.

The reason why unresolved symlinks are marked as experimental is exactly the interaction with remote execution (maybe it works, but I certainly haven't tried). In addition, it hasn't gone through a proper design review. ctx.actions.symlink() itself for non-unresolved (double negation! say, regualr symlinks) is a pretty safe change, it only exposes old internal functionality that hasn't changed since forever.
Yep. You may want to ask @laurentlb what he thinks about it, but I don't think he'd object.

brandjon · 2020-04-27T15:39:18Z

Thanks, that should be everything then!

brandjon · 2020-04-30T14:22:18Z

(In internal merge, I added a documentation note to target_path that it's experimental and requires setting the flag.)

Follow-up to bazelbuild#10695

Follow-up to #10695 Closes #11268. PiperOrigin-RevId: 309291255

googlebot added the cla: yes label Feb 3, 2020

irengrig assigned lberki Feb 3, 2020

irengrig added the team-Rules-Server Issues for serverside rules included with Bazel label Feb 3, 2020

lberki reviewed Feb 4, 2020

View reviewed changes

laurentlb reviewed Feb 4, 2020

View reviewed changes

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java Outdated Show resolved Hide resolved

laurentlb mentioned this pull request Feb 4, 2020

Add Starlark support for Symlink actions. #7514

Closed

Yannic force-pushed the symlink branch 3 times, most recently from 67efa42 to 7e3d5c9 Compare February 18, 2020 14:04

Allow artifact -> artifact symlink actions from Starlark

2292177

Updates bazelbuild#7514

Yannic force-pushed the symlink branch from 7e3d5c9 to 2292177 Compare February 18, 2020 14:12

Yannic commented Feb 18, 2020

View reviewed changes

Yannic marked this pull request as ready for review February 18, 2020 14:28

lberki reviewed Mar 9, 2020

View reviewed changes

Yannic added 2 commits March 11, 2020 01:31

Address comments

4f10e46

Disable executability test on Windows

390c73c

Yannic commented Mar 11, 2020

View reviewed changes

Yannic mentioned this pull request Mar 23, 2020

Remove import-path guessing from ProtoCompileActionBuilder #10939

Closed

lberki reviewed Apr 1, 2020

View reviewed changes

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java Outdated Show resolved Hide resolved

lberki approved these changes Apr 1, 2020

View reviewed changes

lberki mentioned this pull request Apr 1, 2020

Refactoring: Rename ProtoModuleApi to ProtoCommonApi, rename ProtoInfoApi.Provider to ProtoInfoProviderApi, and extract most needs of ruleContext out of ProtoCommon and into BazelProtoLibrary #10966

Closed

lberki requested a review from brandjon April 2, 2020 06:50

Yannic mentioned this pull request Apr 14, 2020

Use released protoc binaries bazelbuild/rules_proto#36

Merged

brandjon requested changes Apr 16, 2020

View reviewed changes

Address review comments

8735236

Yannic commented Apr 19, 2020

View reviewed changes

brandjon requested changes Apr 22, 2020

View reviewed changes

Yannic added 2 commits April 23, 2020 19:04

Address more review comments

1662eda

Merge remote-tracking branch 'origin/master' into symlink

b52fe5c

Yannic commented Apr 23, 2020

View reviewed changes

brandjon mentioned this pull request Apr 23, 2020

Expose Artifact#isSymlink() to Starlark #11209

Closed

brandjon approved these changes Apr 23, 2020

View reviewed changes

bazel-io closed this in 43f793c Apr 30, 2020

Yannic added a commit to Yannic/bazel that referenced this pull request Apr 30, 2020

Fix documentation of ctx.actions.symlink

40b6a32

Follow-up to bazelbuild#10695

Yannic mentioned this pull request Apr 30, 2020

Fix documentation of ctx.actions.symlink #11268

Closed

bazel-io pushed a commit that referenced this pull request Apr 30, 2020

Fix documentation of ctx.actions.symlink

1726c05

Follow-up to #10695 Closes #11268. PiperOrigin-RevId: 309291255

Yannic mentioned this pull request May 29, 2020

feature request: Create common rule for symlinking bazelbuild/bazel-skylib#248

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow artifact -> artifact symlink actions from Starlark #10695

Allow artifact -> artifact symlink actions from Starlark #10695

Yannic commented Feb 3, 2020

lberki Feb 4, 2020

lberki Feb 4, 2020

Yannic Feb 4, 2020

lberki Feb 4, 2020

brandjon commented Feb 6, 2020

laurentlb commented Feb 7, 2020

lberki commented Feb 7, 2020

Yannic commented Feb 8, 2020

lberki commented Feb 10, 2020

brandjon commented Feb 12, 2020

Yannic Feb 18, 2020

lberki Mar 9, 2020

Yannic Mar 9, 2020

Yannic commented Feb 18, 2020

lberki left a comment

lberki Mar 9, 2020

Yannic left a comment

Yannic Mar 9, 2020

lberki left a comment

lberki commented Apr 1, 2020

Yannic commented Apr 13, 2020

brandjon commented Apr 15, 2020 •

edited

Loading

brandjon left a comment

Yannic left a comment

brandjon left a comment

brandjon commented Apr 22, 2020

Yannic left a comment

brandjon left a comment •

edited

Loading

lberki commented Apr 27, 2020

brandjon commented Apr 27, 2020

brandjon commented Apr 30, 2020

Allow artifact -> artifact symlink actions from Starlark #10695

Allow artifact -> artifact symlink actions from Starlark #10695

Conversation

Yannic commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandjon commented Feb 6, 2020

laurentlb commented Feb 7, 2020

lberki commented Feb 7, 2020

Yannic commented Feb 8, 2020

lberki commented Feb 10, 2020

brandjon commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic commented Feb 18, 2020

lberki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lberki left a comment

Choose a reason for hiding this comment

lberki commented Apr 1, 2020

Yannic commented Apr 13, 2020

brandjon commented Apr 15, 2020 • edited Loading

brandjon left a comment

Choose a reason for hiding this comment

Yannic left a comment

Choose a reason for hiding this comment

brandjon left a comment

Choose a reason for hiding this comment

brandjon commented Apr 22, 2020

Yannic left a comment

Choose a reason for hiding this comment

brandjon left a comment • edited Loading

Choose a reason for hiding this comment

lberki commented Apr 27, 2020

brandjon commented Apr 27, 2020

brandjon commented Apr 30, 2020

brandjon commented Apr 15, 2020 •

edited

Loading

brandjon left a comment •

edited

Loading