Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix git dependencies comparison on bootstrap #659

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: fix git dependencies comparison on bootstrap #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef8399b
fix: fix git dependencies comparison on bootstrap
mateusfccp Mar 13, 2024
11a72eb
Add todo to code
mateusfccp Mar 14, 2024
c03ce6a
add test to behavior fix
mateusfccp Mar 29, 2024
da61c56
Merge remote-tracking branch 'upstream/main'
mateusfccp Nov 28, 2024
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
16 changes: 15 additions & 1 deletion packages/melos/lib/src/commands/bootstrap.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,14 @@ mixin _BootstrapMixin on _CleanMixin {
return didUpdate;
}

bool _areDependenciesEqual(DependencyReference? a, DependencyReference? b) {
if (a is GitReference && b is GitReference) {
return a == b && a.path == b.path;
} else {
return a == b;
}
}

int _updateDependencies({
required YamlEditor pubspecEditor,
required Map<String, Dependency>? workspaceDependencies,
Expand All @@ -363,7 +371,13 @@ mixin _BootstrapMixin on _CleanMixin {
// dependencies that have a different version specified in the workspace.
final dependenciesToUpdate = workspaceDependencies.entries.where((entry) {
if (!packageDependencies.containsKey(entry.key)) return false;
if (packageDependencies[entry.key] == entry.value) return false;
// TODO: We may want to replace the `pubspec` dependency with something
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is replaced with pubspec_parse now

Copy link
Author

@mateusfccp mateusfccp Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this PR obsolete or does the bug still exist? I am updating it, but I realized that the package has changed and I am not sure if it solves the problem by itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure. Can you try running main?

// else that is actively maintained, so we don't have to provide our own
// equality logic.
// See: https://github.com/invertase/melos/discussions/663
if (_areDependenciesEqual(packageDependencies[entry.key], entry.value)) {
mateusfccp marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
return true;
});

Expand Down
120 changes: 120 additions & 0 deletions packages/melos/test/commands/bootstrap_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,126 @@ Generating IntelliJ IDE files...
);
});

test('properly compares the path changes on git references', () async {
final temporaryGitRepository = createTestTempDir();

await io.Process.run(
'git',
['init'],
workingDirectory: temporaryGitRepository.absolute.path,
);

await createProject(
io.Directory('${temporaryGitRepository.absolute.path}/dependency1'),
const PubSpec(name: 'dependency'),
);

await createProject(
io.Directory('${temporaryGitRepository.absolute.path}/dependency2'),
const PubSpec(name: 'dependency'),
);

await io.Process.run(
'git',
['add', '-A'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty directories won't be added to git, maybe create an empty file in both of the directories first?

Copy link
Author

@mateusfccp mateusfccp Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The directory, supposedly, is not empty, as we created two projects within it with createProject.

If this was not the case, the test would not pass in my local machine either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right, I read createDirectory and not createProject when I read it this morning. 😅
The test passes locally for me too (linux), so permissions does sound like a good guess, but it doesn't look like it from the error message in the pipeline, if it doesn't fail silently earlier.

Copy link
Author

Choose a reason for hiding this comment

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

I am going to try to reproduce the problem by using a container locally and running the same images. Maybe I can debug this way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea, I re-ran the failing test with GitHub debug printing, but it didn't give anything

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mateusfccp did you manage to find anything causing it?

Copy link
Author

Choose a reason for hiding this comment

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

Hello, @spydon.

Sorry for the delay. I actually didn't get to reproduce the environment on my local machine. I tried to do it with act, but with no success.

workingDirectory: temporaryGitRepository.absolute.path,
);

await io.Process.run(
'git',
['commit', '--message="Initial commit"'],
workingDirectory: temporaryGitRepository.absolute.path,
);

final workspaceDirectory = await createTemporaryWorkspace();

final initialReference = {
'git': {
'url': 'file://${temporaryGitRepository.absolute.path}',
'path': 'dependency1/packages/dependency',
},
};

final package = await createProject(
workspaceDirectory,
PubSpec(
name: 'git_references',
dependencies: {
'dependency': GitReference.fromJson(initialReference),
},
),
);

final logger = TestLogger();
final initialConfig = MelosWorkspaceConfig.fromYaml(
{
'name': 'test',
'packages': const ['packages/**'],
'command': {
'bootstrap': {
'dependencies': {
'dependency': initialReference,
},
},
},
},
path: workspaceDirectory.path,
);

final melosBeforeChangingPath = Melos(
logger: logger,
config: initialConfig,
);

await runMelosBootstrap(melosBeforeChangingPath, logger);

final packageConfig = packageConfigForPackageAt(package);
final dependencyPackage = packageConfig.packages.singleWhere(
(package) => package.name == 'dependency',
);

expect(
dependencyPackage.rootUri,
contains('dependency1/packages/dependency'),
);

final configWithChangedPath = MelosWorkspaceConfig.fromYaml(
{
'name': 'test',
'packages': const ['packages/**'],
'command': {
'bootstrap': {
'dependencies': {
'dependency': {
'git': {
'url': 'file://${temporaryGitRepository.absolute.path}',
'path': 'dependency2/packages/dependency',
},
},
},
},
},
},
path: workspaceDirectory.path,
);

final melosAfterChangingPath = Melos(
logger: logger,
config: configWithChangedPath,
);

await runMelosBootstrap(melosAfterChangingPath, logger);

final alteredPackageConfig = packageConfigForPackageAt(package);
final alteredDependencyPackage = alteredPackageConfig.packages
.singleWhere((package) => package.name == 'dependency');

expect(
alteredDependencyPackage.rootUri,
contains('dependency2/packages/dependency'),
);
});

test(
'resolves workspace packages with path dependency',
() async {
Expand Down
Loading