Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Enable logging a warning when the user opts out of using Impeller. #51849

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

chinmaygarde
Copy link
Member

Part of flutter/flutter#144439

This does two things:

  • Logs a warning when the embedder requests a non-Impeller preference when creating a shell.
  • Makes the iOS embedder request a warning be logged when Impeller is not used.

I decided to put the warning logs in the shell so that as we get more opinionated about Impeller on other platforms, those platforms can just flip a flag with common log origin.

…mpeller.

Part of flutter/flutter#144439

This does two things:

* Logs a warning when the embedder requests a non-Impeller preference
  when creating a shell.
* Makes the iOS embedder request a warning be logged when Impeller is
  not used.

I decided to put the warning logs in the shell so that as we get more
opinionated about Impeller on other platforms, those platforms can just
flip a flag.
@jonahwilliams
Copy link
Member

Two thoughts:

  • If the goal is to flip the default for Q3 stable, this won't actually land in a stable release before that. From the perspective of a user on stable, they will never see this warning - you'd need to cherry pick it into the current beta. If the plan is Q4 then that is plenty of time.

  • We don't currently have anyone working on migrating the rest of the g3 users. You should reach out to @xster and see if the FL@G team can follow up or if there are known blockers

@chinmaygarde
Copy link
Member Author

chinmaygarde commented Apr 2, 2024

If the goal is to flip the default for Q3 stable, this won't actually land in a stable release before that. From the perspective of a user on stable, they will never see this warning - you'd need to cherry pick it into the current beta. If the plan is Q4 then that is plenty of time.

Good point. For some reason, I thought this had to wait past the last stable release cut. Can CP if the plan is to flip the default in Q3.

We don't currently have anyone working on migrating the rest of the g3 users. You should reach out to @xster and see if the FL@G team can follow up or if there are known blockers

It was tracked in flutter/flutter#144739 but @matanlurey just unassigned himself about an hour ago since he has higher priority tasks. I can initiate the followup. Push comes to shove, I don't think we have to necessarily conflate moving to Impeller only builds in 3P with 1P because the release models are different.

@jonahwilliams
Copy link
Member

Right, we don't have to align the two at all, but we might just want to give the fl@g folks a heads up. CPing this change seems reasonable to me.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 3, 2024
@auto-submit auto-submit bot merged commit e9b3a05 into flutter:main Apr 3, 2024
28 checks passed
@chinmaygarde chinmaygarde deleted the warn_no_impeller branch April 3, 2024 19:54
@chinmaygarde chinmaygarde added the cp: stable cherry pick to the stable release candidate branch label Apr 3, 2024
@flutteractionsbot
Copy link

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

@jonahwilliams
Copy link
Member

If you use the CP label that will go to the previous stable 3.19. If you want it to go to the upcoming stable you'll need to wait until the beta is released and then do a beta cp.

@chinmaygarde
Copy link
Member Author

So, whatever points to flutter-3.21-candidate.0 right? I believe that is cp: beta then. I thought 3.21 was already out.

@chinmaygarde
Copy link
Member Author

A bit confused about the timing. Will bring it up during the sync.

@XilaiZhang
Copy link
Contributor

Yeah current stable is 3.19 and current beta is 3.21.

the tool finds current beta and current stable based on static version files that are updated when new beta/stable are created

the tool isn't aware of future branches. (because it's hard to predict which branch will be a future beta for example)

@chinmaygarde
Copy link
Member Author

I need to get this into 3.21 stable. I should CP this to flutter-3.21-candidate.0 by using the cp: beta label right?

@zanderso
Copy link
Member

zanderso commented Apr 3, 2024

The stable will be 3.22, so you want to CP this change to flutter-3.22-candidate.0. It's too soon to do that with the magic tag, but you can create the CP PR manually.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 3, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 3, 2024
…146240)

flutter/engine@349608d...18fdcad

2024-04-03 [email protected] Roll Skia from 8d7482b998d0 to 7d6dce620e46 (1 revision) (flutter/engine#51878)
2024-04-03 [email protected] Be slightly more lenient about the assertion, as it differs on different backends. (flutter/engine#51877)
2024-04-03 [email protected] Roll Skia from afa233bb1979 to 8d7482b998d0 (2 revisions) (flutter/engine#51876)
2024-04-03 [email protected] [Impeller] Enable logging a warning when the user opts out of using Impeller. (flutter/engine#51849)
2024-04-03 [email protected] [Impeller] delete unused code. (flutter/engine#51871)
2024-04-03 [email protected] [Impeller] eliminate sub-render pass for blended color + texture vertices. (flutter/engine#51778)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@Jasguerrero
Copy link
Contributor

This is breaking google testing b/332791027

@matanlurey
Copy link
Contributor

This is breaking google testing b/332791027

I commented on the bug, but any FL@G specific tooling should be routed to the FL@G team first.

The Engine/Impeller team doesn't have any idea about how these tools are built and we don't guarantee a stable stdout.

chinmaygarde added a commit that referenced this pull request Apr 4, 2024
…mpeller. (#51849)

Part of flutter/flutter#144439

This does two things:

* Logs a warning when the embedder requests a non-Impeller preference when creating a shell.
* Makes the iOS embedder request a warning be logged when Impeller is not used.

I decided to put the warning logs in the shell so that as we get more opinionated about Impeller on other platforms, those platforms can just flip a flag with common log origin.
auto-submit bot pushed a commit that referenced this pull request Apr 8, 2024
…ng Impeller. (#51849) (#51914)

Part of flutter/flutter#144439

This does two things:

* Logs a warning when the embedder requests a non-Impeller preference when creating a shell.
* Makes the iOS embedder request a warning be logged when Impeller is not used.

I decided to put the warning logs in the shell so that as we get more opinionated about Impeller on other platforms, those platforms can just flip a flag with common log origin.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App cp: stable cherry pick to the stable release candidate branch platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants