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

Replace dated decompress lib for zip lib. #61

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jah-On
Copy link

@Jah-On Jah-On commented Dec 21, 2024

Description

The decompress library is a bit dated and would not compile on Windows on Arm. I replaced it with the zip library while keeping the function API the same.

Related

None.

Testing

Pointed the im-cli Cargo.toml to my repo to test changes and it compiled and ran as expected. Perhaps even better since it's not running through the translation system now :).


Checklist

  • [ X ] 🚨 This PR does not introduce breaking changes.
  • [ X ] All CI checks (GH Actions) pass.
  • [ X ] Documentation is updated as needed.
  • [ X ] Tests are updated or added as necessary.
  • [ X ] Code is well-commented, especially in complex areas.
  • [ X ] Git history is clean — commits are squashed to the minimum necessary.

@Hahihula
Copy link
Collaborator

Hahihula commented Jan 7, 2025

Hi Jah-On,

Thank you for your PR and for taking the time to improve the compatibility of the project for Windows on ARM! I really appreciate the effort you’ve put into replacing the decompress library while maintaining the function API.

However, there are a few issues with this change:

  1. Functionality Impact: The zip library you’ve chosen does not support .tar files, which is a critical requirement for this project. As the application needs to remain fully multi-platform, this replacement would unfortunately break functionality on other systems, even though it resolves the ARM64 Windows issue.

  2. Underlying Compatibility Issue: Replacing the decompress library alone isn’t enough to resolve the Windows ARM64 build issues. Both decompress and git2 libraries depend on libz-sys, which currently cannot be compiled for ARM64 Windows. This means the root cause lies deeper, and replacing decompress won’t resolve the issue entirely.

Given these challenges, we cannot accept this change as-is. However, we are actively working to address the compatibility issues for Windows ARM64 while ensuring the application remains multi-platform. I’ve opened a follow-up PR to address this and plan to introduce Windows ARM64 builds to our pipeline to catch and prevent such issues in the future.

Your contributions and insights are very much appreciated. Please feel free to review the linked PRs or share any further suggestions on how to tackle these challenges. Thank you again for your efforts, and I look forward to your future contributions!

Best regards,
Hahihula

@Jah-On
Copy link
Author

Jah-On commented Jan 7, 2025

Hi Hahihula,

Thank you for your detailed response. I apologize for the misunderstanding. I saw that the example usage for the method I had modified only contained ".zip" and incorrectly concluded that was the only file type of interest. Perhaps I can add support for tar files too. What specific tar extensions need to be supported?

As for git2, it may have been skipped when I was building im-cli? If it wasn't skipped then it had built fine. I may have tweaked the im-cli config file to use a newer version to get it to build. I don't recall anymore. If I did, it didn't introduce breaking API changes, if that helps.

Thanks,
John Schulz

@Hahihula
Copy link
Collaborator

Hahihula commented Jan 8, 2025

Hi Jah-On,
I already tried to address the decompressing issue in the #62 PR. It would be great if you could verify the arm64 Windows build is working for you.
Also, if you have not encountered troubles with git2 during the build, could you please tell me which target are you using for the build? I am still in the process of getting the arm64 windows pipeline up and running.
Thanks,
Hahihula

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