Use FlutterProject to locate files by sigurdm · Pull Request #18913 · flutter/flutter

sigurdm · 2018-06-28T13:01:14Z

A bit of cleanup of flutter_tools' handling of project directories and manifests.

mravn-google

LGTM

mravn-google · 2018-06-28T13:08:04Z

packages/flutter_tools/lib/src/commands/create.dart

    copyDirectorySync(
      cache.getArtifactDirectory('gradle_wrapper'),
-      fs.directory(fs.path.join(projectDir, 'android')),
+      project.directory.childDirectory('android'),


project.android?

mravn-google · 2018-06-28T13:11:13Z

packages/flutter_tools/lib/src/ios/cocoapods.dart

+    final File file = iosProject.xcodeConfigFor(mode);
    if (file.existsSync()) {
-      final String content = file.readAsStringSync();
+      final String content = iosProject.xcodeConfigFor(mode).readAsStringSync();


Could we not retain the use of file here?

mravn-google · 2018-06-28T13:17:59Z

packages/flutter_tools/lib/src/ios/cocoapods.dart

+
+    final File podfileFile = iosProject.podfile;
+    final File podfileLockFile = iosProject.podfileLock;
+    final File manifestLockFile =iosProject.podManifestLock;


Add space after =

mravn-google · 2018-06-28T13:20:33Z

packages/flutter_tools/lib/src/ios/xcodeproj.dart

-    if (!Cache.instance.fileOlderThanToolsStamp(propertiesFile)) {
+Future<void> generateXcodeProperties({FlutterProject project}) async {
+  if ((await project.manifest).isModule ||
+      project.directory.childDirectory('ios').existsSync()) {


project.ios.directory.existsSync()?

mravn-google · 2018-06-28T13:25:36Z

packages/flutter_tools/lib/src/plugins.dart

+    'main',
+    'java',
+  );
+  final String registryPath = fs.path.join(javaSourcePath,


[nit] Move javaSourcePath, to the next line.

mravn-google · 2018-06-28T13:29:54Z

packages/flutter_tools/lib/src/project.dart

+  Future<File> _androidLocalPropertiesFile;
+
+  Future<File> get generatedXcodePropertiesFile {
+    return _generatedXcodeProperties ??= manifest.then((FlutterManifest manifest) {


Add <File> after then

mravn-google · 2018-06-28T13:30:41Z

packages/flutter_tools/lib/src/project.dart

  IosModuleProject get iosModule => new IosModuleProject(directory.childDirectory('.ios'));

+  Future<File> get androidLocalPropertiesFile {
+    return _androidLocalPropertiesFile ??= manifest.then((FlutterManifest manifest) {


Add <File> after then.

mravn-google · 2018-06-28T13:33:09Z

packages/flutter_tools/lib/src/project.dart

+  }
+  File _flutterPluginsDotFile;
+
+  Future<Directory> get androidPluginRegistrantDirectory async {


This Directory is not where we should place the registrant. It is the Android sub project in which we should place the registrant. This discrepancy might be confusing.

Maybe androidPluginHost or similar?

Better!
Done

mravn-google · 2018-06-28T13:33:34Z

packages/flutter_tools/lib/src/project.dart

+  }
+  Future<Directory> _androidPluginRegistrantDirectory;
+
+  Future<Directory> get iosPluginRegistrantDirectory async {


Similarly here.

mravn-google · 2018-06-28T13:38:53Z

packages/flutter_tools/test/project_test.dart

        new FlutterProject(directory).directory.absolute.path,
-        directory.absolute.path,
-      );
+        directory.absolute.path,);


Please put the ); back to whence it came.

ok, ok... will do );

sigurdm

Thanks for the review!

sigurdm · 2018-07-03T12:04:43Z

packages/flutter_tools/lib/src/commands/create.dart

    copyDirectorySync(
      cache.getArtifactDirectory('gradle_wrapper'),
-      fs.directory(fs.path.join(projectDir, 'android')),
+      project.directory.childDirectory('android'),


sigurdm · 2018-07-03T12:07:57Z

packages/flutter_tools/lib/src/ios/cocoapods.dart

+    final File file = iosProject.xcodeConfigFor(mode);
    if (file.existsSync()) {
-      final String content = file.readAsStringSync();
+      final String content = iosProject.xcodeConfigFor(mode).readAsStringSync();


sigurdm · 2018-07-03T12:08:17Z

packages/flutter_tools/lib/src/ios/cocoapods.dart

+
+    final File podfileFile = iosProject.podfile;
+    final File podfileLockFile = iosProject.podfileLock;
+    final File manifestLockFile =iosProject.podManifestLock;


sigurdm · 2018-07-03T12:09:49Z

packages/flutter_tools/lib/src/ios/xcodeproj.dart

-    if (!Cache.instance.fileOlderThanToolsStamp(propertiesFile)) {
+Future<void> generateXcodeProperties({FlutterProject project}) async {
+  if ((await project.manifest).isModule ||
+      project.directory.childDirectory('ios').existsSync()) {


sigurdm · 2018-07-03T12:10:37Z

packages/flutter_tools/lib/src/plugins.dart

+    'main',
+    'java',
+  );
+  final String registryPath = fs.path.join(javaSourcePath,


sigurdm · 2018-07-03T12:12:30Z

packages/flutter_tools/lib/src/project.dart

+  Future<File> _androidLocalPropertiesFile;
+
+  Future<File> get generatedXcodePropertiesFile {
+    return _generatedXcodeProperties ??= manifest.then((FlutterManifest manifest) {


sigurdm · 2018-07-03T12:13:57Z

packages/flutter_tools/lib/src/project.dart

+  }
+  Future<File> _generatedXcodeProperties;
+
+  File get flutterPluginsDotFile {


sigurdm · 2018-07-03T12:14:41Z

packages/flutter_tools/lib/src/project.dart

+  }
+  File _flutterPluginsDotFile;
+
+  Future<Directory> get androidPluginRegistrantDirectory async {


Better!
Done

sigurdm · 2018-07-03T12:15:57Z

packages/flutter_tools/lib/src/project.dart

+  }
+  Future<Directory> _androidPluginRegistrantDirectory;
+
+  Future<Directory> get iosPluginRegistrantDirectory async {


sigurdm · 2018-07-03T12:16:53Z

packages/flutter_tools/test/project_test.dart

        new FlutterProject(directory).directory.absolute.path,
-        directory.absolute.path,
-      );
+        directory.absolute.path,);


ok, ok... will do );

sigurdm · 2018-07-06T13:21:26Z

@szakarias Could you take a look. Especially the latest commit?

# Conflicts: # packages/flutter_tools/lib/src/plugins.dart

szakarias

LGTM

szakarias · 2018-07-09T12:17:15Z

packages/flutter_tools/lib/src/application_package.dart

 class AndroidApk extends ApplicationPackage {
  /// Path to the actual apk file.
-  final String apkPath;
+  final File apk;


szakarias · 2018-07-09T12:19:44Z

packages/flutter_tools/lib/src/android/gradle.dart

-const String gradleManifestPath = 'android/app/src/main/AndroidManifest.xml';
-const String gradleAppOutV1 = 'android/app/build/outputs/apk/app-debug.apk';
-const String gradleAppOutDirV1 = 'android/app/build/outputs/apk';
+


szakarias · 2018-07-09T12:40:18Z

packages/flutter_tools/lib/src/application_package.dart

      // command will grab a new AndroidApk after building, to get the updated
      // IDs.
-      manifestPath = gradleManifestPath;
+      manifest = androidProject.gradleManifestFile;


set manifest outside the if and place it together with the exists check.

szakarias · 2018-07-09T12:40:47Z

packages/flutter_tools/lib/src/application_package.dart

      return null;

-    final String manifestString = fs.file(manifestPath).readAsStringSync();
+    final String manifestString = fs.file(manifest).readAsStringSync();


manifest is now a file

szakarias · 2018-07-09T12:43:21Z

packages/flutter_tools/lib/src/application_package.dart

  /// Creates a new IOSApp from an existing app bundle or IPA.
-  factory IOSApp.fromPrebuiltApp(String applicationBinary) {
-    final FileSystemEntityType entityType = fs.typeSync(applicationBinary);
+  factory IOSApp.fromPrebuiltApp(File apk) {


Keep applicationBinary as name?

szakarias · 2018-07-09T12:45:00Z

packages/flutter_tools/lib/src/application_package.dart


 Future<ApplicationPackage> getApplicationPackageForPlatform(TargetPlatform platform, {
-  String applicationBinary
+  File apk


Keep applicationBinary ?

szakarias · 2018-07-09T12:48:03Z

packages/flutter_tools/lib/src/resident_runner.dart

    package = await getApplicationPackageForPlatform(
      targetPlatform,
-      applicationBinary: hotRunner.applicationBinary
+      apk: hotRunner.applicationBinary


Keep applicationBinary ?

# Conflicts: # packages/flutter_tools/test/android/gradle_test.dart # packages/flutter_tools/test/ios/xcodeproj_test.dart

This reverts commit 57d78cc.

* flutter_master: (810 commits) Revert engine roll to 316b026 roll engine to 316b026 (flutter#19419) Revert "enable lint unnecessary_const (flutter#19342)" (flutter#19423) enable lint unnecessary_const (flutter#19342) Chevrons in month picker are semi-transparent when the month is scrolled (flutter#19363) Revert "Use FlutterProject to locate files (flutter#18913)" (flutter#19409) Extra debug information in run_release_test (flutter#19405) Fix typo (flutter#19402) Use FlutterProject to locate files (flutter#18913) Revert "roll engine to 9af920e (flutter#19365)" (flutter#19376) roll engine to 9af920e (flutter#19365) increase cache size if image is loaded that is larger than max size (flutter#19352) Add Bash and Zsh command-line completion for flutter (flutter#19243) Support keyboardAppearance field for iOS (flutter#19244) Add option to silence driver extension errors (flutter#19247) Add HeroController to CupertinoApp (flutter#19326) have text finder convert Text.rich to plain text for comparison (flutter#19270) Correct contentPadding type in InputDecoration.debugFillProperties (flutter#19318) Revert "Revert "Use runTests in fuchsia tester. (flutter#19178)" (flutter#19321)" (flutter#19327) Roll engine to c5a63d (flutter#19288) ...

…lutter#19409)" This reverts commit 6a8f904.

…19409) This reverts commit 57d78cc.

…" (#19456) With a fix of a path being printed relative instead of absolute.

Use the FlutterProject abstraction

abbcfed

sigurdm requested a review from mravn-google June 28, 2018 13:01

googlebot added the cla: yes label Jun 28, 2018

mravn-google approved these changes Jun 28, 2018

View reviewed changes

Address review

790eeff

sigurdm commented Jul 3, 2018

View reviewed changes

sigurdm added 5 commits July 3, 2018 21:17

Check for android sdk availability

c5cfad5

Check for android sdk availability2

c381d53

Remove unused import

284c179

Further refactorings

9b77421

Merge branch 'master' into use_project_abstraction

b936998

sigurdm requested a review from szakarias July 6, 2018 13:20

Merge branch 'master' into use_project_abstraction

cb16973

# Conflicts: # packages/flutter_tools/lib/src/plugins.dart

szakarias approved these changes Jul 9, 2018

View reviewed changes

sigurdm added 6 commits July 9, 2018 17:21

Normalize path for windows support

fa51678

Address review

6903848

Use .path when printing

2b0bc29

Merge branch 'master' into use_project_abstraction

603991f

# Conflicts: # packages/flutter_tools/test/android/gradle_test.dart # packages/flutter_tools/test/ios/xcodeproj_test.dart

Fix after merge

78945ae

Await updateGeneratedXcodeProperties in test

74d9b5e

sigurdm merged commit 57d78cc into flutter:master Jul 16, 2018

sigurdm added a commit to sigurdm/flutter that referenced this pull request Jul 16, 2018

Revert "Use FlutterProject to locate files (flutter#18913)"

b4b6825

This reverts commit 57d78cc.

sigurdm added a commit that referenced this pull request Jul 16, 2018

Revert "Use FlutterProject to locate files (#18913)" (#19409)

6a8f904

This reverts commit 57d78cc.

sigurdm added a commit to sigurdm/flutter that referenced this pull request Jul 17, 2018

Revert "Revert "Use FlutterProject to locate files (flutter#18913)" (f…

caa52c5

…lutter#19409)" This reverts commit 6a8f904.

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request Jul 19, 2018

Use FlutterProject to locate files (flutter#18913)

ad532f7

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request Jul 19, 2018

Revert "Use FlutterProject to locate files (flutter#18913)" (flutter#…

b281808

…19409) This reverts commit 57d78cc.

sigurdm added a commit that referenced this pull request Jul 20, 2018

Revert "Revert "Use FlutterProject to locate files (#18913)" (#19409)…

2d3a5c7

…" (#19456) With a fix of a path being printed relative instead of absolute.

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021

Conversation

sigurdm commented Jun 28, 2018

Uh oh!

mravn-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigurdm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigurdm commented Jul 6, 2018

Uh oh!

szakarias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!