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

let NOTICES be double gzip wrapped to reduce on-disk installed space #71899

Merged
merged 8 commits into from Dec 15, 2020

Conversation

xster
Copy link
Member

@xster xster commented Dec 8, 2020

Fixes #71102. It doesn't quite deduplicate the contents of NOTICES while in transport, but just leaves it compressed on disk until it's actually read since it's an uncommonly used file. Reduces uncompressed (.ipa perspective, not this file's perspective) size from ~800kB before to ~60kB after.

It leaves web as is since the client doesn't have dart:io or any "native" abilities to unzip. It also has a .Z extension instead of a .gz extension because there are logic baked in in gradle that traverses all the files of an APK and unzips the .gz files. Checked with the gradle team. Our usage here isn't circumventing any meaningful optimizations. It was just originally meant to avoid people thinking this reduces network size (which this doesn't and doesn't intend to) and costing unzip CPU cost (which doesn't affect this since it's uncommonly used).

Added some new tests but https://github.com/flutter/flutter/pull/71899/files#diff-7e359e27713e3f080b4385c7e6b690e7aa3ab982c98855a088ed00e07bfaa6e5R16 actually ends up making this get tested everywhere.


For future rollers, this will break g3. Subclasses will need to implement the new signature to match.

@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Dec 8, 2020
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@google-cla google-cla bot added the cla: yes label Dec 8, 2020
@xster xster marked this pull request as draft December 8, 2020 08:30
@flutter-dashboard flutter-dashboard bot added the team Infra upgrades, team productivity, code health, technical debt. See also team: labels. label Dec 12, 2020
@xster xster requested a review from jmagman December 12, 2020 08:15
@xster xster marked this pull request as ready for review December 12, 2020 08:15
@jmagman
Copy link
Member

jmagman commented Dec 14, 2020

7za isn't available on LUCI
https://ci.chromium.org/ui/p/flutter/builders/try/Windows%20module_test/1747/overview

  "success": false,
  "reason": "Invalid argument(s): Cannot find executable for 7za."

You can see the powershell fallbacks here:

If (Get-Command 7z -errorAction SilentlyContinue) {
# The built-in unzippers are painfully slow. Use 7-Zip, if available.
& 7z x $dartSdkZip "-o$cachePath" -bd | Out-Null
} ElseIf (Get-Command 7za -errorAction SilentlyContinue) {
# Use 7-Zip's standalone version 7za.exe, if available.
& 7za x $dartSdkZip "-o$cachePath" -bd | Out-Null
} ElseIf (Get-Command Microsoft.PowerShell.Archive\Expand-Archive -errorAction SilentlyContinue) {
# Use PowerShell's built-in unzipper, if available (requires PowerShell 5+).
Microsoft.PowerShell.Archive\Expand-Archive $dartSdkZip -DestinationPath $cachePath
} Else {
# As last resort: fall back to the Windows GUI.
$shell = New-Object -com shell.application
$zip = $shell.NameSpace($dartSdkZip)
foreach($item in $zip.items()) {
$shell.Namespace($cachePath).copyhere($item)
}
}

@xster
Copy link
Member Author

xster commented Dec 15, 2020

Thanks for the tip. I ended up just unpacking the APK in dart instead of shelling out.

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

The code LGTM. Can you give this a TestFlight run to confirm there's no unexpected consequences to publishing an app with that embedded file type?

@xster
Copy link
Member Author

xster commented Dec 15, 2020

@renyou
Copy link
Contributor

renyou commented Dec 18, 2020

g3 rollers: subclasses must implement https://github.com/flutter/flutter/pull/71899/files#diff-1339eded5eeb35ab9fbc61051c530c73e307d4640328760c1af0eee36c142be2R172.

Hi, @xster , actually I didn't see this comment until now. Could you please communicate on the internal channels next time so we can track this sooner? Thanks!

@xster
Copy link
Member Author

xster commented Dec 18, 2020

@renyou will do. Though this one in particular was reverted, so no action needed in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deduplicate NOTICES
4 participants