Skip to content

[tool] Check for any Flutter SDK dependency#11875

Merged
auto-submit[bot] merged 2 commits into
flutter:mainfrom
stuartmorgan-g:tool-require-flutter-for-any-sdk-dep
Jun 9, 2026
Merged

[tool] Check for any Flutter SDK dependency#11875
auto-submit[bot] merged 2 commits into
flutter:mainfrom
stuartmorgan-g:tool-require-flutter-for-any-sdk-dep

Conversation

@stuartmorgan-g

Copy link
Copy Markdown
Collaborator

When determining whether a package requires flutter, look for any sdk: flutter dependency rather than just for the flutter dependency in particular. This will ensure that includes, for example, packages with a flutter_test dev dependency, but no flutter dependency.

When determining whether a package requires `flutter`, look for any
`sdk: flutter` dependency rather than just for the `flutter` dependency
in particular. This will ensure that includes, for example, packages
with a `flutter_test` dev dependency, but no `flutter` dependency.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the requiresFlutter method in RepositoryPackage to identify Flutter SDK dependencies within both dependencies and devDependencies using a new helper method. A corresponding test has been added to verify this behavior. The review feedback points out that mutating the parsed Pubspec object and writing its toString() representation to disk in the test is fragile and will corrupt the file, suggesting instead to write the YAML string directly to the file.

Comment on lines +230 to +238
test('returns true for a dev dependency on other Flutter SDK packages', () async {
final RepositoryPackage package = createFakePackage('a_package', packagesDir);
final File pubspecFile = package.pubspecFile;
final Pubspec pubspec = package.parsePubspec();
pubspec.devDependencies['flutter_test'] = SdkDependency('flutter');
pubspecFile.writeAsStringSync(pubspec.toString());

expect(package.requiresFlutter(), true);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Mutating the parsed Pubspec object in memory and writing pubspec.toString() to the file is fragile and corrupts the file on disk, as Pubspec from pubspec_parse does not serialize back to valid YAML via toString(). Instead, write the valid YAML string directly to the pubspecFile before calling any methods on the package.

    test('returns true for a dev dependency on other Flutter SDK packages', () async {
      final RepositoryPackage package = createFakePackage('a_package', packagesDir);
      package.pubspecFile.writeAsStringSync('''
name: a_package
dev_dependencies:
  flutter_test:
    sdk: flutter
''');

      expect(package.requiresFlutter(), true);
    });

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

All the existing tests of this method use this pattern, and they've all been fine. Reworking all of these tests is out of scope here.

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2026
@auto-submit

auto-submit Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/packages/11875, because - The status or check suite Linux_android_legacy android_platform_tests_legacy_api_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@jakemac53 jakemac53 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but see the test comments from gemini

@github-actions github-actions Bot added p: vector_graphics triage-engine Should be looked at in engine triage and removed CICD Run CI/CD labels Jun 9, 2026
@stuartmorgan-g stuartmorgan-g added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Jun 9, 2026
@auto-submit auto-submit Bot merged commit 1ca9c97 into flutter:main Jun 9, 2026
87 checks passed
@stuartmorgan-g stuartmorgan-g deleted the tool-require-flutter-for-any-sdk-dep branch June 9, 2026 14:30
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 CICD Run CI/CD p: vector_graphics triage-engine Should be looked at in engine triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants