to clarify, will flutter create . include a .migrate_config file in the app's root directory?

is there any issue with using the existing .metadata file for the root project directory, and rename .migrate_config as .metadata for the platform directories?

Migrate is adding a few entries to the dictionary, I'm not sure if we need a dedicated file.

the existing .metadata file is being managed via templates, while the migrate_config is being managed by the MigrateConfig class. It is possible to merge the two of course, but we would also have to add it to each platform and handle the case where we need to modify it contrary to what the template would generate.

And yes, flutter create will create the .migrate_config file in the root as well as each platform dir.

the existing .metadata file is being managed via templates, while the migrate_config is being managed by the MigrateConfig class.

That sounds good. The .metadata file was created with these goals in mind:

# This file tracks properties of this Flutter project.
# Used by Flutter tool to assess capabilities and perform upgrades etc.
#
# This file should be version controlled and should not be manually edited.

It sounds like the use cases of .migrate_config should be covered by this file. Any thoughts?

I agree that we shouldnt have too many metadata files. I'll see if we can merge the existing .metadata file with the .migrate_config file. Though the new version of .metadata will probably no longer be a read-only template generated file anymore.

Awesome

Here is a proposed extended .metadata file. This unifies separate config files in each platform dir to one single migrate section in the root .metadata file to remain consistent with the existing metadata tracking system. Since this file will now be owned by the tool, we can easily update the values in it as new platforms are added/edited.

# This file tracks properties of this Flutter project.
# Used by Flutter tool to assess capabilities and perform upgrades etc.
#
# This file should be version controlled.

version:
  revision: {{flutterRevision}}
  channel: {{flutterChannel}}

project_type: app

# Tracks metadata for the flutter migrate command
migrate:
  platforms:
    - platform: android
      createRevision: fj19vkla9vnlka9vni3n808v3nch8cd
      baseRevision: 93kf9v3njfa90vnidfjvn39nvi3vnie
    - platform: ios
      createRevision: fj19vkla9vnlka9vni3n808v3nch8cd
      baseRevision: 93kf9v3njfa90vnidfjvn39nvi3vnie

  # User provided section

  # List of Local paths (relative to this file) that should be
  # ignored by the migrate tool.
  #
  # Files that are not part of the templates will be ignored by default.
  unmanagedFiles:
    - lib/file1/etc.dart
    - android/my_file.java

LGTM

the only minor comment is to maybe use a noun for consistency, so migrate -> migration

an enum may be safer here

I might go through the whole tools repo and unify references to the different platforms into a single enum. We currently have a few different ways of id-ing platforms already.

I'd prefer an enum; if not, it doesn't seem like the String? platform arg should be nullable?

why would a consumer of this API avoid passing the platform?

Made non-nullable

nit: projectDirectory docs

this can be avoided if an enum is used instead. I believe Dart will enforce all cases in the enum to be handled in the switch statement. If that's not true, I've been doing too much golang on the side then :)

+1

this seems like a utility that belongs in FlutterProject. A method that provides all the supported platforms.

Moved.

what about if result.exitCode != 0 ?

I'll be moving this into a utility class in the next PR where I handle all of this. I'd prefer to not replicate all the error handling here temporarily as it will be removed very soon.

projectPath sounds like the app's project path, but this is the directory where Flutter is installed

did it mean .metadata?

Nope, this doc is correct. The fallback here is for when the migration config doesn't know the revision, so we try to parse it out of .metadata and other means.

can this throw?

cc @jmagman

We have this, but it needs to be run on a Mac with Xcode installed.

I'm guessing this needs to work on all platforms, like if you ran this on Windows you would still want the ios directory migrated.

A user could rename AppDelegate.swift but they likely wouldn't.

Yes. All platform compatibility is important. I can try to include a few more fallback checks if that is something that could help with robustness.

does it need to be in the app's directory? could it use the temp directory instead?

The intention of this working dir is to be easily accessible and obvious. Putting it in a temp or hidden away folder makes it hard for developers to enter and work with the files inside.

sgtm

What about .migration? Some context: https://unix.stackexchange.com/a/21867

I specifically don't want this directory hidden as it is meant to be accessed by the developer as part of the migration process. Also, just calling it migrate may not be sufficiently clear that it is meant to be modified/edited/viewed as part of migration imo.

does the tool also support Flutter modules?

No. Not yet.

But if the future if/when we do, it would be nice to have this already in the .gitignore to allow for better backwards compatibility.

should this support the glob pattern? for example: lib/**/*.dart seems reasonable to me.

Is there a lib we typically use to support this? It seems we support just simple directories and file URIs in pubspec.yaml

Yup: https://github.com/dart-lang/glob

nit: EOF

oh TIL

is this file test data or code that runs as part of the test?

This runs as part of the test to set up the environment/sample app.

I was under the impression that this was test data since it's under a directory named test_data

Could it be moved to a different directory?

This generates/gets the test fixtures, which is consistent with what the other files in this directory do.

ok. This seems like a different pattern. Usually test data doesn't contain code that is executed to generate test data or fixtures.

could this be set in the constructor?

Not in the way the current project class is designed. The directory is only known once we call setUpIn(), which must be called first before any of the other getters/methods are valid. We track the appPath here because some of the getters are dynamically filled with the CIPD downloaded contents.

Can you pull these into locals, it's used right above here:

androidPlatform: templateContext['android'] as bool ?? false

and to update the gradle properties.

When would this need to be specified?

This is used in the other half of the feature where create is called programmatically to create an app to diff against. We want to specify this so that the metadata around the revisions is using the app's metadata, not creating brand new metadata as a regular create run would do.

If this only supposed to be called re-entrantly, let's make it hide: true and specify in the help text this is not supposed to be called by users.

Turns out test/general.shard/args_test.dart: Help for command line arguments is consistently styled and complete enforces that options are not permanently hidden. The description advising not to use the flag should be sufficient.

hide: !verboseHelp seems correct.

Instead of "revision" maybe more specific "Flutter SDK git commit hash" or something like that @christopherfujino

Use Object? in null safe files, not dynamic.

+1

Use flutterVersion.frameworkRevision?

Let me validate if flutterVersion.frameworkRevision always produces the form that I need.

Changed to use FlutterVersion!

Refactor your new code to not use the global context variables, these should all pass with testWithoutContext.

FlutterProject in project.dart seems to be using globals.projectFactory and I'd prefer to leave this refactoring for a different PR. I've made the tests that can be context free context free.

Gotcha, yeah do what you can, and avoid it where possible in new code. There's still a bunch of tricky ones still hanging out.

I don't think we should be downloading cipd packages in a tools integration test. It should probably be its own test shard, with the cipd package dependency set up in the ci.yaml? @christopherfujino

yeah, if a test requires a cipd dependency, it should be specified in the ci.yaml: https://github.com/flutter/flutter/blob/master/.ci.yaml#L174 and then the actual downloading specified in a recipe module

The way this has to be tested requires frequent setup and teardown of a sample app, and the recipes+deps system is not designed to support this case efficiently. I would need to frequently re-get the cipd deps and the recipes gets deps once at the beginning.

I could host the vanilla sample apps somewhere else and avoid CIPD. The choice to use CIPD is to avoid having to check in entire vanilla apps as test fixtures.

We already check in quite a few test apps, I say we just check it in. This way also if the app needs to be fixed, it's easy to find.

I checked in a test fixture app for multidex which is a much smaller mini-app, and even that needed to get hosted files such as icon images and stuff.

The test apps required for this must be full apps, which can get quite large. Also, due to the nature of this feature, I intend to eventually branch out to test between many full apps created by different historic flutter versions. Checking in multiple full flutter apps is not sustainable, which is why I turned to hosting on cipd. I could shift the host to github if that makes more sense. I'm not sure if github will appreciate the test infra traffic though, and git makes it much harder to cherrypick individual app fixtures by version and would likely need to be in separate repos to be efficient.

Sorry, I missed this message. I'll file a tracking issue for discussion on the approach to this.