Merge pull request #151 from devmil/feature/remove_directory_tree_copy

remove directory tree copying logic
bmw-tech · Sep 11, 2023 · b64a5e7 · b64a5e7
2 parents 48f71a9 + 275eba1
commit b64a5e7
Show file tree

Hide file tree

Showing 19 changed files with 128 additions and 192 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,8 @@
 # Changelog
 
+## Version 0.16.0
+- simplify preparation of packages by replacing the directory structure copy operation with carrying over the package config (that contains the absolute paths to the original path references). Thanks to @mosuem and @jonasfj for this idea! This makes the `--include-path-dependencies` option obsolete. This option has no effect anymore and will be removed in a future version.
+
 ## Version 0.15.0
 - Remove effect of `--dependency-check-mode` and add a deprecation warning (for why refer to https://github.com/bmw-tech/dart_apitool/issues/144)
 

diff --git a/README.md b/README.md
@@ -49,11 +49,7 @@ Usage: dart-apitool extract [arguments]
                                            (e.g. /path/to/package)
                                          - any package from pub
                                            (e.g. pub://package_name/version)
--p, --[no-]include-path-dependencies     Scans package for path dependencies and makes sure to copy all path dependencies for evaluation.
-                                         Warning: This option might cause copy to lift the copying scope outside the current working directory,
-                                         depending on paths defined by path dependencies.
-                                         Affects only local references.
-                                         (defaults to off)
+-p, --[no-]include-path-dependencies     OBSOLETE: Has no effect anymore.
     --output                             Output file for the extracted Package API.
                                          If not specified the extracted API will be printed to the console.
     --no-analyze-platform-constraints    Disables analysis of platform constraints.
@@ -79,11 +75,7 @@ Usage: dart-apitool diff [arguments]
                                            (e.g. /path/to/package)
                                          - any package from pub
                                            (e.g. pub://package_name/version)
--p, --[no-]include-path-dependencies     Scans package for path dependencies and makes sure to copy all path dependencies for evaluation.
-                                         Warning: This option might cause copy to lift the copying scope outside the current working directory,
-                                         depending on paths defined by path dependencies.
-                                         Affects only local references.
-                                         (defaults to off)
+-p, --[no-]include-path-dependencies     OBSOLETE: Has no effect anymore.
     --version-check-mode                 Defines the mode the versions of the packages shall be compared.
                                          This affects the exit code of this program.
                                          [none, fully (default), onlyBreakingChanges]
@@ -95,6 +87,8 @@ Usage: dart-apitool diff [arguments]
                                          (in your CI) that the version - once ready - matches semver.
                                          (defaults to on)
     --no-analyze-platform-constraints    Disables analysis of platform constraints.
+    --dependency-check-mode              DEPRECATED - this option as no effect any more
+                                         [none, allowAdding (default), strict]
     --[no-]remove-example                Removes examples from the package to analyze.
                                          (defaults to on)
     --[no-]ignore-requiredness           Whether to ignore the required aspect of interfaces (yielding less strict version bump requirements)

diff --git a/analysis_options.yaml b/analysis_options.yaml
@@ -8,3 +8,7 @@ analyzer:
     - '**.freezed.dart'
     - scripts/**
     - test/test_packages/**
+
+linter:
+  rules:
+    - unawaited_futures
diff --git a/lib/src/cli/commands/command_mixin.dart b/lib/src/cli/commands/command_mixin.dart
@@ -1,10 +1,8 @@
 import 'dart:io';
-import 'dart:math';
 
 import 'package:dart_apitool/api_tool.dart';
 import 'package:dart_apitool/src/cli/source_item.dart';
 import 'package:path/path.dart' as p;
-import 'package:pubspec_parse/pubspec_parse.dart';
 
 import '../package_ref.dart';
 import '../prepared_package_ref.dart';
@@ -20,63 +18,43 @@ Package reference can be one of:
 ''';
 
   final includePathDependenciesExplanation = '''
-Scans package for path dependencies and makes sure to copy all path dependencies for evaluation.
-Warning: This option might cause copy to lift the copying scope outside the current working directory,
-depending on paths defined by path dependencies.
-Affects only local references.
-(defaults to off)
+OBSOLETE: Has no effect anymore.
  ''';
 
   /// prepares given [ref]. Depending on the type of ref this can include
   /// - copying the package to a temporary directory
   /// - running pub get
   /// If you use [analyze] with this result then it will take care to clean up
   /// everything (e.g. removing temp directory)
-  Future<PreparedPackageRef> prepare(
-    PackageRef ref, {
-    bool shouldCheckPathDependencies = false,
-  }) async {
+  Future<PreparedPackageRef> prepare(PackageRef ref) async {
     final stdoutSession = StdoutSession();
     List<SourceItem> sources = [];
     String? packageRelativePath;
     if (ref.isDirectoryPath) {
-      stdoutSession.writeln('Preparing ${ref.ref}');
+      await stdoutSession.writeln('Preparing ${ref.ref}');
       String sourceDir = ref.ref;
       if (sourceDir.endsWith(p.separator)) {
         sourceDir =
             sourceDir.substring(0, sourceDir.length - p.separator.length);
       }
-      if (shouldCheckPathDependencies) {
-        String commonPath = sourceDir;
-        final pathDependencies = await _listPathDependencies(sourceDir);
-        if (pathDependencies.isNotEmpty) {
-          stdoutSession.writeln(
-              'Found path dependencies: [\n${pathDependencies.reduce((v, e) => '$v\n$e')}\n]');
-          commonPath = _commonRootPathForPackagesPaths(
-              paths: pathDependencies.toList() + [sourceDir]);
-          stdoutSession.writeln('Common path: $commonPath');
-          for (final path in pathDependencies) {
-            sources.add(SourceItem.forCommonPath(
-                sourceDir: path, commonPath: commonPath));
-          }
-        }
 
-        final sourceItem = SourceItem.forCommonPath(
-            sourceDir: sourceDir, commonPath: commonPath);
-        packageRelativePath = sourceItem.relativeDestinationDir;
-        sources.add(sourceItem);
-      } else {
-        sources.add(SourceItem(sourceDir: sourceDir));
-      }
+      sources.add(SourceItem(
+        sourceDir: sourceDir,
+        isInCache: false,
+      ));
     } else if (ref.isPubRef) {
-      stdoutSession.writeln('Preparing ${ref.pubPackage!}:${ref.pubVersion!}');
-      stdoutSession.writeln('Downloading');
+      await stdoutSession
+          .writeln('Preparing ${ref.pubPackage!}:${ref.pubVersion!}');
+      await stdoutSession.writeln('Downloading');
       String sourceDir = await PubInteraction.installPackageToCache(
         ref.pubPackage!,
         ref.pubVersion!,
         stdoutSession: stdoutSession,
       );
-      sources.add(SourceItem(sourceDir: sourceDir));
+      sources.add(SourceItem(
+        sourceDir: sourceDir,
+        isInCache: true,
+      ));
     } else {
       throw ArgumentError('Unknown package ref: ${ref.ref}');
     }
@@ -86,9 +64,22 @@ Affects only local references.
         sources.any((s) => p.isWithin(s.sourceDir, sToRemove.sourceDir)));
     final tempDir = await Directory.systemTemp.createTemp();
     await Future.forEach<SourceItem>(sources, (sourceItem) async {
-      stdoutSession.writeln('Copying sources from ${sourceItem.sourceDir}');
+      await stdoutSession
+          .writeln('Copying sources from ${sourceItem.sourceDir}');
       await _copyPath(sourceItem.sourceDir,
           sourceItem.destinationPath(forPrefix: tempDir.path));
+      if (!sourceItem.isInCache) {
+        await stdoutSession.writeln(
+            'Preparing package dependencies for local package ${sourceItem.sourceDir}');
+        await PubInteraction.runPubGet(sourceItem.sourceDir,
+            stdoutSession: stdoutSession);
+        final sourcePackageConfig =
+            File(_getPackageConfigPathForPackage(sourceItem.sourceDir));
+        final targetPackageConfig =
+            File(_getPackageConfigPathForPackage(tempDir.path))
+              ..createSync(recursive: true);
+        await sourcePackageConfig.copy(targetPackageConfig.path);
+      }
     });
     return PreparedPackageRef(
         packageRef: ref,
@@ -130,10 +121,18 @@ Affects only local references.
     if (doRemoveExample && await Directory(exampleDirPath).exists()) {
       await Directory(exampleDirPath).delete(recursive: true);
     }
-    stdoutSession.writeln('Running pub get');
-    await PubInteraction.runPubGet(packagePath, stdoutSession: stdoutSession);
 
-    stdoutSession.writeln('Analyzing $path');
+    // Check if the package_config.json is already present from the preparation step
+    final packageConfig = File(_getPackageConfigPathForPackage(packagePath));
+    if (!packageConfig.existsSync()) {
+      await stdoutSession.writeln('Running pub get');
+      await PubInteraction.runPubGet(packagePath, stdoutSession: stdoutSession);
+    } else {
+      await stdoutSession
+          .writeln('Omitting pub get (package config already present)');
+    }
+
+    await stdoutSession.writeln('Analyzing $path');
     final analyzer = PackageApiAnalyzer(
       packagePath: packagePath,
       doAnalyzePlatformConstraints: doAnalyzePlatformConstraints,
@@ -152,55 +151,6 @@ Affects only local references.
     return Future.value();
   }
 
-  Future<Set<String>> _listPathDependencies(String packagePath) async {
-    File pubspecFile = File(p.join(packagePath, 'pubspec.yaml'));
-    if (!pubspecFile.existsSync()) {
-      throw 'Cannot find pubspec.yaml at ${pubspecFile.path}, while searching for path dependencies.';
-    }
-
-    Set<String> pathDependencies = {};
-
-    final yamlContent = await pubspecFile.readAsString();
-    final pubSpec = Pubspec.parse(yamlContent);
-    await Future.forEach<Dependency>([
-      ...pubSpec.dependencies.values,
-      ...pubSpec.devDependencies.values,
-    ], (dependency) async {
-      if (dependency is PathDependency) {
-        String pathDependencyPath =
-            p.normalize(p.join(packagePath, dependency.path));
-        pathDependencies.add(pathDependencyPath);
-        pathDependencies = pathDependencies
-            .union(await _listPathDependencies(pathDependencyPath));
-      }
-    });
-
-    return pathDependencies;
-  }
-
-  String _commonRootPathForPackagesPaths({required List<String> paths}) {
-    if (paths.isEmpty) {
-      return '';
-    }
-    return paths.reduce((value, element) {
-      final valueComponents = p.split(value);
-      final elementComponents = p.split(element);
-      List<String> commonComponents = [];
-
-      for (int i = 0;
-          i < min(valueComponents.length, elementComponents.length);
-          i++) {
-        if (valueComponents[i] == elementComponents[i]) {
-          commonComponents.add(valueComponents[i]);
-        } else {
-          break;
-        }
-      }
-
-      return p.joinAll(commonComponents);
-    });
-  }
-
   bool _doNothing(String from, String to) {
     if (p.canonicalize(from) == p.canonicalize(to)) {
       return true;
@@ -231,3 +181,6 @@ Affects only local references.
     }
   }
 }
+
+String _getPackageConfigPathForPackage(String packagePath) =>
+    p.join(packagePath, '.dart_tool', 'package_config.json');
diff --git a/lib/src/cli/commands/diff_command.dart b/lib/src/cli/commands/diff_command.dart
@@ -103,8 +103,6 @@ You may want to do this if you want to make sure
   Future<int> run() async {
     final oldPackageRef = PackageRef(argResults![_optionNameOld]);
     final newPackageRef = PackageRef(argResults![_optionNameNew]);
-    final shouldCheckPathDependencies =
-        argResults![_optionNameIncludePathDependencies] as bool;
     final versionCheckMode = VersionCheckMode.values.firstWhere(
         (element) => element.name == argResults![_optionNameVersionCheckMode]);
     final ignorePrerelease = argResults![_optionNameIgnorePrerelease] as bool;
@@ -123,11 +121,9 @@ You may want to do this if you want to make sure
 
     final preparedOldPackageRef = await prepare(
       oldPackageRef,
-      shouldCheckPathDependencies: shouldCheckPathDependencies,
     );
     final preparedNewPackageRef = await prepare(
       newPackageRef,
-      shouldCheckPathDependencies: shouldCheckPathDependencies,
     );
 
     final oldPackageApi = await analyze(

diff --git a/lib/src/cli/commands/extract_command.dart b/lib/src/cli/commands/extract_command.dart
@@ -65,8 +65,6 @@ If not specified the extracted API will be printed to the console.
   @override
   Future<int> run() async {
     final packageRef = PackageRef(argResults![_optionNameInput]);
-    final shouldCheckPathDependencies =
-        argResults![_optionNameIncludePathDependencies] as bool;
     final noAnalyzePlatformConstraints =
         argResults![_optionNameNoAnalyzePlatformConstraints] as bool;
     final doRemoveExample = argResults![_optionNameRemoveExample] as bool;
@@ -75,7 +73,6 @@ If not specified the extracted API will be printed to the console.
 
     final preparedPackageRef = await prepare(
       packageRef,
-      shouldCheckPathDependencies: shouldCheckPathDependencies,
     );
     final packageApi = await analyze(
       preparedPackageRef,

diff --git a/lib/src/cli/source_item.dart b/lib/src/cli/source_item.dart
@@ -7,26 +7,14 @@ import 'package:path/path.dart' as path;
 class SourceItem {
   final String relativeDestinationDir;
   final String sourceDir;
+  final bool isInCache;
 
   SourceItem({
     required this.sourceDir,
+    required this.isInCache,
     this.relativeDestinationDir = '.',
   });
 
-  factory SourceItem.forCommonPath({
-    required String sourceDir,
-    required String commonPath,
-  }) {
-    if (path.isWithin(commonPath, sourceDir)) {
-      final commonRelativePath = path.relative(sourceDir, from: commonPath);
-      return SourceItem(
-        sourceDir: sourceDir,
-        relativeDestinationDir: commonRelativePath,
-      );
-    }
-    return SourceItem(sourceDir: sourceDir);
-  }
-
   String destinationPath({required String forPrefix}) {
     return path.normalize(path.join(forPrefix, relativeDestinationDir));
   }

diff --git a/lib/src/diff/package_api_differ.dart b/lib/src/diff/package_api_differ.dart
@@ -718,8 +718,8 @@ class PackageApiDiffer {
   }
 
   List<ApiChange> _calculateSuperTypesDiff(
-    List<String> oldSuperTypes,
-    List<String> newSuperTypes,
+    Set<String> oldSuperTypes,
+    Set<String> newSuperTypes,
     Stack<Declaration> context, {
     required bool isExperimental,
   }) {

diff --git a/lib/src/model/interface_declaration.dart b/lib/src/model/interface_declaration.dart
@@ -42,8 +42,8 @@ class InterfaceDeclaration with _$InterfaceDeclaration implements Declaration {
     /// list of type parameter names
     required List<String> typeParameterNames,
 
-    /// list of super type names
-    required List<String> superTypeNames,
+    /// set of super type names
+    required Set<String> superTypeNames,
 
     /// executables that belong to this interface
     required List<ExecutableDeclaration> executableDeclarations,