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

Rewrite central package versions and version properties in .props files, so a repository with central package management (Directory.Packages.props) restores after fallout-migrate.

What changed

RewriteCsprojsStep rewrites *.props files as well as *.csproj files.
Package items (PackageReference, PackageVersion, PackageDownload) go from Nuke.X to Fallout.X. A literal Version or VersionOverride is pinned to the current Fallout version, in either attribute order. Version ranges stay as written. PackageDownload keeps its [exact] brackets. An item that is already Fallout.X keeps its version.
The step checks version variables across all files before it rewrites any file. A variable used only by Nuke.* packages is bumped in the file that defines it, such as an imported Version.props. <NukeVersion> becomes <FalloutVersion>.
A variable shared with an unrelated package keeps its value. Only the Fallout references are redirected to $(FalloutVersion). That property is added to a file with a redirected reference, and only when no file defines it yet.
The System.Security.Cryptography.Xml pin is still removed from *.csproj files only. A pin in a *.props file applies to every project, so it stays.
A file that can't be read gets one warning and is skipped. Errors while rewriting or writing still throw.
The regexes live in MsBuildPatterns.cs and the variable check in VersionVariables.cs. Each pass of the step is a named method.
The migration step skill and docs/Migration/from-nuke.md mention *.props files and central package versions.

This builds on #478 (version variable handling) instead of replacing it.

Verification

./build.sh Test on 167947c: all 68 Fallout.Migrate.Specs pass.
13 new specs in RewriteCsprojsStepSpecs and 2 in MigrationFileOperationsSpecs.

Closes #492 (NU1010 restore failure in central package management repositories after migration)

🤖 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.

…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

Done in 276ae8a. The shared regexes live in MsBuildPatterns.cs now, and every regex has a <summary> and <remarks> in the style of your example.

Follow-up: b4500df writes every pattern as a raw string literal and keeps the group names next to their pattern. Would you rather we replace these regexes with structured XML editing, for example XDocument with LoadOptions.PreserveWhitespace (Fallout.Utilities' ReadXml already loads that way)? Renames and version pins would then work on elements and attributes instead of text, so findings like the attribute-order and version-range ones could not happen. I'd do that as a separate PR after this one, since most of these regexes predate it.

No, don't bother.

…r behavior TryRead reads with File.ReadAllText. AbsolutePath.ReadAllText throws an ArgumentException for a missing path before it reads, so that file was not reported as a warning and the unreadable-file spec failed. The specs say "redirected to FalloutVersion" instead of "decoupled". The new spec lines use explicit types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ANcpLua · 2026-10-12T22:38:02Z

@dennisdoomen @ChrisonSimtian Ready for another look. I replied on each thread.

Changes since your reviews:

dennisdoomen (reviewed b54279c): https://github.com/Fallout-build/Fallout/pull/694/files/b54279ca3ce34132ad861fc3ea17a66db5260200..167947cc5f4646be52a72262c8d8dda748a71310
ChrisonSimtian (reviewed c346a4a): https://github.com/Fallout-build/Fallout/pull/694/files/c346a4ad39004a3d7fd91fbe05f0200f4bd91266..167947cc5f4646be52a72262c8d8dda748a71310. Your six points were fixed in b54279c, but 276ae8a restructured the step after that.

The CI failure on 276ae8a came from my new spec. It is fixed in 167947c, and CI is green.

Could a maintainer add the target/vCurrent and bug labels? I can't set labels on this repo.

Once you're both happy, I'll fold the review commits into the commits they belong to and rebase onto develop.

ANcpLua · 2026-10-13T00:07:28Z

@dennisdoomen @ChrisonSimtian A scope question before your next look.

#686 (fallout-migrate 10.4.0 leftovers) has one open point. When the migration removes the System.Security.Cryptography.Xml reference from a .csproj, it leaves the comment above that reference behind.

The fix belongs in the code this PR moves to MsBuildPatterns.cs: also remove the comment directly above the pin, but only when it mentions Cryptography.Xml, NU1903 or NU1605. Two specs: that comment goes, and an unrelated comment stays.

Should I add it to this PR? Otherwise I'll open a separate PR after this one merges. A separate PR now would conflict with this one.

Every pattern is now a raw string literal, so quotes read as quotes instead of doubled "" escapes (AV1582). The renamed Nuke* properties are a named list instead of one long alternation. The include and variable group names stay next to their pattern behind PackageNameOf and VariableNameOf, and replacing the variable inside a matched tag is the named ReplaceGroupValue helper. The patterns themselves are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dennisdoomen · 2026-10-13T07:43:32Z

Should I add it to this PR? Otherwise I'll open a separate PR after this one merges. A separate PR now would conflict with this one.

Please include it here as well.

dennisdoomen · 2026-10-13T07:44:19Z


-        return (variablesToBump, ambiguousVariables);
+        // Only a file with a redirected reference needs a property, and only if no file defines it.
+        return edits > 0 && !variables.DefinesFalloutVersion


⛏️ DefinesFalloutVersion is computed before any file is rewritten. If several files have a redirected reference and no file defines FalloutVersion yet, each of those files gets its own <FalloutVersion>. For example, Directory.Packages.props and a .csproj that both use a shared variable. The values are the same, so nothing breaks, but the definitions are redundant. Optional: add the property to one file only, for example the first one that needs it.

I kept one property per file that needs it. Your example is safe, because every project imports Directory.Packages.props. But two project files that each need the property would break if only the first one got it. A property in one project file is not visible in another, and this step does not follow MSBuild imports. 8c30f3c adds a comment that says this.

dennisdoomen · 2026-10-13T07:44:19Z

+            content = content.Insert(
                propertyGroupIndex + "<PropertyGroup>".Length,
-                $"\n    <{falloutVersionVariable}>{falloutVersion}</{falloutVersionVariable}>");
+                $"\n    <FalloutVersion>{falloutVersion}</FalloutVersion>");


⛏️ This always inserts \n, here and in the fallback on line 185. In a file with CRLF line endings, the result has mixed line endings. Consider using the file's own line ending, for example content.Contains("\r\n") ? "\r\n" : "\n".

Fixed in 8c30f3c. Both insertions now use the file's own line ending. A spec covers a CRLF file with and without a PropertyGroup.

dennisdoomen · 2026-10-13T07:44:19Z

+    /// Property names and values are case-sensitive.
+    /// </remarks>
+    private static Regex CreatePropertyValuePattern(string variableName) =>
+        new($"""(?<=<{variableName}\s*>)[^<]+(?=</{variableName}\s*>)""");


⛏️ variableName comes from the file content and goes into the pattern without Regex.Escape. MSBuild property names are normally safe, but escaping it costs nothing and protects against unexpected input.

Fixed in 8c30f3c. The name is escaped now. A spec uses $(Odd[Name), which crashed the run before.

dennisdoomen · 2026-10-13T07:44:19Z

+    /// dependency version to win. A pin in a shared props file is kept because it affects other projects.
+    /// </remarks>
+    public static readonly Regex CryptographyXmlPackageReference = new(
+        """^[ \t]*<PackageReference\s+Include="System\.Security\.Cryptography\.Xml"[^/]*/>[ \t]*\r?\n?""",


⛏️ This pattern still requires Include as the first attribute, so <PackageReference Version="..." Include="System.Security.Cryptography.Xml" /> is not removed. The remarks document this, and it was the same before this PR. Optional, since the other patterns now accept either attribute order. Could also go in the follow-up for #686.

Fixed here in 8c30f3c. The pattern now finds Include in any position, and a spec covers Version before Include.

ChrisonSimtian · 2026-10-13T18:58:30Z

Two specs: that comment goes, and an unrelated comment stays

iirc then we did this to leave behind a breadcrumb of what we did, so Id say keep it

I'll look at the rest once im in front of a screen 😊

…ibute order The inserted FalloutVersion property now uses the file's own line ending, so a CRLF file no longer gets mixed line endings. A version variable name comes from the file, so it is escaped before it goes into the property pattern. A name such as Odd[Name crashed the run before. The System.Security.Cryptography.Xml pin is now removed when Version comes before Include, like the other package patterns. A comment says why each file with a redirected reference gets its own FalloutVersion property. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ANcpLua · 2026-10-14T17:42:53Z

@dennisdoomen @ChrisonSimtian Changes since your last review: https://github.com/Fallout-build/Fallout/pull/694/files/b4500df807dacc16ea07d8cdc9c8f41c024b23a0..8c30f3c0a7f7360ad9b09e6dfc4db5a69dd8f3cc. I replied on each thread.

About the comment above the removed System.Security.Cryptography.Xml pin (#686, the fallout-migrate 10.4.0 leftovers): you answered differently. dennisdoomen asked me to include the fix here. ChrisonSimtian said the comment is a breadcrumb and should stay. I left the comment as it is. If you both want it removed, I'll add the fix to this PR.

CI: all tests pass on 8c30f3c. One build run failed in PackageGuard, because github.com returned 503 for a license URL. Could a maintainer re-run it? I can't.

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

ChrisonSimtian assigned dennisdoomen Oct 11, 2026

ANcpLua and others added 2 commits October 12, 2026 12:46

Clarify MSBuild migration passes and cover imported version properties

276ae8a

dennisdoomen added bug Something isn't working target/vCurrent Targets the current version and removed needs-triage labels Oct 13, 2026

dennisdoomen reviewed Oct 13, 2026

View reviewed changes

ChrisonSimtian mentioned this pull request Oct 15, 2026

Move the AI skills to Fallout.Workbench and slim AGENTS.md #712

Draft

Conversation

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

Uh oh!

What changed

Verification

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ANcpLua commented Oct 12, 2026

Uh oh!

ANcpLua commented Oct 13, 2026

Uh oh!

dennisdoomen commented Oct 13, 2026

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!

ChrisonSimtian commented Oct 13, 2026

Uh oh!

ANcpLua commented Oct 14, 2026

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