Rewrite central package versions and version properties in .props files in fallout-migrate by ANcpLua · Pull Request #694 · Fallout-build/Fallout

ANcpLua · 2026-10-03T20:06:39Z

Fixes #492 (central package management: Directory.Packages.props and VersionOverride are not rewritten, so restore fails with NU1010), including the 10.4.0 comment where the version is a property in an imported Version.props.

RewriteCsprojsStep now covers *.props files as well as *.csproj, and classifies version variables over all files before rewriting any of them. It builds on the variable handling from #478 (bump or decouple version variables) instead of replacing it.

What changed

Package items (PackageReference, PackageVersion, PackageDownload) are handled per element: a Nuke.X item becomes Fallout.X, and a literal Version or VersionOverride on it is pinned to the current Fallout version, whatever the attribute order. PackageDownload keeps its [exact] brackets. An item that is already Fallout.X keeps its pin.
A version variable used by a Nuke.* PackageVersion in one file and defined in another is bumped where it is defined. <NukeVersion> in an imported file is renamed to <FalloutVersion> and bumped.
A variable shared with an unrelated package across files is decoupled to $(FalloutVersion) as before. The property is only inserted when no file defines it yet.
An unreadable file contributes nothing to the classification; ApplyRewrite reports it as before.
The System.Security.Cryptography.Xml pin is still removed from *.csproj files only. A pin in a *.props file, such as a root Directory.Build.props, applies to every project in the repository, so it is kept.
The migration step skill and docs/Migration/from-nuke.md now mention *.props and central package versions.

Tests
Nine specs in RewriteCsprojsStepSpecs: central versions, the imported Version.props property, a variable defined in another file, a shared variable across files, VersionOverride, Version before Include, PackageDownload, an already-migrated pin that stays, and a Cryptography.Xml pin in a .props file that stays.

@avidenic is assigned to #492. If you have a change in progress, say so and I will close this one.

🤖 Generated with Claude Code

…es in fallout-migrate Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…rate Now that RewriteCsprojsStep also rewrites *.props files, a System.Security.Cryptography.Xml reference in a root Directory.Build.props was removed too. That reference applies to every project in the repository, not only the build project, so it is kept. Also mention *.props and central package versions in the migration step skill and in docs/Migration/from-nuke.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

ChrisonSimtian

One blocking issue, the rest are optional.

Blocking: <FalloutVersion> is added to unrelated .csproj/.props files when any variable is ambiguous (reproduced, see the inline comment on line 206).

The design is sound: classifying version variables over all files first is the right fix for the imported Version.props case in #492. The specs are well chosen and follow the repo conventions.

ChrisonSimtian · 2026-10-08T07:33:21Z


-        content = EnsureFalloutVersionPropertyExists(content, ambiguousVariables, falloutVersionVariable, falloutVersion,
-            ref edits);
+        if (!variables.DefinesFalloutVersion)


Reproduced on c346a4a. <FalloutVersion> is inserted into every .csproj and .props file, not only the file that needs it.

Setup:

Directory.Packages.props: Nuke.Common and Serilog both use $(ToolsVersion) (ambiguous).

Version.props: defines ToolsVersion.

src/Lib.csproj: unrelated, uses no version variable.

Result: Lib.csproj gets <FalloutVersion>11.0.0</FalloutVersion> added to its first <PropertyGroup>. Version.props is affected too.

Cause: variables.Ambiguous is the same set for every file, and EnsureFalloutVersionPropertyExists only skips a file that already has <FalloutVersion>. Before this PR the classification was per file, so only the file with the ambiguity changed.

Suggestion: insert the property only into a file that contains a redirected $(FalloutVersion) reference. A spec asserting that unrelated files are unchanged would catch this.

Fixed in b54279c. <FalloutVersion> is now added only to a file where a reference was redirected to $(FalloutVersion). Added the spec you suggested: with your setup, Version.props and src/Lib.csproj stay unchanged.

ChrisonSimtian · 2026-10-08T07:33:21Z

+
+    // The literal value of a Version or VersionOverride attribute. PackageDownload needs an exact
+    // range (`[10.1.0]`), so the brackets stay outside the match.
+    private static readonly Regex literalVersionPattern = new(


A version range can be broken here.

For Version="[10.1.0,)" the match is 10.1.0,). The leading [ is consumed by the lookbehind. The result is Version="[11.0.0", which is an invalid range.

Ranges are rare on Nuke packages. Either skip values that contain ,, ( or ), or add a spec that shows the intended result.

Fixed in b54279c. A value that contains ,, ( or ) is no longer matched, so a range is kept as written. The package name is still renamed. A spec covers [10.1.0,) and (10.1.0,11.0.0).

ChrisonSimtian · 2026-10-08T07:33:21Z

+            {
+                return path.ReadAllText();
+            }
+            catch (IOException)


ReadOrEmpty catches only IOException. An UnauthorizedAccessException now crashes the run in this pre-pass. Before, ApplyRewrite would have reported it as a warning.

Suggestion: catch UnauthorizedAccessException too. Every file is also read twice (here and in ApplyRewrite). That is fine for repo-sized inputs, but worth knowing.

Fixed in b54279c. ReadOrEmpty now also catches UnauthorizedAccessException. ApplyRewrite in MigrationFileOperations caught only IOException too, so it also crashed before this PR. It now catches both and reports the file as a warning. No spec, because permission tests are not reliable on CI (for example when the runner is root).

ChrisonSimtian · 2026-10-08T07:33:21Z

@@ -204,7 +268,7 @@ private static (string content, int edits) RedirectAmbiguousVariablesToFalloutVe
            // everything up to and including the opening `Version="` so it can be re-emitted
            // unchanged while just swapping the variable reference.
            var redirectPattern = new Regex(


This pattern still needs Include before Version. Pass 1 now accepts either order, so <PackageVersion Version="$(X)" Include="Nuke.Common" /> is classified as ambiguous but never redirected to $(FalloutVersion).

Same gap before this PR, but it is now easier to hit with .props files. Optional: match the Version attribute separately from the Include attribute.

Fixed in b54279c. Classification and redirect now use one pattern that reads the Include and Version attributes separately, in either order. A spec covers <PackageVersion Version="$(X)" Include="Nuke.Common" />.

ChrisonSimtian · 2026-10-08T07:33:21Z

+        // A variable also shared with a non-Fallout package is ambiguous: bumping it directly would
+        // change that unrelated package's version too, so it's decoupled instead — the Fallout
+        // reference is redirected to a dedicated $(FalloutVersion) property.
+        public static VersionVariables Collect(IEnumerable<string> contents)


Ambiguity is now decided by variable name across all files. If $(Foo) is a per-project property, used by Fallout in project A and by an unrelated package in project B, then A is decoupled even though A's own definition is not shared.

The result is safe, only noisier. A short comment here would document the trade-off.

Added a comment on Collect in b54279c that documents this trade-off.

ChrisonSimtian · 2026-10-08T07:33:21Z

+        @"(?<=\bVersion(?:Override)?=""\[?)(?!\$\()[^""\[\]]+(?=\]?"")",
+        RegexOptions.Compiled);
+
+    // ProjectReference / Remove `Include="Nuke.X"` → `Include="Fallout.X"` — namespace only.


Comment drift: this says "ProjectReference / Remove", but the pattern still matches Include|Update|Remove. It also overlaps nukeIncludePattern. Please fix the comment, or merge the two patterns.

Fixed in b54279c. Merged the two patterns into one nukeItemNamePattern (Include, Update or Remove). Pass 1 and pass 2 both use it, and the comment now matches.

…ut-migrate Review feedback on the central package versions change: - Add the FalloutVersion property only to a file where a reference was redirected to $(FalloutVersion). Before, every .csproj and .props file got the property when any version variable was shared with another package. - Keep a version range such as [10.1.0,) as written. Before, only the lower bound was replaced, which left an invalid range. - Report a file that can't be read because of missing permissions as a warning instead of stopping the run. - Find a version variable when the Version attribute comes before Include. - Merge the two Nuke. prefix patterns into one and document that ambiguity is decided by variable name over all files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ANcpLua · 2026-10-08T08:02:08Z

@ChrisonSimtian All 5 points are addressed in b54279c, with a reply on each thread. Ready for another review. (I can't re-request review myself, because I don't have triage permission on this repo.)

CI for b54279c is waiting for approval from a maintainer. Could you approve the two build runs? https://github.com/Fallout-build/Fallout/actions/runs/37745892640 and https://github.com/Fallout-build/Fallout/actions/runs/37745892575

dennisdoomen

Impressive change, but because all the AI slop, it needs some rework to be understandable by us mere mortals.

dennisdoomen · 2026-10-10T06:50:19Z

+    // produce NU1603 ("not found, falling back to next-higher") which `WarningsAsErrors` in the
+    // migrated project escalates. Pinning in the same pass avoids a broken post-migrate build
+    // (#217). MSBuild variables (`$(...)`) are handled by HandleMsBuildVariable below.
+    private static readonly Regex packageItemPattern = new(


🔧 We need to start clarifying all these regexes in a human-friendly way. Like this one

/// <summary> /// Finds the opening tag of a NuGet package element in an MSBuild file. /// It matches <c><PackageReference></c>, <c><PackageVersion></c>, /// and <c><PackageDownload></c>. /// </summary> /// <remarks> /// The match is the whole opening tag, including all attributes. /// It does not return the package name or version as separate values. /// The match is case-sensitive. A <c>></c> character inside an attribute value /// ends the match early. /// </remarks>

Same for the other regexes that got affected by this PR

dennisdoomen · 2026-10-10T06:53:57Z

    public Task ExecuteAsync(MigrationContext context, Summary summary)
    {
-        foreach (var path in MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.csproj"))
+        var files = MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.csproj")


🔧 Please avoid var, unless the type is visible. It's important information to understand the code

dennisdoomen · 2026-10-10T06:55:51Z

+                    content,
+                    context.FalloutVersion,
+                    variables,
+                    isProjectFile: path.ToString().EndsWith(".csproj", StringComparison.OrdinalIgnoreCase)),


🔧🤖 Use path.HasExtension(".csproj") instead of ToString().EndsWith(...); the AbsolutePath helper already exists.

dennisdoomen · 2026-10-10T06:55:52Z

-        content = EnsureFalloutVersionPropertyExists(content, ambiguousVariables, falloutVersionVariable, falloutVersion,
-            ref edits);
+        // Only a file with a redirected reference gets the property. Every other file stays unchanged.
+        if (redirectEdits > 0 && !variables.DefinesFalloutVersion)


🔧🤖 No spec covers FalloutVersion defined in another file; add one that shows it is not inserted again.

dennisdoomen · 2026-10-10T06:55:53Z

+        string content, string falloutVersionVariable, string falloutVersion, ref int edits)
    {
-        if (ambiguousVariables.Count == 0 || content.Contains($"<{falloutVersionVariable}>"))
+        if (content.Contains($"<{falloutVersionVariable}>"))


♻️🤖 Drop this Contains guard: DefinesFalloutVersion already covers it, so it can never be true here.

dennisdoomen · 2026-10-10T07:05:23Z

+    // produce NU1603 ("not found, falling back to next-higher") which `WarningsAsErrors` in the
+    // migrated project escalates. Pinning in the same pass avoids a broken post-migrate build
+    // (#217). MSBuild variables (`$(...)`) are handled by HandleMsBuildVariable below.
+    private static readonly Regex packageItemPattern = new(


🔧 Use <summary> so in-line docs in the IDE work

dennisdoomen · 2026-10-10T07:06:15Z

-        // Pass 2 — namespace-only rewrites for anything Pass 1 didn't consume (CPM-managed
-        // PackageReferences without inline Version, ProjectReferences, MSBuild properties).
-        content = packageReferencePattern.Replace(content, _ =>
+        // Pass 2 — namespace-only rewrites for anything Pass 1 didn't consume (other item types,


🤔 What do you mean with "namespace"?

dennisdoomen · 2026-10-10T07:06:51Z

            return "Fallout.";
        });

        content = msBuildPropertyPattern.Replace(content, _ =>


🤔 Isn't this a separate pass?

dennisdoomen · 2026-10-10T07:08:18Z

+        }

-        return HandleMsBuildVariable(falloutVersion, content, edits);
+        return HandleMsBuildVariable(falloutVersion, content, edits, variables);


🔧 If you decide to wrap one of the passes into a separate methode, do it for all. It's already quite hard to follow this entire class.

dennisdoomen · 2026-10-10T07:08:42Z

-    // shared with non-Fallout packages via a dedicated $(FalloutVersion) property, and bump every
-    // variable that's now exclusively Fallout's to the current Fallout version.
-    private static RewriteResult HandleMsBuildVariable(string falloutVersion, string content, int edits)
+    // Pass 5 — decouple the variables ambiguously shared with non-Fallout packages via a dedicated


❓ What does "decouple" mean?

ANcpLua and others added 2 commits October 3, 2026 22:06

Rewrite central package versions and version properties in .props fil…

b1e8863

…es in fallout-migrate Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

ANcpLua marked this pull request as ready for review October 6, 2026 21:56

ANcpLua requested a review from a team as a code owner October 6, 2026 21:56

Copilot AI balanced review requested due to automatic review settings October 6, 2026 21:56

Copilot AI reviewed Oct 6, 2026

ChrisonSimtian assigned ChrisonSimtian and ANcpLua Oct 8, 2026

ChrisonSimtian added the needs-triage label Oct 8, 2026

ChrisonSimtian requested changes Oct 8, 2026

View reviewed changes

ChrisonSimtian self-requested a review October 8, 2026 09:21

dennisdoomen requested changes Oct 10, 2026

View reviewed changes

dennisdoomen mentioned this pull request Oct 10, 2026

Auto-remove manual NuGet.* version pins on future major versions #666

Open

Conversation

ANcpLua commented Oct 3, 2026 • 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

Uh oh!

ChrisonSimtian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ANcpLua commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dennisdoomen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ANcpLua commented Oct 3, 2026 •

edited

Loading

ANcpLua commented Oct 8, 2026 •

edited

Loading