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

[srawlins]

See flutter/flutter#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 flutter/flutter#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 dart-lang/pub#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]

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