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

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

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

Conversation

Pinguin2001
Copy link

@Pinguin2001 Pinguin2001 commented Dec 8, 2024

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
Copy link
Member

yaira2 commented Dec 8, 2024

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

@Pinguin2001
Copy link
Author

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
Copy link
Member

yaira2 commented Dec 8, 2024

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

@Pinguin2001
Copy link
Author

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
Copy link
Author

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
Copy link
Member

0x5bfa commented Dec 9, 2024

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
Copy link
Member

yaira2 commented Dec 22, 2024

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

@0x5bfa
Copy link
Member

0x5bfa commented Dec 22, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants