Do not ignore non-updated dependencies at version merger #5332
Conversation
adamzip
force-pushed
the
prevent-version-downgrades
branch
from
2b6dbe1 to
ffd7737
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR changes version/dependency diff logic to stop filtering out entries whose version/commit/repo URI did not change, causing all dependencies/properties to be surfaced as "changes". Key intent appears to be ensuring non-updated dependencies are not ignored in merge operations.
- Removed filtering in ComputeChanges so unchanged dependencies are now returned.
- Modified FlatJson.GetDiff to always add Updated entries, potentially including unchanged and even removed keys.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VersionDetailsFileMerger.cs | Returns all dependencyChanges instead of only those with field differences. |
| src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/FlatJson.cs | Unconditionally adds Updated entries, removing equality guard logic. |
Comments suppressed due to low confidence (1)
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/FlatJson.cs:1
- The unconditional addition on line 66 causes a Removed entry (lines 62-64 when the key is absent) to be immediately followed by an Updated entry for the same key, and also flags unchanged values as Updated. This produces contradictory or misleading diff results. Restore the previous conditional (e.g., use else if (!kvp.Value.Equals(newValue))) or introduce an explicit Unchanged result (if supported) and skip adding Updated when the key is missing or the value is identical.
// Licensed to the .NET Foundation under one or more agreements.
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VersionDetailsFileMerger.cs
Show resolved
Hide resolved
| Commit = build2.Commit, | ||
| Version = "1.0.3", // Not part of the last 2 builds | ||
| RepoUri = build1.GitHubRepository, | ||
| Commit = build1.Commit, |
There was a problem hiding this comment.
Why did it have to be downgraded?
There was a problem hiding this comment.
I think we will have to understand this because maybe we just introduced a bug?
| List<(string, string)> expectedUpdates = [ | ||
| ("Package.Updated.In.Both", "3.0.0")]; | ||
| ("Package.From.Build", "1.0.1"), | ||
| ("Package.Updated.In.Both", "3.0.0"), |
There was a problem hiding this comment.
Why is this in the updates if it didn't change and stayed at 3.0.0?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I see, yeah Updates is misleading
2 participants
#5331 (comment)
Repro'd PR: maestro-auth-test/emsdk#9
I also repro'd without the fix to confirm the bug.