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

feature request: Create common rule for symlinking #248

Open
Yannic opened this issue May 18, 2020 · 6 comments
Open

feature request: Create common rule for symlinking #248

Yannic opened this issue May 18, 2020 · 6 comments

Comments

@Yannic
Copy link
Contributor

Yannic commented May 18, 2020

It would be great if skylib had a rule similar to copy_file that, instead of manually copying the file, uses ctx.actions.symlink (since Bazel 3.2).

This could be either implemented as a new rule (symlink_file?), or as attribute of copy_file (which could potentially become the default behavior in the future).

Yannic added a commit to Yannic/bazel-skylib that referenced this issue May 29, 2020
@aiuto
Copy link
Contributor

aiuto commented May 29, 2020

I have some reservations about this. I don't have time to do a deep explanation this week, so I'm just asking questions about the need and the PR you sent. Because knowing the needs is important to evaluate the PR.

why does someone need a symlink?

  • Do we just want the file under a new name and this is speed optimization over copy?
  • Why do we want a new name in the first place?
  • Is this for packaging later in tar or zip? (e.g. BUILD_dist in source, but need BUILD in the tgz distribution).
  • Do we want to support naked symlinks, which are a different real need. Again, the motivations are the same

If we only are doing this to rename in a performant way, why can't we make this a fast_copy rule which promises to do a symlink if it can or copy if it can't, but it leaves a real artifiact either way.

If we are doing this to define packaging structures, then I don't think we want to build the symlinks. We want package rules that describe the need for the symlink, and the packager gets to pick what it builds. For example, it is reasonable for a developer with remote build to have a windows host, but build a tarball that contains a symlink.

@Yannic
Copy link
Contributor Author

Yannic commented May 29, 2020

The purpose of this new rule is to have a faster way to give a file a new name. The reason why we need to make files available under different names is that some tools unfortunately behave differently depending on their name (e.g. sha256(clang) == sha256(clang++), but they do different things when called as clang vs clang++ 😡). Obviously, symlinking a large binary is faster than copying it.

I'm not sure what you mean with naked symlinks. Are you talking about the experimental unresolved symlinks in Bazel which allow creating symlinks which point to arbitrary strings (bazelbuild/bazel#10298)? If so, then, at this moment, I don't have any intention in supporting them in the symlink_file rule. IMO, unresolved symlinks are tricky (caching, remote execution, ...) and should only be used to reach absolute paths outside of a WORKSPACE (e.g. /usr/bin/gcc). Making them easier to use will most-likely cause more harm than good.

For the actual implementation, I think you may not yet be aware of the brand-new ctx.actions.symlink that landed in Bazel 3.2 which also allows Artifact -> Artifact (bazelbuild/bazel#10695) symlinks. It behaves exactly like the fast_copy rule you're describing.

@aiuto
Copy link
Contributor

aiuto commented Jun 1, 2020

Thanks. The use case makes things much clearer.

If you want it for speed, let's make a rule name that declares that intent, something like 'lightweight_copy. Of course, that name is ugly, but the point is that we hint that it is less resource consumptive and that does not mean "always a symlnk".

Or maybe this could just be a parameter to copy_file, like "allow_fast_copy".

By naked, I did mean dangling. I'm aware of the symlink action.

My intent is that the rule name here does not make people believe they are getting deep properties of a symlink. I've been working on packaging rules a lot lately, and there, the symlink-ness property does matter. That is, if the rule says symlink and you are building a zip file from windows and expect the symlink to be created in the zip file, you would be disappointed. But if the rule as called lightweight_copy I would not have that expectation. Does that make sense to you?

@Yannic
Copy link
Contributor Author

Yannic commented Jun 2, 2020

Thanks! Yeah, that makes sense.

I'm leaning more towards a parameter on copy_file (allow_symlink might make it more obvious that a symlink can be created than allow_fast_copy).

@aiuto
Copy link
Contributor

aiuto commented Jun 2, 2020

Sounds good to me.
@laurentlb any thoughts?

Yannic added a commit to Yannic/bazel-skylib that referenced this issue Jun 3, 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 bazelbuild#248
aiuto pushed a commit that referenced this issue Jul 10, 2020
* copy_file: Add parameter to allow symlinks

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

* Update docs

* Refactor `is_executable` into attribute

* Fix typo

* s/_impl/_copy_file_impl/
@adam-azarchs
Copy link

I believe this issue is closed by #252?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants