Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions script/tool/lib/src/make_deps_path_based_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,10 @@ class MakeDepsPathBasedCommand extends PackageCommand {

// Find the relative path from the common base to the local package.
final List<String> repoRelativePathComponents = path.split(
path.relative(localDependencies[packageName]!.path, from: repoRootPath),
path.relative(
localDependencies[packageName]!.directory.absolute.path,
from: repoRootPath,
),
);
final String pathValue = p.posix.joinAll(<String>[
...relativeBasePathComponents,
Expand Down Expand Up @@ -272,11 +275,14 @@ ${newOverrideLines.join('\n')}
// example app doesn't. Since integration tests are run in the example app,
// it needs the overrides in order for tests to pass.
for (final RepositoryPackage example in package.getExamples()) {
final String parentPackageName = package.parsePubspec().name;
await _addDependencyOverridesIfNecessary(
example,
localDependencies,
<String, RepositoryPackage>{...localDependencies, parentPackageName: package},
versions,
additionalPackagesToOverride: packagesToOverride,
// Add an override to the parent package in case a transitive dependency has a dependency on it,
// since that (non-path) dependency would conflict with the path-based dependency in the example.
additionalPackagesToOverride: <String>{...packagesToOverride, parentPackageName},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an inline comment on parentPackgaeName being here? Since foo/example always depends on foo by path, it's really non-obvious why this would be necessary outside the context of someone reviewing this PR :)

Maybe something like:

// Add an override to the parent package in case a transitive dependency has a dependency on it,
// since that (non-path) dependency would conflict with the path-based dependency in the example.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! On the way. Thank you!

);
}

Expand Down
50 changes: 48 additions & 2 deletions script/tool/test/make_deps_path_based_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ ${overrides.map((String dep) => ' $dep:\n path: $path').join('\n')}
final Map<String, String?> exampleOverrides = getDependencyOverrides(
pluginAppFacing.getExamples().first,
);
expect(exampleOverrides.length, 1);
expect(exampleOverrides.length, 2);
expect(exampleOverrides['bar'], '../../../../packages/bar/bar');
expect(exampleOverrides['bar_android'], '../../../../packages/bar/bar_android');
});

Expand All @@ -248,8 +249,9 @@ ${overrides.map((String dep) => ' $dep:\n path: $path').join('\n')}
final Map<String, String?> exampleOverrides = getDependencyOverrides(
pluginAppFacing.getExamples().first,
);
expect(exampleOverrides.length, 2);
expect(exampleOverrides.length, 3);
expect(exampleOverrides['another_package'], '../../../../packages/another_package');
expect(exampleOverrides['bar'], '../../../../packages/bar/bar');
expect(exampleOverrides['bar_android'], '../../../../packages/bar/bar_android');
});

Expand All @@ -264,6 +266,50 @@ ${overrides.map((String dep) => ' $dep:\n path: $path').join('\n')}
expect(exampleOverrides.length, 0);
});

test(
'example overrides include parent package to resolve dependency overrides cleanly',
() async {
createFakePackage('material_ui', packagesDir);
final RepositoryPackage cupertinoUi = createFakePlugin('cupertino_ui', packagesDir);

addDevDependenciesSection(cupertinoUi, <String>['material_ui']);

await runCapturingPrint(runner, <String>[
'make-deps-path-based',
'--target-dependencies=material_ui',
]);

final Map<String, String?> exampleOverrides = getDependencyOverrides(
cupertinoUi.getExamples().first,
);
expect(exampleOverrides['material_ui'], '../../../packages/material_ui');
expect(exampleOverrides['cupertino_ui'], '../../../packages/cupertino_ui');
},
);

test(
'does not recursively override dependencies of target packages to preserve safety net',
() async {
final RepositoryPackage clientPkg = createFakePackage('pkg_client', packagesDir);
final RepositoryPackage targetPkg = createFakePackage('pkg_target', packagesDir);
createFakePackage('pkg_dependency', packagesDir);

addDependencies(clientPkg, <String>['pkg_target', 'pkg_dependency']);
addDependencies(targetPkg, <String>['pkg_dependency']);

await runCapturingPrint(runner, <String>[
'make-deps-path-based',
'--target-dependencies=pkg_target',
]);

final Map<String, String?> clientOverrides = getDependencyOverrides(clientPkg);
expect(clientOverrides['pkg_target'], '../../packages/pkg_target');
// Dependencies of target packages must not be recursively pathified,
// ensuring clients are tested against published versions to preserve safety.
expect(clientOverrides['pkg_dependency'], isNull);
},
);

test(
'alphabetizes overrides from different sections to avoid lint warnings in analysis',
() async {
Expand Down
Loading