copy_file: Add parameter to allow symlinks #252
Conversation
rules/private/copy_file_private.bzl
Outdated
| @@ -78,9 +85,10 @@ def _ximpl(ctx): | |||
| return _common_impl(ctx, True) | |||
|
|
|||
| _ATTRS = { | |||
| "src": attr.label(mandatory = True, allow_single_file = True), | |||
| "out": attr.output(mandatory = True), | |||
| "is_symlink": attr.bool(mandatory = True), | |||
There was a problem hiding this comment.
It looks like the is_executable should be folded in here as another bool attribute.
Then you would not need _copy_file vs copy_xfile and the if is_executable at line 127.
Having two different techniques for doing this differentiation is confusing.
There was a problem hiding this comment.
AFAIK, we also have to set is_executable = True on the rule to allow it as tool for other actions.
There was a problem hiding this comment.
Yes, but it would be an attribute on copy_file, and you would just pass the value down from the macro to the rule. The virtually duplicated call at lines 131-150 would fold away.
This change adds a new parameter `allow_symlinks` to `copy_file` that allows the action to create a symlink instead of doing an expensive copy if the execution platform (host) allows it. Updates bazelbuild#248
Yannic
changed the title
|
Updated the PR according to the discussion in #248. ptal, thanks! |
|
Sorry for the delay, I've been busy doing other things! Updated the PR to address your comments. ptal, thanks! |
|
Updating the branch did fix CI, thanks for the review! |
|
Is there anything left to get this merged? |
|
Nope. Other than a reminder. It turns out last week was busier than I thought and it slipped my mind. |
This change adds a new parameter
allow_symlinkstocopy_filethatallows the action to create a symlink instead of doing an expensive
copy if the execution platform (host) allows it.
Updates #248