search

files-community / Files

A modern file manager that helps users organize their files and folders.
https://files.community
MIT License
35.52k stars 2.26k forks source link

Code Quality: Only bundle architecture specific 7z dll #16584

Closed Pinguin2001 closed 5 days ago

Pinguin2001 commented 1 month ago

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

  1. Build files for all three architectures
  2. Verify only the architecture specific dll has been copied

While looking into the outdated 7z dlls, I noticed files bundles all three dlls regardless of the architecture being built. This pr addresses this issue

yaira2 commented 1 month ago

Btw, I tried this once before, but it broke the installation. https://github.com/files-community/Files/pull/15690/commits/7e288451026515d9bcfb4c82317185446c1d79cc

Pinguin2001 commented 1 month ago

Btw, I tried this once before, but it broke the installation. 7e28845

Which issue occurred? I don't see how this could affect msix-based app installation

yaira2 commented 1 month ago

Users couldn't update or install the new version due to resource mapping issue.

Pinguin2001 commented 1 month ago

Users couldn't update or install the new version due to resource mapping issue.

This seems odd, can you generate a signed test appx with and without the change and try it for yourself? This really doesn't seem like intended behavoir at all

Pinguin2001 commented 1 month ago

I will generate a test signed msix as well and update from that to the one with the other two dlls removed in a virtual machine and on a secondary pc. Will report back my tests

0x5bfa commented 1 month ago

This doesn't work in CD (the side loading pipeline) because we configure it to generate x64|arm64 msixbundle and configure building to be only x64. I like this size reduction improvement but unless we configure CD to build 2 times for each x64 and arm64 and then bundle them into msixbundle we can't enjoy this improvement.

If we want, we can re-configure CD to build two times and bundle them AND we don't have to use WAP project any longer, plus I doubt this would break auto-update since CDN package URL doesn't change.

  1. Build two times for x64 and arm64
  2. Bundle these msix files to msixbundle
  3. Bundle msixbundle and appxsym files for each configuration into msixupload into zip file
yaira2 commented 4 weeks ago

Aside from the workaround to use msixbundler, are there any other paths forward?

0x5bfa commented 4 weeks ago

I don't know. I guess it's easier.

yaira2 commented 5 days ago

@Pinguin2001 sorry for the delay in responding. We're definitely interested in implementing this improvement, but our current packaging setup isn't compatible with this change, as @0x5bfa pointed out. We hope to make the necessary adjustments in the future, but right now, we lack the resources to do so, which means we'll have to put this on hold.

We'll keep you updated on our progress in #16583. Also, it's worth mentioning that we're looking forward to a significant reduction in package size once we enable Native AOT, something we've been working towards for some time now. Keep an eye on the issues related to Vanara and the Windows Community Toolkit for more details.