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

Incorporate @compnerd's patch for Windows support #9

Closed
timsneath opened this issue Sep 5, 2024 · 7 comments · Fixed by #10
Closed

Incorporate @compnerd's patch for Windows support #9

timsneath opened this issue Sep 5, 2024 · 7 comments · Fixed by #10
Assignees
Labels
enhancement New feature or request

Comments

@timsneath
Copy link

It would be great if we could incorporate https://github.com/marmelroy/Zip/pull/246/files, which would enable this to work on Windows. Today there's no well-maintained ZIP implementation that includes Windows support, and you're achingly close!

@fpseverino
Copy link
Member

I've implemented the code from the original PR in #10, but I'm having some trouble with the CI, as I am not able to pass a build of zlib. Maybe @compnerd could give us a hand?

@fpseverino fpseverino linked a pull request Sep 6, 2024 that will close this issue
@timsneath
Copy link
Author

timsneath commented Sep 7, 2024

I think (from the actions log), the problem is that tar only supports the Zip format in Windows 11 builds and later, and the GitHub Actions windows-latest image is Windows Server 2022, which is based on Windows 10 21H2.

It looks like the runner image does include 7Zip, so you can probably replace the tar line with something like:

7z x archive.7z -opath/to/dir

@fpseverino
Copy link
Member

Ok, now zilb is correctly downloaded and linked.

All tests succeed except for a important one. I'll do what I can in the coming days.

We are almost there! 😃

@fpseverino
Copy link
Member

That test I previously mentioned fails because of two reasons:

  • The zip file used in the test has colons in some of the dir/file names it contains, which are not valid characters in Windows file names.
    • This zip file was zipped with vapor-community/Zip itself. Changing it to an identical file zipped with Finder seems to fix the issue.
    • I'm unable to test if a file (containing colons in its name) zipped with vapor-community/Zip on Windows can be unzipped (or even if it can be zipped). If it's zipped in Linux/macOS it can't be unzipped on Windows.
  • Permissions are very different in Windows. Copilot helped me converting the POSIX ones, but there's a single file to which it fails to set the correct permissions.

Right now the solution to the colons problem is avoiding it, and the one for the permissions problem is an ugly hack that doesn't even work all the time. Of course more work is needed. If you know more on Windows permissions I would appreciate your help!

@fpseverino
Copy link
Member

I've managed to remove colons in file names and I've removed any permissions setting specific to Windows, now all the test succeed.

I've marked the PR as ready to review and requested some reviews. When it gets merged I'll do a new patch release, but I think that more real world testing will be needed.

@fpseverino
Copy link
Member

Unfortunately, because of issues in the C code I'm still investigating, the package doesn't work with Swift 6.0 in Windows.

@fpseverino
Copy link
Member

Thanks to @compnerd's help we managed to make it working, now the PR is again ready to review!

fpseverino added a commit that referenced this issue Nov 7, 2024
### Issue #9 

Originally authored by @compnerd in marmelroy#246

- Add support for Windows
- Add Windows CI tests
- Drop support for Swift 5.8
- Adopt `swift-format`
- Add CI for iOS and MUSL
- Various bug fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

2 participants