Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

copy_file: Add parameter to allow symlinks #252

Merged
merged 6 commits into from Jul 10, 2020
Merged

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented May 29, 2020

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 #248

@Yannic Yannic marked this pull request as ready for review May 29, 2020
@@ -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),
Copy link
Collaborator

@aiuto aiuto May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Yannic Yannic Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, we also have to set is_executable = True on the rule to allow it as tool for other actions.

Copy link
Collaborator

@aiuto aiuto Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rules/private/copy_file_private.bzl Outdated Show resolved Hide resolved
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 Yannic changed the title Add symlink_file rule copy_file: Add parameter to allow symlinks Jun 3, 2020
@Yannic
Copy link
Contributor Author

@Yannic Yannic commented Jun 3, 2020

Updated the PR according to the discussion in #248. ptal, thanks!

Copy link
Collaborator

@aiuto aiuto left a comment

Nice. With the refactoring you were able to add the new feature while only growing the code by 7 lines.

rules/private/copy_file_private.bzl Show resolved Hide resolved
rules/private/copy_file_private.bzl Outdated Show resolved Hide resolved
@Yannic
Copy link
Contributor Author

@Yannic Yannic commented Jun 18, 2020

Sorry for the delay, I've been busy doing other things!

Updated the PR to address your comments. ptal, thanks!

aiuto
aiuto approved these changes Jun 19, 2020
Copy link
Collaborator

@aiuto aiuto left a comment

I think it is mostly good. Just the question about executable.

rules/private/copy_file_private.bzl Show resolved Hide resolved
aiuto
aiuto approved these changes Jun 27, 2020
Copy link
Collaborator

@aiuto aiuto left a comment

Things must have gone out of sync again. Maybe you just need a merge to fix CI.

@Yannic Yannic requested a review from juliexxia as a code owner Jun 27, 2020
@Yannic
Copy link
Contributor Author

@Yannic Yannic commented Jun 28, 2020

Updating the branch did fix CI, thanks for the review!

aiuto
aiuto approved these changes Jun 29, 2020
Copy link
Collaborator

@aiuto aiuto left a comment

I'll merge on Monday unless I hear objections

Copy link
Member

@laurentlb laurentlb left a comment

lgtm

@Yannic
Copy link
Contributor Author

@Yannic Yannic commented Jul 10, 2020

Is there anything left to get this merged?

@aiuto
Copy link
Collaborator

@aiuto aiuto commented Jul 10, 2020

Nope. Other than a reminder. It turns out last week was busier than I thought and it slipped my mind.

@aiuto aiuto merged commit 8f3151f into bazelbuild:master Jul 10, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants