Skip to content

the flutter-analyze try bot should also speculatively analyze flutter/plugins #45873

Closed

Description

@devoncarew

Currently the flutter-analyze try bot analyzes flutter/flutter and flutter/engine w/ the latest analysis server from head, in order to give us advance notice that an analysis change will break the sdk -> engine or engine -> flutter roll process.

We do also see breakages for the roll when an analysis change would impact flutter/plugins - we should also include that in our smoke test.

Activity

  1. devoncarew commented on Apr 30, 2021

    @devoncarew
    ContributorAuthor

    @stuartmorgan - flutter-team-archive/plugins#3837 was a first step towards letting us run some portion of analyzing the flutter/plugins repo on the SDK bots.

    I'm not 100% sure how we'll test the plugins repo here as part of the CI (try and drive ./script/incremental_build.sh analyze as per https://dart-review.googlesource.com/c/sdk/+/197420; introduce a flag to https://github.com/flutter/plugins/tree/master/script/tool to let it use a different dart analyze for analysis; manually iiterate over the https://github.com/flutter/plugins/tree/master/packages dirs calling flutter pub get and dart analyze), but in any case I think one issue we'll run into is that fact that the plugins tool seems to have encoded into it knowledge that some packages use different analysis options files.

    Is that an important feature for the plugins repo - that different packages can use different analysis options files? Or just tech debt? Can they all just be changed to use one file?

  2. stuartmorgan-g commented on Apr 30, 2021

    @stuartmorgan-g
    Contributor

    I think if we do this we could turn off the flutter/plugins-in-flutter/flutter tests that we currently run, since I think its only purpose (at least currently) is to catch analyzer change issues that would break flutter/plugins slightly earlier.

    try and drive ./script/incremental_build.sh analyze as per https://dart-review.googlesource.com/c/sdk/+/197420

    This would probably be the most future-proof approach, since any changes we made in how our analysis works would automatically be handled.

    introduce a flag to https://github.com/flutter/plugins/tree/master/script/tool to let it use a different dart analyze for analysis

    Should be easy to add if it'll make this easier.

    manually iiterate over the https://github.com/flutter/plugins/tree/master/packages dirs calling flutter pub get and dart analyze

    It'll be slightly trickier than that, since some plugins have an extra level of nesting (because they've been federated) and some don't (because they haven't yet). But otherwise, this should be just about as good as calling incremental_build.sh since you don't need functionality like only running on changed plugins.

    but in any case I think one issue we'll run into is that fact that the plugins tool seems to have encoded into it knowledge that some packages use different analysis options files.

    I believe all that's encoded into the tool is that we consider it an error for plugins to have their own analysis files unless they are on an exception list, as a way to prevent people from accidentally diverging the plugin analysis styles. That shouldn't matter for this purpose though. They are still using the standard names, include mechanisms, etc.

    Is that an important feature for the plugins repo - that different packages can use different analysis options files? Or just tech debt? Can they all just be changed to use one file?

    It's tech debt. We want to switch the whole repo to using flutter/flutter's set of analysis options, but there are thousands of warnings and errors that need to be resolved, so we added the new options in the standard root options file, put temporary files in all the plugins that point to a differently-named legacy file, and we'll slowly migrate plugins over.

  3. devoncarew commented on Apr 30, 2021

    @devoncarew
    ContributorAuthor

    OK, thanks for the detailed response.

    It sounds like migrating to one analysis options file is desired but not at all a blocker for this work.

    try and drive ./script/incremental_build.sh analyze as per https://dart-review.googlesource.com/c/sdk/+/197420
    This would probably be the most future-proof approach, since any changes we made in how our analysis works would automatically be handled.

    Definitely agree that trying to keep the entry-point as similar as possible to what the CI for flutter/plugins does will limit skew going forward.

    For Dart's CI, we'd want to just consume enough of the plugins repo's CI script to get analysis results (but analyzed using the analysis server build from dart-langs/sdk head). At a higher level, that might look like:

    • git clone flutter/plugins
    • git clone flutter/flutter
    • start the flutter/plugins CI script up; let it know where the flutter/flutter check out was and pass an override to the analysis command to tell it to use a different sdk for dart analyze. Have the script run flutter pub get and then dart analyze per package

    For some prior art, the flutter tool in flutter/flutter has a special --dart-sdk flag to indicate that it should use a different sdk for analysis. We use this (along with the --flutter-repo flag) to smoke test analysis for the flutter repo on the Dart CI system.

       --dart-sdk=<path-to-sdk>                                 The path to the Dart SDK.
    
  4. devoncarew commented on Jun 2, 2021

    @devoncarew
    ContributorAuthor

    this is done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions