System for making changes to package:macros without breaking the flutter engine roll

[srawlins]

See https://github.com/flutter/flutter/issues/149093.

As I understand our system today: every change to the macros package requires a release. And every macros package release requires a specific, new companion analyzer package. And the analyzer package is pinned somewhere in flutter/flutter.

So any time the macros package, which is vendored with the Dart SDK, is changed, the flutter engine -> framework autoroll breaks. In order for it to not break, a new version of the analyzer package would need to be available on pub, and the flutter framework would need to be able to accept that new version.

CC @jakemac53 @scheglov @zanderso @a-siva

[jakemac53]

Nit: not every change. Only changes to the APIs which are implemented by the analyzer, or breaking changes to APIs used by the analyzer.

But yes, whenever a macros release requires a change in the analyzer, those packages have to be updated in lock step. The fact that one actually comes from the SDK means it has to happen as a part of the engine -> framework roll.

[a-siva]

But yes, whenever a macros release requires a change in the analyzer, those packages have to be updated in lock step. The fact that one actually comes from the SDK means it has to happen as a part of the engine -> framework roll.

How often do we have the situation where a change in the analyzer is required? Is it possible to ensure that the macros release, analyzer release and the engine->framework roll happen in lock step ?

[jakemac53]

One solution I proposed in the linked issue, is to allow version constraints for certain packages, instead of pinning them, in the framework. So for example analyzer: >=6.5.0 <6.5.2 instead of analyzer: 6.5.1.

[jakemac53]

How often do we have the situation where a change in the analyzer is required?

It is difficult to predict, in the short term they will be more common, as we evolve the API.

In the long term, we are looking into solutions to avoid the SDK vendored package entirely.

Is it possible to ensure that the macros release, analyzer release and the engine->framework roll happen in lock step ?

We could look into processes to ensure this, such as requiring an analyzer release to be prepared and landed together with any minor release of package:macros (analyzer depends on minor release versions because it implements the APIs, and that is the contract we have established). In theory we could also automate the publishing step, we have setups for that in GitHub repos already. I don't know what it would take to enable that for SDK packages.

This hasn't been the pain point really though, it is the actual engine -> framework roll. There are already the required versions of things published and available. The issue is the framework pins to old versions which aren't compatible with the new SDK being rolled from the engine.

[bwilkerson]

I haven't really paid much attention to this in the past, but how do we ensure that the version of the analyzer that's pinned in flutter matches the version of the analyzer that's being used by the Dart SDK?

Can we end up in a situation where flutter analyze and dart analyze behave differently because of a version skew between the two analyzer versions being run?

[srawlins]

I haven't seen the wiring in a while, but I'm 95% sure flutter analyze calls out to the analysis_server snapshot. It doesn't use any analyzer package from pub.

[bwilkerson]

Then that begs the question of why there's a version of the analyzer package being rolled into flutter? What is it being used for?

[srawlins]

git grep shows the following:

dev/bots/analyze.dart:import 'package:analyzer/dart/analysis/features.dart';
dev/bots/analyze.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/bots/analyze.dart:import 'package:analyzer/dart/analysis/utilities.dart';
dev/bots/analyze.dart:import 'package:analyzer/dart/ast/ast.dart';
dev/bots/analyze.dart:import 'package:analyzer/dart/ast/visitor.dart';
dev/bots/custom_rules/analyze.dart:import 'package:analyzer/dart/analysis/analysis_context.dart';
dev/bots/custom_rules/analyze.dart:import 'package:analyzer/dart/analysis/analysis_context_collection.dart';
dev/bots/custom_rules/analyze.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/bots/custom_rules/analyze.dart:import 'package:analyzer/dart/analysis/session.dart';
dev/bots/custom_rules/avoid_future_catcherror.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/bots/custom_rules/avoid_future_catcherror.dart:import 'package:analyzer/dart/ast/ast.dart';
dev/bots/custom_rules/avoid_future_catcherror.dart:import 'package:analyzer/dart/ast/visitor.dart';
dev/bots/custom_rules/avoid_future_catcherror.dart:import 'package:analyzer/dart/element/type.dart';
dev/bots/custom_rules/no_double_clamp.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/bots/custom_rules/no_double_clamp.dart:import 'package:analyzer/dart/ast/ast.dart';
dev/bots/custom_rules/no_double_clamp.dart:import 'package:analyzer/dart/ast/visitor.dart';
dev/bots/custom_rules/no_double_clamp.dart:import 'package:analyzer/dart/element/element.dart';
dev/bots/custom_rules/no_double_clamp.dart:import 'package:analyzer/dart/element/type.dart';
dev/bots/custom_rules/no_stop_watches.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/bots/custom_rules/no_stop_watches.dart:import 'package:analyzer/dart/ast/ast.dart';
dev/bots/custom_rules/no_stop_watches.dart:import 'package:analyzer/dart/ast/visitor.dart';
dev/bots/custom_rules/no_stop_watches.dart:import 'package:analyzer/dart/element/element.dart';
dev/bots/custom_rules/no_stop_watches.dart:import 'package:analyzer/dart/element/type.dart';
dev/bots/custom_rules/render_box_intrinsics.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/bots/custom_rules/render_box_intrinsics.dart:import 'package:analyzer/dart/ast/ast.dart';
dev/bots/custom_rules/render_box_intrinsics.dart:import 'package:analyzer/dart/ast/visitor.dart';
dev/bots/custom_rules/render_box_intrinsics.dart:import 'package:analyzer/dart/element/element.dart';
dev/bots/custom_rules/render_box_intrinsics.dart:import 'package:analyzer/dart/element/type.dart';
dev/bots/utils.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/bots/utils.dart:import 'package:analyzer/dart/ast/ast.dart';
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/dart/analysis/features.dart';
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/dart/analysis/utilities.dart';
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/dart/ast/ast.dart';
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/dart/ast/token.dart';
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/dart/ast/visitor.dart';
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/file_system/file_system.dart' as afs;
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/file_system/physical_file_system.dart' as afs;
dev/snippets/lib/src/analysis.dart:import 'package:analyzer/source/line_info.dart';
dev/snippets/lib/src/import_sorter.dart:import 'package:analyzer/dart/analysis/features.dart';
dev/snippets/lib/src/import_sorter.dart:import 'package:analyzer/dart/analysis/results.dart';
dev/snippets/lib/src/import_sorter.dart:import 'package:analyzer/dart/analysis/utilities.dart';
dev/snippets/lib/src/import_sorter.dart:import 'package:analyzer/dart/ast/ast.dart';
dev/snippets/lib/src/import_sorter.dart:import 'package:analyzer/dart/ast/token.dart';
dev/snippets/lib/src/import_sorter.dart:import 'package:analyzer/error/error.dart';
dev/snippets/lib/src/import_sorter.dart:import 'package:analyzer/source/line_info.dart';
dev/snippets/test/filesystem_resource_provider.dart:import 'package:analyzer/file_system/file_system.dart';
dev/snippets/test/filesystem_resource_provider.dart:import 'package:analyzer/src/generated/source.dart';
dev/snippets/test/filesystem_resource_provider.dart:import 'package:analyzer/src/source/source_resource.dart';

The imports in analyze.dart seem mostly to determine test skips: https://github.com/flutter/flutter/blob/124aacbaef3ff0476f3a02c2ab05271e152224db/dev/bots/analyze.dart#L823-L854

(analysis.dart is separate; this is the code behind flutter analyze, which executes analysis_server.dart.snapshot.)

[bwilkerson]

Thanks! I'm not seeing anything in that list that looks like it would cause problems for users if the two versions are out of date.

The biggest risk I can see is if the pinned version of analyzer is incompatible with the pinned version of one of it's dependencies, but hopefully there are checks in place to prevent that (other than depending on a compilation error).

It still isn't clear why they aren't just depending on the version of the package in the SDK, but maybe because it would feel like a hack to add a dependency override?

[jakemac53]

The biggest risk I can see is if the pinned version of analyzer is incompatible with the pinned version of one of it's dependencies, but hopefully there are checks in place to prevent that (other than depending on a compilation error).

Flutter repo packages still do use pub so the solve will fail if version constraints are incompatible. So, assuming we version correctly this shouldn't be an issue.

It still isn't clear why they aren't just depending on the version of the package in the SDK, but maybe because it would feel like a hack to add a dependency override?

If by "they" you mean the flutter/flutter repo, there is no version of analyzer present in the SDK. They have a vendored Dart SDK which is equivalent to some dev release, and we don't ship analyzer sources with released SDKs.

[a-siva]

We seem to have hit this issue again with the change https://dart-review.googlesource.com/c/sdk/+/370120

The engine to framework rolls have been failing for a day now, see https://github.com/flutter/flutter/pull/150084

[bwilkerson]

Does the analyzer package need to be published in order to fix the immediate problem? If so that's doable.

More importantly, though, what process do we need to put in place to ensure that this doesn't happen again?

[mit-mit]

how do we ensure that the version of the analyzer that's pinned in flutter matches the version of the analyzer that's being used by the Dart SDK?

Does the Dart SDK pin to an exact version of the analyzer package? Because if so, I think that is sufficient reason to simply unpin the analyzer in Flutter, as the whole reason for pinning in Flutter is to ensure everyone uses the same single version.

[bwilkerson]

Does the Dart SDK pin to an exact version of the analyzer package?

The analyzer package is developed in the SDK (sdk/pkg/analyzer), so in that sense the SDK always pins to HEAD. But no, the SDK doesn't pin to a published version of the analyzer package.

Is there any way for Flutter to pin to the version in the vended in SDK?

[srawlins]

Is there any way for Flutter to pin to the version in the vended in SDK?

The only way I can imagine doing this is to use a git: key in the pubspec, pointing to a Gerrit URL for the SHA that is being rolled into flutter/engine. Not saying it's a good idea, but I can't imagine another source for the package:analyzer source files in play at any given Dart SDK commit being rolled into flutter/engine.

[jakemac53]

We seem to have hit this issue again with the change https://dart-review.googlesource.com/c/sdk/+/370120

This is a different kind of breakage, we did/do not need a new version of analyzer published.

The issue was that the macros package was blocked on being published because of https://github.com/dart-lang/pub/issues/4305 (well actually, I discovered later I could have published from the beta channel with --skip-validation, but didn't know that at the time).

So, this recent blockage was not really the result of any process needing to be changed/improved, but instead the existing process (to publish package:macros after landing changes) was blocked on an unrelated issue so it couldn't be completed.

[davidmorgan]

https://github.com/dart-lang/language/issues/3904 for discussion of whether we should switch back to internal-SDK-hosted for the macros dep until we are through with breaking changes.

[zanderso]

Hitting this again on the roll to Flutter: https://github.com/flutter/flutter/pull/154731.

[scheglov]

A new analyzer version was published shortly after the macros package was landed into the SDK. Is this not enough to solve the roll problem? Was it not soon enough?

[zanderso]

The issue is that these changes break the autorollers, requiring the team to drop everything and do a manual roll as a P0. I think the overall approach here needs to be given some more thought so that we don't need to do these manual updates as a fire drill anymore.

[jakemac53]

The issue is that these changes break the autorollers, requiring the team to drop everything and do a manual roll as a P0. I think the overall approach here needs to be given some more thought so that we don't need to do these manual updates as a fire drill anymore.

Which part of this is requiring a manual roll though?

[jakemac53]

Oh, re-reading through the issue I see that the analyzer version is pinned, but you need different versions depending on which SDK you are on, so that pinning has to be updated as a part of the roll, which is presumably manual.

[zanderso]

Right. There are two separate rollers: one that updates packages, and one that updates the Dart SDK (via the engine roll). There is currently no team with bandwidth available to change how that behaves, for example by merging those two rollers into one that operates atomically. So when a Dart SDK roll requires package dependencies to be updated, the rolls fail, requiring manual work to do an atomic roll.

[jakemac53]

Fwiw, another way to solve this would be with one giant mono repo instead of separate repos 🤷‍♂️. I don't have any other suggestions at this time other than not pinning analyzer in flutter/flutter.

[zanderso]

Another possibility is for the analyzer to do experiments with the experimental macros APIs on a branch, so that the version of the analyzer that is published on pub does not take on a dependency on macros.

[matanlurey]

Or just not publish the macros package?

I'm guessing that dependencies: macros: git: would work perfectly fine for testing macros and new releases aren't regularly needed?

Edit; actually I guess that wouldn't work as long as Flutter has a dependency on macros, because the analyzer and CFE are constantly (?) changing.

[jakemac53]

Another possibility is for the analyzer to do experiments with the experimental macros APIs on a branch, so that the version of the analyzer that is published on pub does not take on a dependency on macros.

The macro support is built into the analysis server - which ships as a part of the SDK. So, this would mean all consumers having to build their own SDK to try it out, which isn't generally how we do experiments, and would essentially result in very few people using the experiment. Keeping that branch up to date would also be quite challenging. Using it in flutter would be incredibly challenging.

[matanlurey]

If I understand correctly Jake couldn't we (Flutter) just not pin the macros package as it's effectively pinned to the rolled SDK anyway (hence this churn)?

[jakemac53]

If I understand correctly Jake couldn't we (Flutter) just not pin the macros package as it's effectively pinned to the rolled SDK anyway (hence this churn)?

You need to not pin macros and also analyzer, but yes that is imo the correct fix. The root cause is package pinning.

[zanderso]

Allowing dependencies to roll forward without an explicit commit is not something that we can do, in general. There might be an exception for macros since it is an experiment that is tightly coupled with the Dart SDK, but not for analyzer.

[jakemac53]

Allowing dependencies to roll forward without an explicit commit is not something that we can do, in general.

Why? What if we only allowed specific ranges - we could also fix this by having temporary range constraints like analyzer: >=6.8.0 <=6.9.0 to allow two versions, which would be updated to just 6.9.0 after the roll. But, that still potentially allows for unintended versions between 6.8.0 and 6.9.0 to be selected.

One alternative is to have a non-manual process for rolling a dependency as a part of the engine -> flutter roll but that sounds somewhat complicated.

[jakemac53]

I also wonder whether we can have different rules for dev dependencies versus regular dependencies. These are afaik only dev dependencies.

[zanderso]

When dependencies are unpinned, the tree can turn red on an entirely unrelated commit because a new package version with some issue has been published to pub. This is not hypothetical, and it is difficult to determine that this is what has happened, and how to work around it.

[jakemac53]

When dependencies are unpinned, the tree can turn red on an entirely unrelated commit because a new package version with some issue has been published to pub. This is not hypothetical, and it is difficult to determine that this is what has happened, and how to work around it.

I agree this is not hypothetical, but it is rare (especially if you don't make things like deprecations turn the build red). Essentially all packages in the ecosystem operate using semver, other than flutter.

There is a clear trade-off being made here, it has negative consequences for both upstream and downstream developers, for example:

• (downstream) flutter users can't even get bug fixes, much less new features until their flutter channel updates.
• (upstream) It complicates rolls into flutter (evidenced by this issue).
• (upstream & downstream) Packages pinned by flutter are extremely hampered in what they can do, breaking changes are incredibly challenging for their (even transitive) flutter users. Essentially the same effect seen in this issue is pushed onto flutter users. If a package pinned by flutter has a breaking change, updating to that version is now coupled to their flutter SDK upgrade process and must happen atomically.

[srawlins]

@a-siva and I discussed this morning and I have one suggestion for a playbook for bumping the macros package, while it is still tied in 1-1 cadence with analyzer, and while all package versions are pinned in flutter/flutter:

When a change is coming to the _macros package, which comes with a version bump, someone takes a "coordinator" lead. They coordinate the following steps, maybe in a GitHub issue at flutter/flutter:

1. They direct the analyzer team to release the version of analyzer that was bumped in the same commit as the _macros bump. This should happen within a day-ish of the commit landing in the Dart SDK.
2. They direct the flutter team to run a batch of dependency-version-bumping, unless this already happens in an automated way and with a daily-ish cadence. This will bump the pinned versions of all packages that are unrelated to this _macros bump. Hopefully this is also executed within a day, maybe within two days.

3. They alert the flutter engine sheriff to keep this on their radar. Then when the auto-roller breaks, the sheriff doesn't spend a lot of time figuring out that other people knew it was going to break, and who know what the remedy is. The remedy is a manual roll that includes running the dependency-version-bumping tool, which theoretically will only bump the pinned version of analyzer.

   As I understand it, this is part of why it is so expensive for a _macros bump to break the auto-roller: (1) the time cost in investigating, (2) the time and human-effort cost in doing a manual roll (many roll-breakages can be remedied with a revert to flutter/engine or the Dart SDK, and then re-triggering the auto-roller; this has a time cost but much less human-effort cost), (3) the cost of bumping unrelated packages via the dependency-version-bumping tool, which may not succeed if bumping an unrelated package causes test failures.

(We have many alternative ideas, which all require bandwidth from the infra team. Sucha as: add fancy steps to the auto-roller; add fancy infra to the auto-roller to attach hotfixes to Dart SDK commits; stretch the version pinning to allow for precisely two minor version releases of analyzer (like >=6.8.0 <6.10.0).)

[jakemac53]

@a-siva and I discussed this morning and I have one suggestion for a playbook for bumping the macros package, while it is still tied in 1-1 cadence with analyzer, and while all package versions are pinned in flutter/flutter:

That generally seems like a good plan. Step 1 is already something I have been doing. Step 2 seems like a good way to help make bumping the analyzer dependency less risky (fewer other packages getting bumped). And Step 3 would help lower the cost of investigating the failures.

At least for now, that should minimize the pain compared to the current state.

Longer term we do have plans to fix this, so that analyzer isn't locked to specific versions of package:_macros, but this is still O(months) from being complete. We may never need to do another change though before we have that in place.