Skip to content

copy_file should *never* set no-remote #562

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.

Sign up for GitHub

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

Jump to bottom
Open
ulfjack opened this issue Mar 17, 2025 · 5 comments · May be fixed by #565
Open

copy_file should *never* set no-remote #562

ulfjack opened this issue Mar 17, 2025 · 5 comments · May be fixed by #565

Comments

@ulfjack
Copy link

ulfjack commented Mar 17, 2025

When build-without-the-bytes is used, forcing no-remote + no-cache is often worse than allowing it to execute remotely. If the file is a source file, then it's better to upload it right away and execute the copy remotely (cached). If the file is an output file, then Bazel has to download the file in order to perform a local copy. Best case scenario, no work is ever done locally, and this breaks that.

It's a better policy to not try to set these at the rule level.

If this needs to be user-configurable on a per-rule basis, it might be viable to create a flag and then set these tags only if the flag is set.
@ulfjack
Copy link
Author

ulfjack commented Mar 17, 2025

https://github.com/bazelbuild/bazel-skylib/blob/main/rules/private/copy_common.bzl#L18

@fmeum
Copy link
Contributor

fmeum commented Mar 18, 2025

There also won't be a new CAS entry, so this would virtually be all upside. RBE does support source directories now.

That said, ideally allow_symlink would just be the default and then there wouldn't even be a non-trivial action to run in the first place.

@ulfjack
Copy link
Author

ulfjack commented Mar 18, 2025

If I read the code correctly, allow_symlink is the default, except on Windows.

@fmeum
Copy link
Contributor

fmeum commented Mar 19, 2025

@ulfjack I think that's not true, allow_symlink defaults to False in the copy_file macro and is_windows is only used to decide whether to use a shell script or a bat script.

@fmeum fmeum linked a pull request Mar 19, 2025 that will close this issue
Open
@ulfjack
Copy link
Author

ulfjack commented Mar 20, 2025

You're right. Apparently I misread the code. I agree that it should default to true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Optimize copy_file fmeum/bazel-skylib
2 participants
@fmeum @ulfjack