Allow deprecated members from the Dart SDK/Flutter Framework to roll in #6111
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
The only answer I see to my question about a replacement process covering this repo seems to have been a link to flutter/flutter#143344 which is described in the comment linking to it as "a catch-all issue for "I wish we had X tooling or process" and in the issue description says "It's also not clear who or when this work will get done". I would like some basic understand of the replacement process as it relates to this repository (even if it's not the idealized final process) before approving removing the process we have. Absent some specific replacement, I don't think this PR will be a net improvement for the reasons I described here. And for packages, which have to be published and then proactively picked up by users to avoid build breakage, delaying fixes from the deprecation point to the removal point isn't just a wash, it's a substantial net negative for all of our package clients in a way that is not true for the engine or framework repos, or for google3. |
This is a policy change where we are no longer letting a repo force upstream teams to account for deprecation errors, if you want to add a tax on yourself maintaining that repo that's you and your team's decision but it can't impact the framework engine or SDK teams.
We should have a tool that works on any Dart repository that can enumerate call sites of deprecated methods. As I wrote the previous sentence, it occurred to me that the analyzer cli must support identifying all calls to deprecated methods because we have to annotate our code to tell the analyzer to ignore this problem. So we have a production ready tool that can identify all of these issues on any given repository. Given this tool, I think there are two practical issues to tackle:
Would you feel more comfortable if I filed a more narrow bug around these changes? |
So does that mean the answer to my question is that no, there is no replacement process covering this repository, nor is there any concrete plan to create one?
I haven't seen anyone address my point that absent a robust replacement process, all this does is kick the can down the road and then impact the same people. If we land this and then don't do anything else, the process will change from:
to
And as noted above, in this repository, that timing change would be quite bad for all clients of these packages. I'm not going to LGTM a change that I don't believe will actually make things appreciably better for any of the upstream parts of our team but will absolutely make things worse for our customers. I'm happy to approve this once we actually have a concrete plan for a replacement process that will make this a net positive rather than a net negative (or alternately, if someone can explain to me what substantial benefit to the Flutter and Dart teams I am missing here that justifies making things worse for clients). |
I don't see how forcing a human to annotate all call sites with "// ignore: deprecated_method_use" is anything more than busy work that forces our static analysis tool to hide the problem from developers. The existing process matches my definition of papering over a problem and kicking the can down the road, and unfortunately, it also comes with a bunch of pointless busy work that interrupts people from doing impactful work. A serious way of dealing with the problem is to not hide the analysis errors from maintainers, this will encourage maintainers to update their down-stream code faster and actually deal with the problem instead of papering over it.
If someone lands a change that removes deprecated methods before all callers of that method are updated, that is a breaking change and should immediately reverted. I'll just state this explicitly: The developers that could land such a change are your colleagues and are working on the same project. This means that everyone's incentives are aligned such that your scenario above won't ever happen because removing the API before the callers have been fixed isn't acceptable and will immediately be reverted.
I've described a concrete plan on how to avoid adding new uses of deprecated methods, how we should surface these code health signals to developers on the team, and how we will deal with accidental breakages caused by removing a method before all of its callers are fixed. |
When they do that, they also have to (in this repo; I don't know what other repos enforce in code review) file an issue explaining what the required transition is, and leave TODOs on the So the current process requires someone to affirmatively engage with this repo and inject a work item into our stream that we have a process for tracking. If you want to argue there are better ways to do that, great, but we need an actual better replacement, not just to remove a system that, while not perfect, does work, without any concrete replacement.
I don't consider adding TODOs and a tracking issue triaged in our normal process to be hiding things from maintainers.
I don't think you're understanding the scope of the problem with respect to flutter/packages. The call sites are in packages published to pub, which means that in practice updating all call sites before removing the method does not just require someone to land changes here, it requires that, and also for every client to update their packages to get that change. It's a completely different dynamic than the Dart/engine/framework relationships. If we replace deprecated code shortly after it's deprecated (once it's possible to do so on stable), then the new version of the package without that code will, in practice, probably be published 6+ months before the API is removed. Clients then have 6+ months wherein they will, if they update their dependencies, be future-proof. The vast majority of clients will never even notice. If we replace the deprecated code when it's removed, then clients have at most 1-3 months (depending on when exactly it's removed in the
The developers that could land such a change do not work on the projects of the millions of Flutter clients, and thus cannot guarantee that all callers are fixed. No revert will happen when a bunch of people's projects break because the fix is for all of them to update their dependencies. But even if it's technically up to them to fix their own projects, making a case we know is a pain point and making it substantially more common hurts developer experience. What you are describing makes sense for usages in flutter/flutter, in flutter/engine, and in google3. It does not work in this repo because the call sites aren't here, they are massively distributed to machines we don't control. We can avoid breaking this repo's tree, sure, but not our clients. The goal of the CI in this repo is fundamentally to make sure the packages work as well as possible for our clients. |
|
Also, as more evidence that the flutter/packages use case does not appear to have been thought through in this policy change:
Such a feature would not be usable in this repository, because there is an up-to-three-month-window where adding uses of any given deprecated API may be required, in the very common case of an API being deprecated at the same time as the replacement is introduced (meaning the replacement isn't compatible with |
stuartmorgan
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/packages/6111, due to - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
stuartmorgan
added
the
autosubmit
|
auto label is removed for flutter/packages/6111, due to - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
bot
removed
the
autosubmit
Thanks Stuart, I do appreciate that. I'm happy to do the follow-up PR if you'd like. I am also serious about investing my own personal time into building tooling. I don't think there is consensus over what tooling what work for every repo, but I could imagine a couple of different possibilities (dashboards, github bots, custom CI checks). |
matanlurey
added
the
autosubmit
|
auto label is removed for flutter/packages/6111, due to - The status or check suite Mac_arm64 ios_platform_tests_shard_5 master has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
bot
removed
the
autosubmit
matanlurey
added
the
autosubmit
|
auto label is removed for flutter/packages/6111, due to - The status or check suite Mac_arm64 ios_platform_tests_shard_2 master has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
bot
removed
the
autosubmit
stuartmorgan
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/packages/6111, due to - The status or check suite Linux_android android_platform_tests_shard_6 master has failed. Please fix the issues identified (or deflake) before re-applying this label. |
stuartmorgan
added
the
autosubmit
flutter/packages@9385bbb...a864254 2024-02-14 [email protected] Allow deprecated members from the Dart SDK/Flutter Framework to roll in (flutter/packages#6111) 2024-02-14 [email protected] [google_maps_flutter][iOS 12] Skip `testTakeSnapshot` (flutter/packages#6120) 2024-02-13 [email protected] [ci] Allow dependencies on local_auth_ios (flutter/packages#6116) 2024-02-13 [email protected] [url_launcher] Add `InAppBrowserConfiguration` parameter in implementations (flutter/packages#5759) 2024-02-13 [email protected] [flutter_markdown] Use Text.rich to replace RichText in Flutter Markdown (flutter/packages#6062) 2024-02-13 [email protected] [google_maps_flutter][iOS 17] takeSnapshot FIX (flutter/packages#5823) 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-flutter-autoroll Please CC [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
As of #6111 `deprecated_member_use` is no longer on in the repository, so we no longer need (most; see below) of these annotations. Since we will not be annotating deprecated usage going forward, the associated comments with TODOs have also been removed, for consistency with the new process. This also opportunistically removes `deprecated_member_use_from_same_package` everywhere, since that has been disabled for a long time, so they were cruft. I have ensured that issues are filed for all of these usages, with the new `p: deprecated api` tag, per the new process for tracking deprecated APIs that is now described in the Ecosystem gardener rotation handbook. (In summary: there will be a manual weekly process of checking for new deprecations and filing them, and the [update-stable-in-this-repo process](https://github.com/flutter/flutter/wiki/Updating-Packages-repo-for-a-stable-release) will involve causing anything that is unblocked to be re-triaged.) The only annotations that are left are for cases where we have integration tests testing deprecated APIs in that package, as those are false positives; they are conceptually `deprecated_member_use_from_same_package` and aren't tech debt (or used by clients), but technically are from a different package since integration tests are in the example app. This will prevent them from showing up in the manual weekly check.
…in (flutter#6111) Namely, without breaking the tree. This is a deliberate policy decision change. See flutter/flutter#143312.
As of flutter#6111 `deprecated_member_use` is no longer on in the repository, so we no longer need (most; see below) of these annotations. Since we will not be annotating deprecated usage going forward, the associated comments with TODOs have also been removed, for consistency with the new process. This also opportunistically removes `deprecated_member_use_from_same_package` everywhere, since that has been disabled for a long time, so they were cruft. I have ensured that issues are filed for all of these usages, with the new `p: deprecated api` tag, per the new process for tracking deprecated APIs that is now described in the Ecosystem gardener rotation handbook. (In summary: there will be a manual weekly process of checking for new deprecations and filing them, and the [update-stable-in-this-repo process](https://github.com/flutter/flutter/wiki/Updating-Packages-repo-for-a-stable-release) will involve causing anything that is unblocked to be re-triaged.) The only annotations that are left are for cases where we have integration tests testing deprecated APIs in that package, as those are false positives; they are conceptually `deprecated_member_use_from_same_package` and aren't tech debt (or used by clients), but technically are from a different package since integration tests are in the example app. This will prevent them from showing up in the manual weekly check.
Namely, without breaking the tree. This is a deliberate policy decision change.
See flutter/flutter#143312.