-
Notifications
You must be signed in to change notification settings - Fork 29k
Revert "[web:tools] always use CanvasKit from the cache when building web apps (#93002)" #117693
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
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.
LGTM!
@eyebrowsoffire in order for this to fully work, we need to change our engine build so that the canvaskit files are generated in |
Sounds good to me. It sounds like @zanderso has some notes on that PR and so we'll wait on merging this until those are resolved. |
@mdebbar are you still planning to land this? |
… web apps (flutter#93002)" This reverts commit 7737893.
cc @yjbanov I would love to get your review on this one since you are the author of the PR being reverted. |
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 think you missed a spot. Otherwise, LGTM.
@@ -511,15 +510,7 @@ class WebBuiltInAssets extends Target { | |||
|
|||
@override | |||
Future<void> build(Environment environment) async { | |||
// TODO(yjbanov): https://github.com/flutter/flutter/issues/52588 |
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.
There's a similar TODO here that also needs to be cleaned up:
File _canvasKitFile(String relativePath) { |
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.
Ah, that TODO was added in a different PR than the one I'm reverting. That's why I didn't catch it.
I also made other changes:
- Use the new
Artifacts.canvasKitPath
to accurately locate the canvaskit files. - Stop removing the
/flutter_web_sdk/
segment from paths inbin/cache
(it's not needed anymore after Skwasm Renderer - initial implementation engine#39072).
@yjbanov PTAL :)
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.
…building web apps (#93002)" (flutter/flutter#117693)
…building web apps (#93002)" (flutter/flutter#117693)
…building web apps (#93002)" (flutter/flutter#117693)
…building web apps (#93002)" (flutter/flutter#117693)
… web apps (flutter#93002)" (flutter#117693) Revert "[web:tools] always use CanvasKit from the cache when building web apps (flutter#93002)"
…building web apps (#93002)" (flutter/flutter#117693)
…building web apps (#93002)" (flutter/flutter#117693)
…building web apps (#93002)" (flutter/flutter#117693)
…building web apps (#93002)" (flutter/flutter#117693)
This reverts commit 7737893.
Now that CanvasKit is built from sources, let's revert #93002 so that the locally built CanvasKit is copied to the app's
build/web/
folder.Depends on flutter/engine#38448
Part of #118799