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

Remove embedding v1 code in framework #144726

Merged
merged 21 commits into from
Mar 20, 2024

Conversation

gmackall
Copy link
Member

@gmackall gmackall commented Mar 6, 2024

Pre work for flutter/engine#51229. Removes a lot of code referencing v1 of the android embedding, though not necessarily all of it (I may have missed some, it is hard to know).

Will hopefully make landing that PR less painful (or maybe painless?)

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 6, 2024
return;
}
{{#methodChannelPlugins}}
{{class}}.registerWith(registry.registrarFor("{{package}}.{{class}}"));
Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting that this is where the lint in https://discord.com/channels/608014603317936148/846507907876257822/1214337088657293333 was coming from

// better way. This is just to simulate it so I can hunt down tests to
// modify or delete.
throwToolExit(
'Build failed due to use of deleted Android v1 embedding.',
Copy link
Member Author

Choose a reason for hiding this comment

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

This change and the other one should NOT land. They are just to simulate the v1 embedding being deleted, to help me track down tests that need to be fixed or removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noting after merging for anyone who comes across this pr in search: leaving this exit in place (this is likely the first failure point we will hit for someone trying to build a v1 embedding app, and it is good to have a clear message)

throwToolExit(
'Build failed due to use of deprecated Android v1 embedding.',
'Build failed due to use of deleted Android v1 embedding.',
Copy link
Member Author

Choose a reason for hiding this comment

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

The other one

@@ -16,6 +16,17 @@ import 'package:test/fake.dart';
import '../../src/context.dart';
import '../../src/test_flutter_command_runner.dart';

const String minimalV2EmbeddingManifest = r'''
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this to make the these pub get tests report v2 embedding, instead of v1, because the tests themselves were largely unrelated to testing android embedding behavior and thus it didn't make sense to delete them.

But I don't really know why running pub get should be at all linked to the android embedding version? I don't know why we compute and return the android embedding version here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems maybe analytics related, in which case it would also be a good candidate for removal (Not the test, but the behavior of linking the android embedding version to pub get here). We don't need analytics for deleted code.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does indeed look like this was analytics related: #44043

Copy link
Member Author

@gmackall gmackall Mar 12, 2024

Choose a reason for hiding this comment

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

I removed what I could of the analytics, and therefore also the changes to this test (it now works with no new manifest file, original comment in this thread is outdated). Some pieces of it, like this Event class, are not part of the flutter repo? I don't know where they are defined or what the process for removing the embedding version from that class would be like.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noting after merging for anyone who comes across this pr in search: I decided to not delete the analytics, as we will probably have a v3 embedding one day

@gmackall gmackall marked this pull request as ready for review March 12, 2024 19:15
@gmackall gmackall requested a review from a team March 12, 2024 19:34
@gmackall
Copy link
Member Author

This will likely break in post submit, but we will work through those failures if/when they happen

@gmackall gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 20, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 20, 2024
Copy link
Contributor

auto-submit bot commented Mar 20, 2024

auto label is removed for flutter/flutter/144726, due to - The status or check suite Mac tool_tests_general has failed. Please fix the issues identified (or deflake) before re-applying this label.

@gmackall gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 20, 2024
@auto-submit auto-submit bot merged commit 39bdff1 into flutter:master Mar 20, 2024
123 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 21, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 21, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 21, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 21, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 21, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Mar 21, 2024
flutter/flutter@b96c13d...18340ea

2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7cdb240c4a16 to bad4a30e1c75 (2 revisions) (flutter/flutter#145551)
2024-03-21 engine-flutter-autoroll@skia.org Roll Packages from 23e56af to b7fbe68 (3 revisions) (flutter/flutter#145547)
2024-03-21 pascal@welsch.dev Add WidgetsApp.debugShowWidgetInspectorOverride again (deprecated) (flutter/flutter#145334)
2024-03-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll pub packages (#145509)" (flutter/flutter#145550)
2024-03-21 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#145509)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 14b67475cf80 to 7cdb240c4a16 (1 revision) (flutter/flutter#145533)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6de3d9b6196a to 14b67475cf80 (1 revision) (flutter/flutter#145529)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from c0d3ac3178fa to 6de3d9b6196a (1 revision) (flutter/flutter#145520)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 23dc0cacc4db to c0d3ac3178fa (1 revision) (flutter/flutter#145519)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from a73e01364de0 to 23dc0cacc4db (2 revisions) (flutter/flutter#145517)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 45ed36c17bb7 to a73e01364de0 (1 revision) (flutter/flutter#145516)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 34b304a27f73 to 45ed36c17bb7 (1 revision) (flutter/flutter#145513)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 912c61f30512 to 34b304a27f73 (1 revision) (flutter/flutter#145511)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 98cfd9213332 to 912c61f30512 (1 revision) (flutter/flutter#145504)
2024-03-21 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#145476)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from fe6927c79dc3 to 98cfd9213332 (1 revision) (flutter/flutter#145498)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from d1fe8994dedf to fe6927c79dc3 (1 revision) (flutter/flutter#145493)
2024-03-20 bhesaniyavatsal@gmail.com Add helper widget parameter to InputDecoration (flutter/flutter#145157)
2024-03-20 34871572+gmackall@users.noreply.github.com Remove embedding v1 code in framework (flutter/flutter#144726)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b52f57ca07c to d1fe8994dedf (8 revisions) (flutter/flutter#145491)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from f803f2adec54 to 1b52f57ca07c (1 revision) (flutter/flutter#145479)
2024-03-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland #128236 "Improve build output for all platforms" (#145376)" (flutter/flutter#145487)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from fc2b7a9076b4 to f803f2adec54 (1 revision) (flutter/flutter#145475)
2024-03-20 6655696+guidezpl@users.noreply.github.com Reland #128236 "Improve build output for all platforms" (flutter/flutter#145376)
2024-03-20 engine-flutter-autoroll@skia.org Roll Packages from a2f4ce0 to 23e56af (5 revisions) (flutter/flutter#145470)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 883adfe2ef61 to fc2b7a9076b4 (1 revision) (flutter/flutter#145469)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: 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
auto-submit bot added a commit to flutter/engine that referenced this pull request Apr 9, 2024
Reverts: #51229
Initiated by: gmackall
Reason for reverting: blocking engine->framework roll (I missed some framework code referencing the v1 embedding).
Original PR Author: gmackall

Reviewed By: {matanlurey, reidbaker}

This change reverts the following previous change:
Fixes flutter/flutter#143531

Also fixes a random typo I found

~TODO to test this~ (no more todo):
-~test the framework against this as well, probably with a dummy PR changing the engine commit to my branch if this is possible~ not possible, made a best effort removal of framework code in flutter/flutter#144726.
-~figure out if the old embedding is used in g3 at all~ removed all uses
-~figure out exactly what the ShimPluginRegistry/ShimRegistrar are doing, and if fully deleting them was right~ (see #51229 (comment))

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…r#6370)

flutter/flutter@b96c13d...18340ea

2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7cdb240c4a16 to bad4a30e1c75 (2 revisions) (flutter/flutter#145551)
2024-03-21 engine-flutter-autoroll@skia.org Roll Packages from 23e56af to b7fbe68 (3 revisions) (flutter/flutter#145547)
2024-03-21 pascal@welsch.dev Add WidgetsApp.debugShowWidgetInspectorOverride again (deprecated) (flutter/flutter#145334)
2024-03-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll pub packages (#145509)" (flutter/flutter#145550)
2024-03-21 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#145509)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 14b67475cf80 to 7cdb240c4a16 (1 revision) (flutter/flutter#145533)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6de3d9b6196a to 14b67475cf80 (1 revision) (flutter/flutter#145529)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from c0d3ac3178fa to 6de3d9b6196a (1 revision) (flutter/flutter#145520)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 23dc0cacc4db to c0d3ac3178fa (1 revision) (flutter/flutter#145519)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from a73e01364de0 to 23dc0cacc4db (2 revisions) (flutter/flutter#145517)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 45ed36c17bb7 to a73e01364de0 (1 revision) (flutter/flutter#145516)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 34b304a27f73 to 45ed36c17bb7 (1 revision) (flutter/flutter#145513)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 912c61f30512 to 34b304a27f73 (1 revision) (flutter/flutter#145511)
2024-03-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 98cfd9213332 to 912c61f30512 (1 revision) (flutter/flutter#145504)
2024-03-21 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#145476)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from fe6927c79dc3 to 98cfd9213332 (1 revision) (flutter/flutter#145498)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from d1fe8994dedf to fe6927c79dc3 (1 revision) (flutter/flutter#145493)
2024-03-20 bhesaniyavatsal@gmail.com Add helper widget parameter to InputDecoration (flutter/flutter#145157)
2024-03-20 34871572+gmackall@users.noreply.github.com Remove embedding v1 code in framework (flutter/flutter#144726)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b52f57ca07c to d1fe8994dedf (8 revisions) (flutter/flutter#145491)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from f803f2adec54 to 1b52f57ca07c (1 revision) (flutter/flutter#145479)
2024-03-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland #128236 "Improve build output for all platforms" (#145376)" (flutter/flutter#145487)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from fc2b7a9076b4 to f803f2adec54 (1 revision) (flutter/flutter#145475)
2024-03-20 6655696+guidezpl@users.noreply.github.com Reland #128236 "Improve build output for all platforms" (flutter/flutter#145376)
2024-03-20 engine-flutter-autoroll@skia.org Roll Packages from a2f4ce0 to 23e56af (5 revisions) (flutter/flutter#145470)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 883adfe2ef61 to fc2b7a9076b4 (1 revision) (flutter/flutter#145469)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants