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

pkg_zip drops execute attribute when zipping executable file #96

Closed
statik opened this issue Sep 30, 2019 · 12 comments · Fixed by #576
Closed

pkg_zip drops execute attribute when zipping executable file #96

statik opened this issue Sep 30, 2019 · 12 comments · Fixed by #576
Assignees
Labels
bug P2 An issue that should be worked on when time is available
Milestone

Comments

@statik
Copy link
Contributor

statik commented Sep 30, 2019

I am using pkg_zip to zip up a go_binary generated using bazel_rules_go. The go_binary has the execute bit set, but when I unpack the contents of the generated zip file, the program does not have the execute bit set.

The goal I'm trying to accomplish is to deliver an artifact that can run in AWS Lambda, and the lambda environment is expecting the execute bit to be set.

I'm happy to try and contribute a fix for this, figured opening an issue was a good first step.

statik added a commit to kindlyops/rules_pkg that referenced this issue Sep 30, 2019
Signed-off-by: Elliot Murphy <statik@users.noreply.github.com>
@j3parker
Copy link
Contributor

Ouch, that's unfortunate.

It looks like ZipInfo.external_attrs might be the ticket. See 4.4.2 and 4.4.15 of this document, maybe, for some more details.

@j3parker
Copy link
Contributor

Ah you beat me to it :) Thanks.

@aiuto
Copy link
Collaborator

aiuto commented Oct 1, 2019

Solutions that look at the file system mode (like kindlyops@4a8c3e7) do not work in general for Bazel, especially when remote build and caching are involved.

Modes we want in the .tar (or .zip, .rpm, .msi, ...) have to be specified in BUILD files in some way.
Have you tried a mode attribute in your BUILD rules?

@statik
Copy link
Contributor Author

statik commented Oct 6, 2019

Thanks! I will give adding a mode attribute a try.

Note that pkg_tar attempts to look at the mode of the source file as well, I wonder if that is a bug?

# If mode is unspecified, derive the mode from the file's mode.
if mode is None:
mode = 0o755 if os.access(f, os.X_OK) else 0o644

@statik
Copy link
Contributor Author

statik commented Oct 7, 2019

I've redone #97 to use a mode attribute in the BUILD rule as suggested, it seems to work well. Thanks!

@scorsi
Copy link

scorsi commented Jun 25, 2020

Hope it's resolved (and available to use ofc) I'm facing some trouble while migrating to Bazel build. :)

@statik
Copy link
Contributor Author

statik commented Jun 26, 2020

Hi @scorsi sorry you are hitting this. I don't know why #97 hasn't been merged yet. I've had to stop using rules_pkg because of this and #104.

As an alternative, this genrule works for zipping up a binary to deploy to AWS lambda

genrule(
    name = "lambda_deploy",
    srcs = [":lambda_binary"],
    outs = ["lambda_deploy.zip"],
    cmd = "$(location @bazel_tools//tools/zip:zipper) c $@ main=$(SRCS)",
    tools = ["@bazel_tools//tools/zip:zipper"],
    visibility = ["//visibility:public"],
)

@scorsi
Copy link

scorsi commented Jun 26, 2020

@statik It looks like this zip rule is completely broken haha :)
Thanks for the tip, I will investigate it.

@scorsi
Copy link

scorsi commented Jun 26, 2020

Thanks @statik, it works fine with the genrule + zipper ! 🚀

aiuto added a commit that referenced this issue Jul 9, 2020
* Preserve unix attributes on zip inputs (#96)

Signed-off-by: Elliot Murphy <statik@users.noreply.github.com>

* Handle modes in pkg_zip similar to pkg_tar

Signed-off-by: Elliot Murphy <statik@users.noreply.github.com>

* Update BUILD

Fix a merge

Co-authored-by: aiuto <aiuto@google.com>
@aiuto aiuto added bug P2 An issue that should be worked on when time is available labels Feb 24, 2021
@aiuto aiuto added this to the 1.0 milestone Nov 1, 2021
@aiuto aiuto self-assigned this Feb 14, 2022
@aiuto
Copy link
Collaborator

aiuto commented Feb 14, 2022

I think I can now address this in a reasonable way, and it will tie together with support for adding runfiles.
The implementation I am thinking of is to move normal file processing into pkg_files.process_src
It can do the DefaultInfo processing to support all pkg_types at once.
I think the only problem is that pkg_tar won't be able to make use of that if you want to use the files attribute to remap files. That's not really a problem. You can do the remap with pkg_files if you want the runfiles support.
pkg_zip will work without problem, because it didn't add the weird crap in pkg_tar.

aiuto added a commit to aiuto/rules_pkg that referenced this issue Apr 12, 2022
- mostly fix bazelbuild#96 by finding executables and setting mode==755 on them

This new feature can not detect all executables. Bazel does not have the right
capability to make that easy. This seems to get most binaries, except shell.
You will have to wrap those in pkg_files to set the mode bits.
aiuto added a commit that referenced this issue May 10, 2022
* Support for setting executable bits.

- mostly fix #96 by finding executables and setting mode==755 on them

This new feature can not detect all executables. Bazel does not have the right
capability to make that easy. This seems to get most binaries, except shell.
You will have to wrap those in pkg_files to set the mode bits.
@adam-bedrock
Copy link

Hey, I'm currently running into this issue for go binaries that I want to deploy as AWS Lambda's. It seems like there's been some progress, is there an accepted best way to get pkg_zip to retain exectuable attribute.

@aiuto
Copy link
Collaborator

aiuto commented Mar 2, 2023

Do you have a reproduction? This says fixed from 9 moths ago?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug P2 An issue that should be worked on when time is available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants