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

zip and unzip API #317

Merged
merged 48 commits into from
Oct 7, 2024
Merged

zip and unzip API #317

merged 48 commits into from
Oct 7, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Oct 7, 2024

Pulls in changes from #316 and #310 and cleans it up

Mostly documented in the readme.adoc.

Major APIs added:

  • os.zip: create or append to an existing zip file on disk
  • os.zip.stream: create a new zip file but write it to an java.io.OutputStream rather than a file on disk
  • os.unzip: unzip a zip file on disk into a folder on disk
  • os.unzip.stream: unzip a zip file from an java.io.InputStream into a folder on disk
  • os.unzip.list: list the contents of a zip file
  • os.unzip.streamRaw: low-level API used by os.unzip.stream and os.unzip.list, exposed in case users need it
  • os.zip.open: Opens a zip file as java.nio.file.FileSystem and gives you an os.Path you can use to work with it

Hopefully these are APIs we can start using in Mill rather than "zip" subprocesses or ad-hoc helpers like IO.unpackZip

Limitations:

  • Use of java.nio.file.FileSystem is only supported on JVM and not on Scala-Native, and so using os.zip to append to existing jar files or os.zip.open does not work on Scala-Native.
  • Also os.zip doesn't support creating/unpacking symlinks or preserving filesystem permissions in Zip files, because the underlying java.util.zip.Zip*Stream doesn't support them. Apache Commons Compress can work with them (https://commons.apache.org/proper/commons-compress/zip.html), but if we're sticking with std lib we don't have that
  • Bumps the version requirement to Java 11 and above, matching the direction of the rest of com-lihaoyi. Probably not strictly necessary, but we have to do it eventually and now is as good a time as ever with requests already bumped and Mill bumping soon in 0.12.0

chaitanyawaikar and others added 21 commits September 29, 2024 13:22
… file. The zipIn functionality will create a new file `archive.zip` if the name of the zip file is absent
…irectory if the target directory is not present.
- Rename `zipIn` to `zip`
- Remove `options` parameter and replace with individual flags i.e `appendToExisting`
- Include only given files during zipping (similar to -i option in zip)
- Exclude given files during zipping (similar to -x option in zip)
- Delete given files from the zip archieve (similar to -d option in zip)

Tests added along with the implementation
- listing files without unzipping
- excluding certain files/file regex during unzipping
@lihaoyi lihaoyi requested a review from lefou October 7, 2024 10:06
@lihaoyi lihaoyi requested a review from lolgab October 7, 2024 10:06
@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 7, 2024

@lefou @lolgab could use a review on this if you have time before I merge this, since this is a new API in a pretty wide design space.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

I think it looks good. OS-Lib is still in version 0 phase, so we can change API of new features rather easy. Let's release it so we get feedback from users.

Why was Java 8 dropped in CI?

- os: macos-latest
java-version: 11
os: [ubuntu-latest, windows-latest, macos-latest]
java-version: [11, 17]
Copy link
Member

@lefou lefou Oct 7, 2024

Choose a reason for hiding this comment

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

Why is that? Is this just convenient or necessary? I'd like to avoid dropping support for a Java version in a minor release. If we need to drop it, we should bump to 0.11.0. Keeping bin-compat with 0.10.x is a bonus feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just convenience really, but i realized we already have os.Internal.transfer so I can just use that just as conveniently

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like there's some issue with zip filesystem on windows java 8, so need to bump that to java 11 as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I think given that we now have 2/3 OSs only testing on Java 11, let's just bump the official supported version to 11. People can continue to try using it on 8, it'll just be at their own risk

@lihaoyi lihaoyi merged commit bcd84b9 into main Oct 7, 2024
8 checks passed
@lihaoyi lihaoyi deleted the 316 branch October 7, 2024 18:46
@lefou lefou added this to the after 0.10.7 milestone Oct 7, 2024
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.

3 participants