[flutter_tools] analyze --suggestions --machine command

[GaryQian]

Adds flutter analyze --suggestions --machine command, which outputs a simple json map of key properties computed by the Flutter tool.

Example output:

{
  "FlutterProject.directory": "/Users/garyq/projects/hello_world",
  "FlutterProject.metadataFile": "/Users/garyq/projects/hello_world/.metadata",
  "FlutterProject.android.exists": true,
  "FlutterProject.ios.exists": true,
  "FlutterProject.web.exists": true,
  "FlutterProject.macos.exists": true,
  "FlutterProject.linux.exists": true,
  "FlutterProject.windows.exists": true,
  "FlutterProject.fuchsia.exists": false,
  "FlutterProject.android.isKotlin": true,
  "FlutterProject.ios.isSwift": true,
  "FlutterProject.isModule": false,
  "FlutterProject.isPlugin": false,
  "FlutterProject.manifest.appname": "hello_world",
  "FlutterVersion.frameworkRevision": "",
  "Platform.operatingSystem": "macos",
  "Platform.isAndroid": false,
  "Platform.isIOS": false,
  "Platform.isWindows": false,
  "Platform.isMacOS": true,
  "Platform.isFuchsia": false,
  "Platform.pathSeparator": "/",
  "Cache.flutterRoot": "/Users/garyq/flutter"
}

The values reported can be expanded as needed in the future. These values are primarily the values used by flutter/packages#2441

[christopherfujino]

This reminds me, you actually need to remove this code that depends on depot_tools from the integration tests: #100254. We can't add more tests relying on this code.

[GaryQian]

Ahh, ok. I'll remove it here and do it another way for this PR. I'll also send a PR later removing the depot_tools system completely once I get around to ripping out the already landed migrate stuff.

[christopherfujino]

Ahh, ok. I'll remove it here and do it another way for this PR. I'll also send a PR later removing the depot_tools system completely once I get around to ripping out the already landed migrate stuff.

Sounds good. @Jasguerrero was mentioning that you should be able to leverage the work he did for doing diagnostics on a flutter project; by adding a --machine flag both migrate and end users could leverage the same code.

[Jasguerrero]

Sure the command in question is flutter analyze --suggestions it works with validators sort of like the doctor ones.

[GaryQian]

I took a look at the flutter analyze --suggestions and it seems to be a bit heavyweight for what this PR is trying to do. It accomplishes similar things, but the intention for this command is to output machine readable data so that other packages can leverage a lot of the things flutter_tools computes without replicating it. The suggestions seem to be more aimed at human readable output and each validator does a lot more than just output, which is all that is needed here.

[Jasguerrero]

@GaryQian I agree currently --suggestions do more than what is required, the proposal is more to add a new flag alongside --suggestions (--machine) in which we could change the output of each validator (Basically the output of each validator is key: value just what you need) to a json format

[GaryQian]

@Jasguerrero Ahh, I see. My concern is that --suggestions doesn't seem to be the logical name for a dump of internal tool properties. It sounds more like a command that recommends changes. That being said, I'm not sure a data dump belongs in the analyze command at all.

I would agree that this doesn't necessarily need its own command, it could perhaps live under a --dump/--dump-properties flag or something, but I'm also not sure what existing command would be most suited to own this flag.

[christopherfujino]

@Jasguerrero Ahh, I see. My concern is that --suggestions doesn't seem to be the logical name for a dump of internal tool properties. It sounds more like a command that recommends changes. That being said, I'm not sure a data dump belongs in the analyze command at all.

Isn't this something the migrate tool will call under the hood, unseen to the user? If so, what does the flag name matter? In other words, whether --suggestions is the most logical name seems an unrelated issue, unless I'm missing something and this new way of accessing the data is intended to be user facing.

[Jasguerrero]

I see, sure the name --suggestions could be misleading but some time ago in a dash forum most of the team suggested this behavior to be in the analyze command as a flag maybe we could change the name of the flag to something like --info to align more to what we are doing (right now the command do not give any suggestions just information)

[GaryQian]

If we want to hide a certain feature, I dont think the right way to go about it is putting it under unclear flags, rather, we should just directly hide the flag's existence (which I think we can already do that via some parameter) in documentation and help. I guess there isn't currently a better place than analyze to put this capability, so that seems reasonable to me especially if there is some consensus in previous discussions.

[Jasguerrero]

My suggestion was more to use what we have under --suggestions and add a new ProjectValidator to get all of this info, then on https://github.com/flutter/flutter/blob/3.3/packages/flutter_tools/lib/src/commands/validate_project.dart#L47 read your new flag to have a json dump

[GaryQian]

Sorry, I'm not familiar with the validators system, but I don't see how adding this within the validators system improves the organization of this. Specifically, one thing is that the validator prints everything with formatting in a box, which does not help with the intention of this command being purely machine-ingested.

This command doesn't "validate" anything, if the values are invalid/wrong, this command should print it out anyways. It is meant to poll the state of the tool. Let me know if you want to chat real quick to be clear on this!

*edit Upon further inspection, let me try to move my code into a ProjectValidator and change ValidateProject.run() to machine-dump

[GaryQian]

Ok, this should be ready for review. I've integrated it with the suggestions and ProjectValidators systems.

[christopherfujino]

Why have a different code path for the machine dump, rather than standardizing on a single set of validators used in both --machine and non-machine modes, and simply print them differently?

[GaryQian]

I'm not certain how we are choosing which properties to include in the non-machine --suggestions, but it seems that the machine version will expose much deeper and less-human-relevant properties. Also, the non-machine validators encode the name and value as human readable while this command prefers a direct blah.foo.bar structure for the name and a json compatible string as value.

Converting between the human readable version and the machine version seems tedious and unnecessary, especially since I'm not doing any "work" here, just dumping values.

[christopherfujino]

can we have --machine mode be a superset of the non-machine mode, then? I'd like to avoid having essentially two different code paths to maintain.

[GaryQian]

The cleanest way to do this is still the separate code path. The way the validator API works is that it stores name - value pairings as two strings in each ProjectValidatorResult that it produces.

For example, in the existing --suggestions validator, we store the app's pubspec name as ProjectValidatorResult(name: 'App name', value: 'my_app', status: StatusProjectValidator.success). There is no way of converting this to the desired machine format of ProjectValidatorResult(name: 'FlutterProject.manifest.appName', value: '"my_app"', status: StatusProjectValidator.info) without just hardcoding the different name and value properties. This same issue is repeated for every single existing validation. Any benefit of overlapping validations is made moot by having to write a bunch of if statements on what string to store creating much more complicated, harder to maintain, and longer code.

The other concern is that the existing validator is simply not trying to accomplish the same task as the new validator I'm adding. It is trying to check if different properties are good or not, while the dump validator is just trying to expose the values regardless of the correctness. Lumping them together seems wrong.

[christopherfujino]

as our current testing on the toilet suggests, can you parse the stdout with json.decode and then assert on the keys and values?

[GaryQian]

Done.

[Jasguerrero]

What we can do is having 2 types of ProjectValidator, make a new abstract class that inherits from ProjectValidator called MachineProjectValidator move all of the validators under allProjectValidators and on validate_project.dart run and output only the ones needed

[christopherfujino]

LGTM

[Jasguerrero]

LGTM

[auto-submit]

auto label is removed for flutter/flutter, pr: 112217, due to - The status or check suite Windows tool_integration_tests_1_6 has failed. Please fix the issues identified (or deflake) before re-applying this label.