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

feat: Add force_zip64 option #2711

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

devtbi
Copy link

@devtbi devtbi commented Mar 31, 2025

  • CLA signing not done yet
  • test?

Currently, there is no possibility to pass the force zip64 option to the wheel creation. This hinders creation of packages that contain >2Gb files (e.g. large projects with debug symbols). This change adds an argument to pass the force_zip to the wheel file creation.

@devtbi devtbi requested review from rickeylev and aignas as code owners March 31, 2025 12:15
@devtbi devtbi marked this pull request as draft March 31, 2025 12:16
@rickeylev
Copy link
Collaborator

zip64 is enabled by default, so shouldn't that obviate the need for forcing it to be enabled?

@devtbi
Copy link
Author

devtbi commented Apr 1, 2025

@rickeylev for my case it doesnt seem to suffice.. this is the output of the failure for the current main (7d431d8), which then works with forcing zip64:

~/<...>-mlir$ bazel build --config debug //packages/...
INFO: Analyzed 178 targets (331 packages loaded, 23354 targets configured).
ERROR: <...>/packages/<...>-iree/compiler/BUILD.bazel:668:9: Building wheel @@//packages/<...>-iree/compiler:compiler_python_wheel failed: (Exit 1): wheelmaker failed: error executing PyWheel command (from target //packages/<...>-iree/compiler:compiler_python_wheel) bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_python~/tools/wheelmaker --name iree_compile_bazel --version 0.0.1 --python_tag py3 --abi none --platform any --out ... (remaining 12 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
Traceback (most recent call last):
  File "/<...>/.cache/bazel/_bazel_devtbi/ab6f2c0bfbd923053b3e2b5e324ae76c/sandbox/processwrapper-sandbox/3/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_python~/tools/wheelmaker.runfiles/_main/../rules_python~/tools/wheelmaker.py", line 631, in <module>
    main()
  File "/<...>/.cache/bazel/_bazel_devtbi/ab6f2c0bfbd923053b3e2b5e324ae76c/sandbox/processwrapper-sandbox/3/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_python~/tools/wheelmaker.runfiles/_main/../rules_python~/tools/wheelmaker.py", line 541, in main
    maker.add_file(package_filename, real_filename)
  File "/<...>/.cache/bazel/_bazel_devtbi/ab6f2c0bfbd923053b3e2b5e324ae76c/sandbox/processwrapper-sandbox/3/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_python~/tools/wheelmaker.runfiles/_main/../rules_python~/tools/wheelmaker.py", line 304, in add_file
    self._whlfile.add_file(package_filename, real_filename)
  File "/<...>/.cache/bazel/_bazel_devtbi/ab6f2c0bfbd923053b3e2b5e324ae76c/sandbox/processwrapper-sandbox/3/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_python~/tools/wheelmaker.runfiles/_main/../rules_python~/tools/wheelmaker.py", line 157, in add_file
    with self.open(zinfo, "w") as fdst:
  File "/<...>/.cache/bazel/_bazel_devtbi/ab6f2c0bfbd923053b3e2b5e324ae76c/execroot/_main/external/python_3_11_x86_64-unknown-linux-gnu/lib/python3.11/zipfile.py", line 1201, in close
    raise RuntimeError("File size too large, try using force_zip64")
    ```

@rickeylev
Copy link
Collaborator

Oh hm. I think the usage of zip64 is inconsistent, depending on which zip APIs is used.

ZipFile() says it defaults to allow_zip64=True. Meanwhile, ZipFile.open (the API used in this code), defaults to force_zip64=False

Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Direction LGTM, but see comment

@@ -154,7 +156,7 @@ def arcname_from(name):
hash = hashlib.sha256()
size = 0
with open(real_filename, "rb") as fsrc:
with self.open(zinfo, "w") as fdst:
with self.open(zinfo, "w", force_zip64=self._force_zip64) as fdst:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just set force_zip64=True always. I don't think there's a disadvantage to always using zip64? Alternatively, stat() real_filename to get its size, and set it conditionally based on the size.

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

Successfully merging this pull request may close these issues.

2 participants