Split Version.props into Version.props and Version.Details.props by dkurepa · Pull Request #5067 · dotnet/arcade-services

dkurepa · 2025-07-21T13:14:17Z

Copilot

Pull Request Overview

This PR introduces support for managing dependency properties in a new file called VersionDetailsProps as an alternative to the existing VersionProps file in the dependency management system. The change allows for better separation of concerns between manually managed properties and auto-generated ones.

Key changes:

Added support for generating and managing Version.Details.props files alongside existing version management files
Implemented validation to prevent property conflicts between VersionDetailsProps and VersionProps files
Updated dependency file management to optionally use the new props file when available

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`src/Microsoft.DotNet.Darc/DarcLib/Helpers/VersionFiles.cs`	Added constant for new VersionDetailsProps file path
`src/Microsoft.DotNet.Darc/DarcLib/Helpers/IDependencyFileManager.cs`	Updated interface to include optional parameter for VersionDetailsProps detection
`src/Microsoft.DotNet.Darc/DarcLib/Helpers/DependencyFileManager.cs`	Implemented VersionDetailsProps generation and conditional dependency management logic
`src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs`	New merge policy to validate property separation between files
`test/Microsoft.DotNet.DarcLib.Tests/Helpers/DependencyFileManagerTests.cs`	Added comprehensive tests for VersionDetailsProps functionality
`test/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/VmrVersionFileMergerTests.cs`	Updated mock setups for new optional parameter

Comments suppressed due to low confidence (2)

test/Microsoft.DotNet.DarcLib.Tests/Helpers/DependencyFileManagerTests.cs:10

This appears to be an unused import that was removed. Consider verifying that FluentAssertions.Specialized is not needed elsewhere in the file before removing it.

using Microsoft.DotNet.DarcLib.Helpers;

src/Microsoft.DotNet.Darc/DarcLib/Helpers/DependencyFileManager.cs:749

[nitpick] Consider adding a blank line after the XML declaration for better readability, following the existing pattern in the method.

src/Microsoft.DotNet.Darc/DarcLib/Helpers/GitFileContentContainer.cs

…sProps

dkurepa · 2025-07-21T14:30:45Z

src/Microsoft.DotNet.Darc/DarcLib/Helpers/DependencyFileManager.cs

        string branch,
-        DependencyDetail dependency)
+        DependencyDetail dependency,
+        bool repoHasVersionDetailsProps)


One issue with adding/removing individual dependencies like this is that we always have to load the xml from git, do our operation, and then regenerate the VersionDetailsProps. We should have something like RemoveDependenciesAsync that would only load the files once and only generate Version.Details.props once. I kind of feel like it'd be better to that in a separate PR

Doesn't seem to be a huge deal. Since all additions and removals are committed separately anyway, re-reading files is not a bottleneck

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs

…cy.cs

…o dkurepa/VersionDetailsProps

adamzip · 2025-07-22T13:23:15Z

src/Microsoft.DotNet.Darc/DarcLib/Helpers/DependencyFileManager.cs

        string branch,
-        DependencyDetail dependency)
+        DependencyDetail dependency,
+        bool repoHasVersionDetailsProps)


Doesn't seem to be a huge deal. Since all additions and removals are committed separately anyway, re-reading files is not a bottleneck

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs

dkurepa · 2025-07-22T13:51:26Z

Doesn't seem to be a huge deal. Since all additions and removals are committed separately anyway, re-reading files is not a bottleneck

It's an IO operation that's happening a lot, and not just there, later we add all non excluded build dependencies, and there can be quite a lot of them.
Every time I run the service locally, it actually spends quite some time doing these operations, so I think it'll be a nice improvement

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs

tools/ProductConstructionService.ReproTool/Operations/FlowCommitOperation.cs

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs

src/Maestro/Maestro.MergePolicies/StandardMergePolicy.cs

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs

src/Microsoft.DotNet.Darc/DarcLib/Helpers/DependencyFileManager.cs

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs

premun · 2025-07-23T13:58:01Z

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs

+            if (foundProperties.Count > 0)
+            {
+                StringBuilder str = new();
+                str.AppendLine("Properties from `VersionDetailsProps` should not be present in `VersionProps`.");


I am not sure this is a valid check. I think we were saying we could now start pinning properties by overwriting them in Versions.props.

why would we need to do that tho? We can just pin them in Version.Detail.xml or not have them there at all

Michael Simons had a point where you don't want to set Pinned=True but still want to occasionally receive an update somehow. I don't remember how it went.

Okay, so we agreed that we can keep this but we will ignore a case where we override the property to empty

premun

@dkurepa will everything work even when updating dependencies via darc? Meaning - will it overwrite the new version file if it already exists?

dkurepa · 2025-07-24T11:40:18Z

@dkurepa will everything work even when updating dependencies via darc? Meaning - will it overwrite the new version file if it already exists?

It should work since it's using the AddDependencyAsync method that we're using in the service flows too. If Version.Details.props exists, it will get regenerated based on Version.Details.xml

premun · 2025-07-24T12:02:35Z

It should work

Can you test it?

akoeplinger · 2025-07-24T14:45:04Z

it means I need an updated darc right?

dkurepa · 2025-07-24T14:47:29Z

It should work

Can you test it?

sure, will do it

it means I need an updated darc right?

yes, if you use darc add-dependency/update-dependencies or the vmr flow commands

akoeplinger · 2025-07-24T15:05:49Z

ok. people often run months old darc so that might be an issue. can we block/warn users with too-old darc?

dkurepa · 2025-07-24T15:19:24Z

ok. people often run months old darc so that might be an issue. can we block/warn users with too-old darc?

I don't think we can :/ all of these are just operations with your local files. Maybe if we made the files somehow unparseable for the older versions, but that doesn't sound like a great idea

akoeplinger · 2025-07-24T18:30:15Z

all of these are just operations with your local files

update-dependencies will call the service to grab the build info, it could send along the current darc version in e.g. the User Agent. but this doesn't need to be solved here :)

premun · 2025-07-25T07:20:30Z

I hope that if people run old darc locally and an unexpected bunch of changes appear in the Versions.props because it puts the props there, the dev either notices and if not and they even manage to check it in, it will show up in the codeflow PRs where the checks will fail once it dumps the duplicate props in the V.D.props?

dkurepa · 2025-07-25T09:02:29Z

It should work

Can you test it?

Tried it out, it works as expected in both cases (with and without Version.Details.props)

…ty attribute changes (#5346) - [x] Review PR #5067 and #5209 changes to understand what was implemented - [x] Add section on Version.Details.props file - [x] Document the SkipProperty attribute in Version.Details.xml - [x] Document header generation behavior for repos with only SkipProperty dependencies - [x] Add examples showing proper configuration - [x] Update index and references throughout the document - [x] Build and test to ensure no syntax errors in documentation - [x] Address review feedback: Remove "When to use SkipProperty" section - [x] Address review feedback: Simplify migration steps  <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Update Darc.md documentation for Version.Details.props and SkipProperty attribute changes</issue_title> > <issue_description>Update the Darc.md documentation to reflect changes implemented in the following PRs: > - #5067 > - #5209 > > Key points to address: > > 1. **Version.Details.props File** > - Describe the addition of the new `Version.Details.props` file. > - Document that if `Version.Details.props` exists, it will be used. > - Explain when and how this file is generated or consumed. > > 2. **SkipProperty Attribute in Version.Details.xml** > - Introduce and document the `SkipProperty` attribute that can be added in `Version.Details.xml`. > - Describe its purpose and how it affects dependency management. > > 3. **Other Relevant Changes** > - Summarize any additional relevant changes from the referenced PRs, such as header generation behavior for repos with only `SkipProperty` dependencies. > > Ensure examples and configuration/process details are clear for users maintaining or integrating with Darc-managed repositories.</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> Fixes #5345  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: dkurepa <91743470+dkurepa@users.noreply.github.com> Co-authored-by: Přemek Vysoký <premek.vysoky@microsoft.com>

dkurepa added 4 commits July 17, 2025 14:46

implementation

8bf3fc1

add unit testing

96f07b3

Create Merge Policy

f1c240e

IMplement the merge policy and add tests for it, improve other stuff

4d3343a

Copilot AI review requested due to automatic review settings July 21, 2025 13:14

Copilot AI reviewed Jul 21, 2025

View reviewed changes

src/Microsoft.DotNet.Darc/DarcLib/Helpers/GitFileContentContainer.cs Show resolved Hide resolved

Merge remote-tracking branch 'source/main' into dkurepa/VersionDetail…

b01cd17

…sProps

dkurepa changed the title ~~Version details props~~ Split Version.props into Version.props and Version.Details.props Jul 21, 2025

dkurepa commented Jul 21, 2025

View reviewed changes

dkurepa added 2 commits July 21, 2025 16:37

suppress warning in test proj

5f1e070

Fix failing unit test

61c798a

dkurepa mentioned this pull request Jul 21, 2025

Split Versions.props into two files: one for humans, and one for Maestro/Darc #4998

Closed

dkurepa added 2 commits July 21, 2025 17:37

Also check if Version.Details.props was imported

9c3c743

Make the messages nicer

645460e

dkurepa requested review from adamzip, mmitche and premun July 21, 2025 16:36

dkurepa commented Jul 22, 2025

View reviewed changes

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs Outdated Show resolved Hide resolved

dkurepa and others added 3 commits July 22, 2025 12:30

Update src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePoli…

5cc416e

…cy.cs

enable nullability in new file

74009b0

Merge remote-tracking branch 'origin/dkurepa/VersionDetailsProps' int…

40a4469

…o dkurepa/VersionDetailsProps

adamzip previously approved these changes Jul 22, 2025

View reviewed changes

adamzip reviewed Jul 22, 2025

View reviewed changes

src/Maestro/Maestro.MergePolicies/VersionDetailsPropsMergePolicy.cs Outdated Show resolved Hide resolved

Better exceltion messaging in VersionDetailsPropsMergePolicy

17ab6f9

dkurepa dismissed adamzip’s stale review via 17ab6f9 July 23, 2025 08:36

dkurepa requested a review from adamzip July 23, 2025 10:35

adamzip previously approved these changes Jul 23, 2025

View reviewed changes

premun reviewed Jul 23, 2025

View reviewed changes

adamzip self-requested a review July 23, 2025 15:31

Also check if Version.Details.props is as it should be

80effc2

dkurepa dismissed adamzip’s stale review via 80effc2 July 24, 2025 09:58

dkurepa added 2 commits July 24, 2025 12:51

Resolve comments

23b3552

fix up tests

bd40f02

premun reviewed Jul 24, 2025

View reviewed changes

Revert testing changes

6c2ccab

Allow verions.props property duplicates if they're empty

03d6aa8

premun approved these changes Jul 25, 2025

View reviewed changes

dkurepa merged commit 90f4ad8 into dotnet:main Jul 25, 2025
9 checks passed

dkurepa mentioned this pull request Oct 7, 2025

Update Darc.md documentation for Version.Details.props and SkipProperty attribute changes #5345

Closed

Copilot AI mentioned this pull request Oct 7, 2025

Update Darc.md documentation for Version.Details.props and SkipProperty attribute changes #5346

Merged

9 tasks

Conversation

dkurepa commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

dkurepa Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

adamzip Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamzip Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dkurepa commented Jul 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

premun Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

dkurepa Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

premun Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

premun Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

premun left a comment

Choose a reason for hiding this comment

Uh oh!

dkurepa commented Jul 24, 2025

Uh oh!

premun commented Jul 24, 2025

Uh oh!

akoeplinger commented Jul 24, 2025

Uh oh!

dkurepa commented Jul 24, 2025

Uh oh!

akoeplinger commented Jul 24, 2025

Uh oh!

dkurepa commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akoeplinger commented Jul 24, 2025

Uh oh!

premun commented Jul 25, 2025

Uh oh!

dkurepa commented Jul 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dkurepa commented Jul 21, 2025 •

edited

Loading

dkurepa commented Jul 24, 2025 •

edited

Loading