Skip to content

Do not ignore non-updated dependencies at version merger #5332

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

Closed
wants to merge 5 commits into from
Closed

Do not ignore non-updated dependencies at version merger #5332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ffd7737
include non-updated dependencies in version merger
adamzip Oct 3, 2025
4b39e8f
rename method to better suit functionality
adamzip Oct 4, 2025
f905823
fix failing unit tests
adamzip Oct 5, 2025
481d575
fix diff logic
adamzip Oct 5, 2025
24f058a
questionable unit test 'fixes'
adamzip Oct 6, 2025
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
14 changes: 8 additions & 6 deletions src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/FlatJson.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,24 +48,26 @@ public List<JsonVersionProperty> GetDiff(FlatJson otherJson)
if (!otherJson.FlatValues.TryGetValue(kvp.Key, out var newValue))
{
changes.Add(new JsonVersionProperty(kvp.Key, NodeComparisonResult.Removed, null));
continue;
}
else if (kvp.Value.GetType() != newValue.GetType())

if (kvp.Value.GetType() != newValue.GetType())
{
throw new ArgumentException($"Key {kvp.Key} value has different types in old and new json");
}
else if (kvp.Value.GetType() == typeof(List<string>))

if (kvp.Value.GetType() == typeof(List<string>))
{
var oldList = (List<string>)kvp.Value;
var newList = (List<string>)newValue;
if (!oldList.SequenceEqual(newList))
{
changes.Add(new JsonVersionProperty(kvp.Key, NodeComparisonResult.Updated, newValue));
continue;
}
}
else if (!kvp.Value.Equals(newValue))
{
changes.Add(new JsonVersionProperty(kvp.Key, NodeComparisonResult.Updated, newValue));
}

changes.Add(new JsonVersionProperty(kvp.Key, NodeComparisonResult.Updated, newValue));
}

foreach (var kvp in otherJson.FlatValues)
Expand Down
13 changes: 4 additions & 9 deletions src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VersionDetailsFileMerger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,11 @@ public async Task<VersionFileChanges<DependencyUpdate>> MergeVersionDetails(
var previousSourceRepoChanges = await GetDependencies(sourceRepo, sourceRepoPreviousRef, sourceRepoVersionDetailsRelativePath);
var currentSourceRepoChanges = await GetDependencies(sourceRepo, sourceRepoCurrentRef, sourceRepoVersionDetailsRelativePath);

List<DependencyUpdate> targetChanges = ComputeChanges(
List<DependencyUpdate> targetChanges = ComputeDependencyDiffs(
previousTargetRepoChanges,
currentTargetRepoChanges);

List<DependencyUpdate> sourceChanges = ComputeChanges(
List<DependencyUpdate> sourceChanges = ComputeDependencyDiffs(
previousSourceRepoChanges,
currentSourceRepoChanges);

Expand All @@ -105,7 +105,7 @@ private async Task<VersionDetails> GetDependencies(ILocalGitRepo repo, string co
: _versionDetailsParser.ParseVersionDetailsXml(content, includePinned: false);
}

private static List<DependencyUpdate> ComputeChanges(VersionDetails before, VersionDetails after)
private static List<DependencyUpdate> ComputeDependencyDiffs(VersionDetails before, VersionDetails after)
{
var dependencyChanges = before.Dependencies
.Select(dep => new DependencyUpdate()
Expand All @@ -132,12 +132,7 @@ private static List<DependencyUpdate> ComputeChanges(VersionDetails before, Vers
}
}

// Check if there are any actual changes
return dependencyChanges
.Where(change => change.From?.Version != change.To?.Version
|| change.From?.Commit != change.To?.Commit
|| change.From?.RepoUri != change.To?.RepoUri)
.ToList();
return dependencyChanges;
}

private async Task<VersionFileChanges<DependencyUpdate>> ApplyVersionDetailsChangesAsync(ILocalGitRepo repo, VersionFileChanges<DependencyUpdate> changes, string? mapping = null)
Expand Down
20 changes: 10 additions & 10 deletions test/Microsoft.DotNet.DarcLib.Codeflow.Tests/BackflowTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,19 +235,19 @@ .. GetExpectedVersionFiles(ProductRepoPath),
new DependencyDetail
{
Name = "Package.D3",
Version = "1.0.2", // Not part of the last 2 builds
RepoUri = build2.GitHubRepository,
Commit = build2.Commit,
Version = "1.0.3", // Not part of the last 2 builds
RepoUri = build1.GitHubRepository,
Commit = build1.Commit,
Copy link
Contributor Author

@adamzip adamzip Oct 6, 2025

Choose a reason for hiding this comment

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

The changes in this file are questionable - I didn't completely understand the original behavior, so I'm not sure if it's right. @dkurepa @premun

Copy link
Member

@premun premun Oct 10, 2025

Choose a reason for hiding this comment

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

Why did it have to be downgraded?

Copy link
Contributor Author

@adamzip adamzip Oct 10, 2025

Choose a reason for hiding this comment

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

I don't know

Copy link
Member

@premun premun Oct 10, 2025

Choose a reason for hiding this comment

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

I think we will have to understand this because maybe we just introduced a bug?

Type = DependencyType.Product,
Pinned = false,
},

new DependencyDetail
{
Name = DependencyFileManager.ArcadeSdkPackageName,
Version = "1.0.2",
RepoUri = build2.GitHubRepository,
Commit = build2.Commit,
Version = "1.0.1",
RepoUri = build1.GitHubRepository,
Commit = build1.Commit,
Type = DependencyType.Toolset,
Pinned = false,
},
Expand All @@ -263,7 +263,7 @@ .. GetExpectedVersionFiles(ProductRepoPath),
DependencyFileManager dependencyFileManager = GetDependencyFileManager();
JObject globalJson = await dependencyFileManager.ReadGlobalJsonAsync(ProductRepoPath, branchName + "-pr", relativeBasePath: null);
JToken? arcadeVersion = globalJson.SelectToken($"msbuild-sdks.['{DependencyFileManager.ArcadeSdkPackageName}']", true);
arcadeVersion?.ToString().Should().Be("1.0.2");
arcadeVersion?.ToString().Should().Be("1.0.1");

var dotnetVersion = await dependencyFileManager.ReadToolsDotnetVersionAsync(ProductRepoPath, branchName + "-pr", relativeBasePath: null);
dotnetVersion.ToString().Should().Be(Constants.VmrBaseDotnetSdkVersion);
Expand Down Expand Up @@ -319,9 +319,9 @@ .. GetExpectedVersionFiles(ProductRepoPath),
new DependencyDetail
{
Name = DependencyFileManager.ArcadeSdkPackageName,
Version = "1.0.2",
RepoUri = build2.GitHubRepository,
Commit = build2.Commit,
Version = "1.0.1",
RepoUri = build1.GitHubRepository,
Commit = build1.Commit,
Type = DependencyType.Toolset,
Pinned = false,
},
Expand Down
22 changes: 1 addition & 21 deletions test/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/SimpleConfigJsonTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public void CompareFlatJsons_AllScenarios_DetectsAllChanges()
var changes = oldSimpleConfigJson.GetDiff(newSimpleConfigJson);

// Assert
changes.Should().HaveCount(6);
changes.Should().HaveCount(10);

// Check removed property
var removedChange = changes.Should().ContainSingle(c => c.Name == "removed:property").Subject;
Expand Down Expand Up @@ -157,26 +157,6 @@ public void CompareFlatJsons_AllScenarios_DetectsAllChanges()
updatedToolsChange.Value.Should().Be("8.0.400");
}

[Test]
public void CompareFlatJsons_IdenticalJsons_ReturnsEmptyChanges()
{
// Arrange
var json = new Dictionary<string, object>
{
["sdk:version"] = "8.0.303",
["isRoot"] = true,
["frameworks"] = new List<string> { "net8.0", "net6.0" }
};

// Act
var oldSimpleConfigJson = new FlatJson(json);
var newSimpleConfigJson = new FlatJson(new Dictionary<string, object>(json));
var changes = oldSimpleConfigJson.GetDiff(newSimpleConfigJson);

// Assert
changes.Should().BeEmpty();
}

[Test]
public void CompareFlatJsons_DifferentTypesForSameKey_ThrowsArgumentException()
{
Expand Down
6 changes: 4 additions & 2 deletions test/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/VersionDetailsFileMergerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ public async Task MergeVersionDetails_BackFlow_UsesPreviousVmrDependencies()
result.Additions.Should().HaveCount(2);
// there should only be one removal because Package.That.Is.Already.Removed.In.Repo was already removed in the target repo
result.Removals.Should().HaveCount(1);
result.Updates.Should().HaveCount(1);
result.Updates.Should().HaveCount(2);
List<(string, string)> expectedAdditions = [
("Package.Added.In.Both", "2.2.2"),
("Package.Added.In.VMR", "2.0.0")];
Expand All @@ -195,7 +195,9 @@ public async Task MergeVersionDetails_BackFlow_UsesPreviousVmrDependencies()
.Should()
.BeEquivalentTo(expectedAdditions, options => options.WithStrictOrdering());
List<(string, string)> expectedUpdates = [
("Package.Updated.In.Both", "3.0.0")];
("Package.From.Build", "1.0.1"),
("Package.Updated.In.Both", "3.0.0"),
Copy link
Member

@premun premun Oct 10, 2025

Choose a reason for hiding this comment

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

Why is this in the updates if it didn't change and stayed at 3.0.0?

Copy link
Contributor Author

@adamzip adamzip Oct 10, 2025

Choose a reason for hiding this comment

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

In this case the PR changed MergeVersionDetails to include existing dependencies whose versions didn't change. At the end, after all the comparisons are done, UpdateDependenciesAndToolsets would filter out dependencies whose old/new versions are the same.
If we do keep some kind of logic like that, we could rename result.Updates to result.ExistingDependencies or something like that - to distinguish them from removed and added dependencies

Copy link
Member

@premun premun Oct 10, 2025

Choose a reason for hiding this comment

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

I see, yeah Updates is misleading

];
result.Updates.Values
.Select(u => (DependencyDetail)u.Value!)
.Select(d => (d.Name, d.Version))
Expand Down