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

Cannot use out to set file name in pkg_zip #414

Closed
criemen opened this issue Aug 19, 2021 · 7 comments · Fixed by #552
Closed

Cannot use out to set file name in pkg_zip #414

criemen opened this issue Aug 19, 2021 · 7 comments · Fixed by #552
Assignees
Labels
P3 An issue that we are not working on but will review quarterly

Comments

@criemen
Copy link

criemen commented Aug 19, 2021

The docs of pkg_zip say

Use out or package_file_name to specify the output file name

However, using out results in

ERROR: Traceback (most recent call last):
        File "/workspaces/semmle-code/extractor-python/BUILD.bazel", line 73, column 5, in <toplevel>
                dist(
        File "/workspaces/semmle-code/common.bzl", line 12, column 12, in dist
                pkg_zip(name = name + ".zip", out = name, srcs = [name])
        File "/home/codespace/.cache/bazel/_bazel_codespace/3c41b00dfcd8c4fb714360e042371c98/external/rules_pkg/pkg.bzl", line 723, column 17, in pkg_zip
                pkg_zip_impl(
Error in pkg_zip_impl: rule(...) got multiple values for parameter 'out'

Presumably this is because out is unconditionally set. Instead, it should do something ala

if not kwargs.get("out"):
  kwargs["out"] = archive_name + "." + extension

instead.

@aiuto aiuto added documentation P3 An issue that we are not working on but will review quarterly and removed documentation labels Aug 19, 2021
@aiuto
Copy link
Collaborator

aiuto commented Aug 19, 2021

@criemen
Copy link
Author

criemen commented Aug 23, 2021

I have a rule ala

pkg_zip(name = "bla.zip", package_file_name = "bla.zip", srcs = [...])

Because the name of the rule is bla.zip, the rule creates an out file with name bla.zip.zip. Due to package_file_name, this file gets renamed to bla.zip (great), but I also get a symlink from bla.zip.zip to bla.zip. Without overwriting out, it is not possible to get rid of this symlink.
It's not a big issue, as I can just ignore the symlink, but it is not as pretty as it could be, either.

@aiuto
Copy link
Collaborator

aiuto commented Aug 23, 2021 via email

@ghost
Copy link

ghost commented Oct 18, 2021

out is deprecated. Use package_file_name

I should fix the documentation, and have the rule stop using it.

So, I ran into an small perf problem related to this. I have a 2+ GiB .zip created by pkg_zip. By not allowing me to use out, I'm forced into setup_output_files's ctx.symlink path, which then incurs a 15s critical path tax. One wouldn't expect "Creating symlink foo.zip" to take 15s, but I can only assume that Bazel is (re-)checksumming the symlink target or similar. (See profile snippet below.) Until this is addressed (in Bazel?), I'd still like to have an escape hatch to avoid (in our case unused) symlinks. (We're patching this out internally for the time being.)

Creating symlink bazel-out/k8-dbg-obj/bin/foo.zip
Category | action dependency checking
Wall Duration	15,173.772 ms

(Edit: Filed bazelbuild/bazel#14125 to discuss underlying ctx.actions.symlink perf.)

@aiuto
Copy link
Collaborator

aiuto commented Oct 19, 2021

I'm floored. That seems so wrong.
Are you willing to make a reproduction to file against bazel?

@ghost
Copy link

ghost commented Oct 19, 2021

Are you willing to make a reproduction to file against bazel?

Absolutely. :)

bazelbuild/bazel#14125

@aiuto
Copy link
Collaborator

aiuto commented Oct 19, 2021

Maybe we could make the symlink "as-needed". The wrapper macro would look at package_file_name and if it had no expansions in it, would pass it as the output file name. The rule, seeing output and package_file_name were identical, would only return the one, not making the symlink. This would only impact people who were scripting picking up the output file based on name+'.zip', but that's not a precise approach anyway.

aiuto added a commit to aiuto/rules_pkg that referenced this issue Mar 1, 2022
aiuto added a commit to aiuto/rules_pkg that referenced this issue Mar 1, 2022
@aiuto aiuto self-assigned this Mar 6, 2022
@aiuto aiuto closed this as completed in #552 Mar 7, 2022
aiuto added a commit that referenced this issue Mar 7, 2022
…p. (#552)

- Make 'out' work in a reasonable way. Fixes #414
- Partial fix for #284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 An issue that we are not working on but will review quarterly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants