-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[flutter_tools] remove SkSL bundling and dump skp on compilation. #162849
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
Conversation
matanlurey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woot!
|
FYI @bkonyi |
bkonyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo for deleting code!
One question regarding the removal of the options, but generally LGTM.
| addDartObfuscationOption(); | ||
| usesDartDefineOption(); | ||
| usesExtraDartFlagOptions(verboseHelp: verboseHelp); | ||
| addBundleSkSLPathOption(hide: !verboseHelp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this option marked as deprecated before? If not, should we hold off on removing them completely and print an error when users try and provide them for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should hold off removing it, but we can call it out in the release notes. We already removed all website documentation on how to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern would be users who are still passing this flag on CI (assuming there are any) will be broken if we remove this option. Hiding it unconditionally and printing a warning will at least notify them that the option doesn't do anything and will be removed.
SkSL precompilation was only ever beneficial for iOS. For other platforms, we recommended against it as Skia generated shaders per target architecture which could be invalid on other devices. It is no longer possible to use Skia on iOS.
Delete all Skia shader bundling logic.
Fixes #80091