From b1e8863e99f00b4b36e65ec9594de87b1e9d4eb0 Mon Sep 17 00:00:00 2001 From: Alexander Nachtmann Date: Sat, 3 Oct 2026 22:06:36 +0200 Subject: [PATCH 1/7] Rewrite central package versions and version properties in .props files in fallout-migrate Co-Authored-By: Claude Fable 5.1 --- .../Steps/RewriteCsprojsStep.cs | 191 ++++++++++------ .../RewriteCsprojsStepSpecs.cs | 215 ++++++++++++++++++ 2 files changed, 338 insertions(+), 68 deletions(-) diff --git a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs index c69d15e77..823591a8f 100644 --- a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs +++ b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs @@ -1,48 +1,61 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Text.RegularExpressions; using System.Threading.Tasks; +using Fallout.Common.IO; using Fallout.Migrate.Common; namespace Fallout.Migrate.Steps; /// -/// Rewrites every *.csproj file under the repository root: Nuke.* package/project -/// references become Fallout.* (pinning the current Fallout version where an inline -/// Version attribute was present), Nuke* MSBuild properties are renamed to -/// Fallout*, and stale explicit System.Security.Cryptography.Xml pins are stripped. +/// Rewrites every *.csproj and *.props file under the repository root: Nuke.* +/// package/project references and central PackageVersion items become Fallout.* +/// (pinning the current Fallout version where a Version or VersionOverride attribute +/// was present), Nuke* MSBuild properties are renamed to Fallout*, version properties +/// are bumped wherever they are defined, and stale explicit +/// System.Security.Cryptography.Xml pins are stripped. /// internal sealed class RewriteCsprojsStep : IMigrationStep { - // Combined rewrite: Nuke.X PackageReference WITH an inline Version attribute → Fallout.X - // at the current Fallout version. NUKE-era pins (e.g. `Version="10.1.0"`) don't exist as - // Fallout.* packages and produce NU1603 ("not found, falling back to next-higher") which - // `WarningsAsErrors` in the migrated project escalates. Bumping in the same pass avoids - // a broken post-migrate build (#217). Tolerates extra attributes between Include and Version - // (e.g. `PrivateAssets="all"`). - // We don't mach MSBuild variables (`$(...)`) here, because they are handled below - private static readonly Regex nukePackageWithInlineVersionPattern = new( - @"(?[A-Z][A-Za-z0-9.]+)(?""[^>]*?\s+Version="")(?!\$\()[^""]+", + // Package items: PackageReference, central PackageVersion (Directory.Packages.props, #492) + // and PackageDownload. 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. NUKE-era pins (e.g. `Version="10.1.0"`) don't exist as Fallout.* packages and + // 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( + @"<(?:PackageReference|PackageVersion|PackageDownload)\b[^>]*>", RegexOptions.Compiled); - // PackageReference / ProjectReference `Include="Nuke.X"` → `Include="Fallout.X"` — namespace - // only. Catches references that DON'T have an inline Version (central package management). - // Must run AFTER NukePackageWithInlineVersionPattern so it only touches what's left. + private static readonly Regex nukeIncludePattern = + new(@"(?<=\b(?:Include|Update)="")Nuke\.(?=[A-Z])", RegexOptions.Compiled); + + // 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( + @"(?<=\bVersion(?:Override)?=""\[?)(?!\$\()[^""\[\]]+(?=\]?"")", + RegexOptions.Compiled); + + // ProjectReference / Remove `Include="Nuke.X"` → `Include="Fallout.X"` — namespace only. + // Must run AFTER the package-item pass so it only touches what's left. private static readonly Regex packageReferencePattern = new(@"(?<=\b(?:Include|Update|Remove)="")Nuke\.(?=[A-Z])", RegexOptions.Compiled); - // Detects MSBuild variables used by already-rewritten Fallout.* PackageReferences: - // Version="$(MyVar)". Scoped to Fallout.* so variables shared with unrelated packages - // aren't mistaken for Fallout version variables. + // Detects MSBuild variables used by Nuke.*/Fallout.* PackageReferences and central + // PackageVersions: Version="$(MyVar)". Matches both spellings because the files are + // classified before they are rewritten. Scoped to those packages so variables shared with + // unrelated packages aren't mistaken for Fallout version variables. private static readonly Regex falloutPackageReferenceVariablePattern = new( - @"]*?Version=""\$\((?[^)]+)\)""", + @"<(?:PackageReference|PackageVersion)\s+Include=""(?:Nuke|Fallout)\.[^""]+""[^>]*?Version=""\$\((?[^)]+)\)""", RegexOptions.Compiled); - // Same variable-usage detection, but for PackageReferences that are NOT Fallout.* — used to + // Same variable-usage detection, but for items that are NOT Nuke.*/Fallout.* — used to // detect a variable ambiguously shared between a Fallout package and an unrelated one. private static readonly Regex nonFalloutPackageReferenceVariablePattern = new( - @"]*?Version=""\$\((?[^)]+)\)""", + @"<(?:PackageReference|PackageVersion)\s+Include=""(?!(?:Nuke|Fallout)\.)[^""]+""[^>]*?Version=""\$\((?[^)]+)\)""", RegexOptions.Compiled); // MSBuild element/property names that begin with `Nuke` followed by an uppercase @@ -78,42 +91,70 @@ internal sealed class RewriteCsprojsStep : IMigrationStep /// public Task ExecuteAsync(MigrationContext context, Summary summary) { - foreach (var path in MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.csproj")) + var files = MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.csproj") + .Concat(MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.props")) + .ToList(); + + // A version variable can be used in one file and defined in another, for example + // `$(NukeVersion)` in Directory.Packages.props and `` in Version.props (#492). + // So the variables are classified over all files first, then every file is rewritten. + var variables = VersionVariables.Collect(files.Select(ReadOrEmpty)); + + foreach (var path in files) { MigrationFileOperations.ApplyRewrite( context, path, - content => Rewrite(content, context.FalloutVersion), + content => Rewrite(content, context.FalloutVersion, variables), summary); } return Task.CompletedTask; + + static string ReadOrEmpty(AbsolutePath path) + { + // An unreadable file is reported by ApplyRewrite; it contributes nothing here. + try + { + return path.ReadAllText(); + } + catch (IOException) + { + return string.Empty; + } + } } /// /// Rewrites content, replacing Nuke.* references and MSBuild /// properties with their Fallout.* equivalents and stripping stale pins. /// - /// The original .csproj file content. - /// The Fallout version to pin into rewritten inline-versioned references. + /// The original .csproj or .props file content. + /// The Fallout version to pin into rewritten versioned references. + /// The version variables classified over all files. /// The rewritten content and the number of edits made. - private static RewriteResult Rewrite(string original, string falloutVersion) + private static RewriteResult Rewrite(string original, string falloutVersion, VersionVariables variables) { var edits = 0; var content = original; - // Pass 1 — combined Include + Version rewrite for Nuke.X PackageReferences with inline Version. - content = nukePackageWithInlineVersionPattern.Replace(content, m => + // Pass 1 — package items: rename a Nuke.X Include and pin its literal Version/VersionOverride. + // One edit per item, however many attributes changed. An item that is already Fallout.X + // keeps its pin. + content = packageItemPattern.Replace(content, m => { + var item = nukeIncludePattern.Replace(m.Value, "Fallout."); + if (item == m.Value) + { + return m.Value; + } + edits++; - return m.Groups["prefix"].Value - + "Fallout." + m.Groups["name"].Value - + m.Groups["between"].Value - + falloutVersion; + return literalVersionPattern.Replace(item, _ => falloutVersion); }); - // Pass 2 — namespace-only rewrites for anything Pass 1 didn't consume (CPM-managed - // PackageReferences without inline Version, ProjectReferences, MSBuild properties). + // Pass 2 — namespace-only rewrites for anything Pass 1 didn't consume (ProjectReferences, + // Remove items, MSBuild properties). content = packageReferencePattern.Replace(content, _ => { edits++; @@ -140,57 +181,71 @@ private static RewriteResult Rewrite(string original, string falloutVersion) return string.Empty; }); - return HandleMsBuildVariable(falloutVersion, content, edits); + return HandleMsBuildVariable(falloutVersion, content, edits, variables); } - // Pass 5 — extract variables used by Fallout.* PackageReferences, decouple the ones ambiguously - // 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 + // $(FalloutVersion) property, and bump every variable that's exclusively Fallout's to the + // current Fallout version, in whichever file defines it. + private static RewriteResult HandleMsBuildVariable(string falloutVersion, string content, int edits, VersionVariables variables) { - const string falloutVersionVariable = "FalloutVersion"; - - var (variablesToBump, ambiguousVariables) = - ClassifyPackageReferenceVariables(content, falloutVersionVariable); - (content, int redirectEdits) = - RedirectAmbiguousVariablesToFalloutVersion(content, ambiguousVariables, falloutVersionVariable); + RedirectAmbiguousVariablesToFalloutVersion(content, variables.Ambiguous, VersionVariables.FalloutVersionVariable); edits += redirectEdits; - content = EnsureFalloutVersionPropertyExists(content, ambiguousVariables, falloutVersionVariable, falloutVersion, - ref edits); + if (!variables.DefinesFalloutVersion) + { + content = EnsureFalloutVersionPropertyExists(content, variables.Ambiguous, VersionVariables.FalloutVersionVariable, falloutVersion, + ref edits); + } - (content, int bumpEdits) = BumpVariableProperties(content, variablesToBump, falloutVersion); + (content, int bumpEdits) = BumpVariableProperties(content, variables.ToBump, falloutVersion); edits += bumpEdits; return new RewriteResult(content, edits); } - // 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. - private static (HashSet variablesToBump, HashSet ambiguousVariables) ClassifyPackageReferenceVariables( - string content, string falloutVersionVariable) + /// + /// The version variables found in the files of one migration run: those used only by Nuke.*/Fallout.* + /// packages (bumped), those also shared with an unrelated package (ambiguous), and whether any + /// file already defines the dedicated FalloutVersion property. + /// + private sealed class VersionVariables { - var nonFalloutVariables = nonFalloutPackageReferenceVariablePattern.Matches(content) - .Select(m => m.Groups["variable"].Value) - .ToHashSet(); + public const string FalloutVersionVariable = "FalloutVersion"; - var variablesToBump = new HashSet - { - falloutVersionVariable - }; + // The property is renamed from NukeVersion by msBuildPropertyPattern, so both spellings count. + private static readonly Regex falloutVersionPropertyPattern = + new(@"<(?:Nuke|Fallout)Version\s*>", RegexOptions.Compiled); - var ambiguousVariables = new HashSet(); + public HashSet ToBump { get; } = [FalloutVersionVariable]; + public HashSet Ambiguous { get; } = []; + public bool DefinesFalloutVersion { get; private set; } - foreach (var variable in falloutPackageReferenceVariablePattern.Matches(content) - .Select(m => m.Groups["variable"].Value)) + // 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 contents) { - (nonFalloutVariables.Contains(variable) ? ambiguousVariables : variablesToBump).Add(variable); - } + var result = new VersionVariables(); + var falloutVariables = new HashSet(); + var nonFalloutVariables = new HashSet(); + + foreach (var content in contents) + { + falloutVariables.UnionWith(falloutPackageReferenceVariablePattern.Matches(content).Select(m => m.Groups["variable"].Value)); + nonFalloutVariables.UnionWith(nonFalloutPackageReferenceVariablePattern.Matches(content).Select(m => m.Groups["variable"].Value)); + result.DefinesFalloutVersion |= falloutVersionPropertyPattern.IsMatch(content); + } - return (variablesToBump, ambiguousVariables); + foreach (var variable in falloutVariables) + { + (nonFalloutVariables.Contains(variable) ? result.Ambiguous : result.ToBump).Add(variable); + } + + return result; + } } private static (string content, int edits) RedirectAmbiguousVariablesToFalloutVersion( @@ -204,7 +259,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( - $@"(]*?Version="")\$\({Regex.Escape(variable)}\)"); + $@"(<(?:PackageReference|PackageVersion)\s+Include=""Fallout\.[^""]+""[^>]*?Version="")\$\({Regex.Escape(variable)}\)"); content = redirectPattern.Replace(content, m => { diff --git a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs index fe1fa19a0..5f9807cbe 100644 --- a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs +++ b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs @@ -480,4 +480,219 @@ public async Task Decouples_ambiguously_used_variables_when_a_property_group_but .And.Contain("$(FalloutVersion)", Exactly.Once()) .And.Contain("$(PkgVersion)"); } + + [Fact] + public async Task Central_package_versions_are_renamed_and_pinned() + { + // Arrange + (tempDirectory / "Directory.Packages.props").WriteAllText(""" + + + + + + + + """, eofLineBreak: false); + (tempDirectory / "build" / "_build.csproj").WriteAllText(""" + + + + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + summary.EditCount.Should().Be(3); + (tempDirectory / "Directory.Packages.props").ReadAllText().Should().Be(""" + + + + + + + + """); + (tempDirectory / "build" / "_build.csproj").ReadAllText().Should().Contain(""""""); + } + + [Fact] + public async Task Version_property_in_an_imported_props_file_is_renamed_and_bumped() + { + // Arrange + (tempDirectory / "Directory.Packages.props").WriteAllText(""" + + + + + + + """, eofLineBreak: false); + (tempDirectory / "Version.props").WriteAllText(""" + + + 10.1.0 + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + (tempDirectory / "Directory.Packages.props").ReadAllText() + .Should().Contain(""""""); + (tempDirectory / "Version.props").ReadAllText().Should().Contain("11.0.0"); + } + + [Fact] + public async Task Version_variable_defined_in_another_file_is_bumped() + { + // Arrange + (tempDirectory / "Directory.Packages.props").WriteAllText(""" + + + + + + """, eofLineBreak: false); + (tempDirectory / "Version.props").WriteAllText(""" + + + 10.1.0 + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + (tempDirectory / "Version.props").ReadAllText().Should().Contain("11.0.0"); + } + + [Fact] + public async Task Version_variable_shared_with_another_package_in_another_file_is_decoupled() + { + // Arrange + (tempDirectory / "Directory.Packages.props").WriteAllText(""" + + + true + + + + + + """, eofLineBreak: false); + (tempDirectory / "build" / "_build.csproj").WriteAllText(""" + + + + + + """, eofLineBreak: false); + (tempDirectory / "Version.props").WriteAllText(""" + + + 4.0.0 + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + var packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); + packages.Should().Contain(""""""); + packages.Should().Contain("11.0.0"); + (tempDirectory / "Version.props").ReadAllText().Should().Contain("4.0.0"); + } + + [Fact] + public async Task Stale_version_override_is_pinned_to_the_current_fallout_version() + { + // Arrange + (tempDirectory / "build" / "_build.csproj").WriteAllText(""" + + + + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + summary.EditCount.Should().Be(1); + (tempDirectory / "build" / "_build.csproj").ReadAllText() + .Should().Contain(""""""); + } + + [Fact] + public async Task Version_attribute_before_include_is_also_pinned() + { + // Arrange + (tempDirectory / "build" / "_build.csproj").WriteAllText(""" + + + + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + summary.EditCount.Should().Be(1); + (tempDirectory / "build" / "_build.csproj").ReadAllText() + .Should().Contain(""""""); + } + + [Fact] + public async Task Package_download_keeps_its_exact_version_brackets() + { + // Arrange + (tempDirectory / "build" / "_build.csproj").WriteAllText(""" + + + + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + (tempDirectory / "build" / "_build.csproj").ReadAllText() + .Should().Contain(""""""); + } + + [Fact] + public async Task Already_migrated_package_pins_are_kept() + { + // Arrange + const string input = """ + + + + + + """; + (tempDirectory / "Directory.Packages.props").WriteAllText(input, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + summary.EditCount.Should().Be(0); + (tempDirectory / "Directory.Packages.props").ReadAllText().Should().Be(input); + } } From c346a4ad39004a3d7fd91fbe05f0200f4bd91266 Mon Sep 17 00:00:00 2001 From: Alexander Nachtmann Date: Tue, 6 Oct 2026 23:56:19 +0200 Subject: [PATCH 2/7] Strip the Cryptography.Xml pin from project files only in fallout-migrate 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 --- .../skills/adding-a-migration-step/SKILL.md | 4 +-- docs/Migration/from-nuke.md | 2 +- .../Steps/RewriteCsprojsStep.cs | 27 ++++++++++++------- .../RewriteCsprojsStepSpecs.cs | 24 +++++++++++++++++ 4 files changed, 45 insertions(+), 12 deletions(-) diff --git a/.agents/skills/adding-a-migration-step/SKILL.md b/.agents/skills/adding-a-migration-step/SKILL.md index 85a2e7307..a8f14eb9a 100644 --- a/.agents/skills/adding-a-migration-step/SKILL.md +++ b/.agents/skills/adding-a-migration-step/SKILL.md @@ -6,12 +6,12 @@ description: How to make fallout-migrate handle a new rename, removal, or rewrit `fallout-migrate` runs a fixed, ordered list of `IMigrationStep` implementations, built in `src/Fallout.Migrate/Migration.cs`. A **step is one operation over one set of files**, and it owns the rewrite rules for those files. -`RewriteCsprojsStep` holds the rules for `*.csproj`; `RewriteCsFilesStep` holds +`RewriteCsprojsStep` holds the rules for `*.csproj` and `*.props`; `RewriteCsFilesStep` holds the rules for `*.cs`. ## Adding a new rename, removal, or rewrite -1. Find the step for that file type — `RewriteCsprojsStep` for `*.csproj`, +1. Find the step for that file type — `RewriteCsprojsStep` for `*.csproj` and `*.props`, `RewriteCsFilesStep` for `*.cs`, `RewriteBootstrapScriptsStep` for the bootstrap scripts. 2. Add a `private static readonly Regex` field to that step, with a comment diff --git a/docs/Migration/from-nuke.md b/docs/Migration/from-nuke.md index 934bb0bf4..0289df2ab 100644 --- a/docs/Migration/from-nuke.md +++ b/docs/Migration/from-nuke.md @@ -15,7 +15,7 @@ Done. Read on if anything looks unusual, or you want to know what the tool did. ## The recommended path: `fallout-migrate` -`fallout-migrate` is a global `dotnet` tool that performs the entire migration in one command. It rewrites `PackageReference`s, `using` directives, MSBuild properties, bootstrap scripts, env vars, and renames `.nuke/` → `.fallout/`. It's idempotent and has a `--dry-run` mode. +`fallout-migrate` is a global `dotnet` tool that performs the entire migration in one command. It rewrites `PackageReference`s and central package versions (`Directory.Packages.props`), `using` directives, MSBuild properties, bootstrap scripts, env vars, and renames `.nuke/` → `.fallout/`. It's idempotent and has a `--dry-run` mode. ### Step 1 — install the tool diff --git a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs index 823591a8f..a4064fdc2 100644 --- a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs +++ b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs @@ -15,7 +15,7 @@ namespace Fallout.Migrate.Steps; /// (pinning the current Fallout version where a Version or VersionOverride attribute /// was present), Nuke* MSBuild properties are renamed to Fallout*, version properties /// are bumped wherever they are defined, and stale explicit -/// System.Security.Cryptography.Xml pins are stripped. +/// System.Security.Cryptography.Xml pins are stripped from *.csproj files. /// internal sealed class RewriteCsprojsStep : IMigrationStep { @@ -83,7 +83,8 @@ internal sealed class RewriteCsprojsStep : IMigrationStep // requires a newer version (10.0.6+) and the conflict trips NU1605 ("Detected package // downgrade"). Removing the explicit pin lets the transitive version win, which is what the // migrated project wants (#217). Matches a self-closing element with optional surrounding - // indentation + trailing newline. + // indentation + trailing newline. Applied to *.csproj only: a pin in a *.props file such as + // a root Directory.Build.props applies to every project in the repository, not just the build. private static readonly Regex cryptographyXmlPackageRefPattern = new( @"^[ \t]*[ \t]*\r?\n?", RegexOptions.Compiled | RegexOptions.Multiline); @@ -105,7 +106,11 @@ public Task ExecuteAsync(MigrationContext context, Summary summary) MigrationFileOperations.ApplyRewrite( context, path, - content => Rewrite(content, context.FalloutVersion, variables), + content => Rewrite( + content, + context.FalloutVersion, + variables, + isProjectFile: path.ToString().EndsWith(".csproj", StringComparison.OrdinalIgnoreCase)), summary); } @@ -132,8 +137,9 @@ static string ReadOrEmpty(AbsolutePath path) /// The original .csproj or .props file content. /// The Fallout version to pin into rewritten versioned references. /// The version variables classified over all files. + /// true for a .csproj, false for a .props file. /// The rewritten content and the number of edits made. - private static RewriteResult Rewrite(string original, string falloutVersion, VersionVariables variables) + private static RewriteResult Rewrite(string original, string falloutVersion, VersionVariables variables, bool isProjectFile) { var edits = 0; var content = original; @@ -174,12 +180,15 @@ private static RewriteResult Rewrite(string original, string falloutVersion, Ver return string.Empty; }); - // Pass 4 — strip the stale System.Security.Cryptography.Xml direct pin. - content = cryptographyXmlPackageRefPattern.Replace(content, _ => + // Pass 4 — strip the stale System.Security.Cryptography.Xml direct pin (project files only). + if (isProjectFile) { - edits++; - return string.Empty; - }); + content = cryptographyXmlPackageRefPattern.Replace(content, _ => + { + edits++; + return string.Empty; + }); + } return HandleMsBuildVariable(falloutVersion, content, edits, variables); } diff --git a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs index 5f9807cbe..68659c757 100644 --- a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs +++ b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs @@ -220,6 +220,30 @@ public async Task Conflicting_system_security_cryptography_xml_pin_is_stripped() buildCsproj.Should().Contain(@"Include=""Fallout.Common"""); } + [Fact] + public async Task Cryptography_xml_pin_in_a_props_file_is_kept() + { + // A pin in a root Directory.Build.props applies to every project in the repository, + // so only the pin in a project file is stripped. + // Arrange + const string input = """ + + + + + + """; + + (tempDirectory / "Directory.Build.props").WriteAllText(input, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + summary.EditCount.Should().Be(0); + (tempDirectory / "Directory.Build.props").ReadAllText().Should().Be(input); + } + [Fact] public async Task Other_system_packages_are_left_alone() { From b54279ca3ce34132ad861fc3ea17a66db5260200 Mon Sep 17 00:00:00 2001 From: Alexander Nachtmann Date: Thu, 8 Oct 2026 09:49:59 +0200 Subject: [PATCH 3/7] Add FalloutVersion only to files with a redirected reference in fallout-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 --- .../Common/MigrationFileOperations.cs | 2 +- .../Steps/RewriteCsprojsStep.cs | 95 ++++++++++--------- .../RewriteCsprojsStepSpecs.cs | 92 ++++++++++++++++++ 3 files changed, 143 insertions(+), 46 deletions(-) diff --git a/src/Fallout.Migrate/Common/MigrationFileOperations.cs b/src/Fallout.Migrate/Common/MigrationFileOperations.cs index a5ba5e44b..3311b7159 100644 --- a/src/Fallout.Migrate/Common/MigrationFileOperations.cs +++ b/src/Fallout.Migrate/Common/MigrationFileOperations.cs @@ -50,7 +50,7 @@ public static void ApplyRewrite( { original = path.ReadAllText(); } - catch (IOException ex) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { summary.Warnings.Add($"could not read {RelativePath(context.RootDirectory, path)}: {ex.Message}"); return; diff --git a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs index a4064fdc2..6be7c5ab5 100644 --- a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs +++ b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs @@ -30,32 +30,25 @@ internal sealed class RewriteCsprojsStep : IMigrationStep @"<(?:PackageReference|PackageVersion|PackageDownload)\b[^>]*>", RegexOptions.Compiled); - private static readonly Regex nukeIncludePattern = - new(@"(?<=\b(?:Include|Update)="")Nuke\.(?=[A-Z])", RegexOptions.Compiled); + // The `Nuke.` prefix of an Include, Update or Remove value: `Include="Nuke.X"` → `Include="Fallout.X"`. + // Pass 1 applies it to package items. Pass 2 applies it to every item that pass 1 didn't touch. + private static readonly Regex nukeItemNamePattern = + new(@"(?<=\b(?:Include|Update|Remove)="")Nuke\.(?=[A-Z])", RegexOptions.Compiled); // The literal value of a Version or VersionOverride attribute. PackageDownload needs an exact - // range (`[10.1.0]`), so the brackets stay outside the match. + // range (`[10.1.0]`), so the brackets stay outside the match. A version range such as + // `[10.1.0,)` or `(10.1.0,11.0.0)` doesn't match and is kept as written: replacing only + // one bound would leave an invalid range. private static readonly Regex literalVersionPattern = new( - @"(?<=\bVersion(?:Override)?=""\[?)(?!\$\()[^""\[\]]+(?=\]?"")", - RegexOptions.Compiled); - - // ProjectReference / Remove `Include="Nuke.X"` → `Include="Fallout.X"` — namespace only. - // Must run AFTER the package-item pass so it only touches what's left. - private static readonly Regex packageReferencePattern = - new(@"(?<=\b(?:Include|Update|Remove)="")Nuke\.(?=[A-Z])", RegexOptions.Compiled); - - // Detects MSBuild variables used by Nuke.*/Fallout.* PackageReferences and central - // PackageVersions: Version="$(MyVar)". Matches both spellings because the files are - // classified before they are rewritten. Scoped to those packages so variables shared with - // unrelated packages aren't mistaken for Fallout version variables. - private static readonly Regex falloutPackageReferenceVariablePattern = new( - @"<(?:PackageReference|PackageVersion)\s+Include=""(?:Nuke|Fallout)\.[^""]+""[^>]*?Version=""\$\((?[^)]+)\)""", + @"(?<=\bVersion(?:Override)?=""\[?)(?!\$\()[^""\[\],()]+(?=\]?"")", RegexOptions.Compiled); - // Same variable-usage detection, but for items that are NOT Nuke.*/Fallout.* — used to - // detect a variable ambiguously shared between a Fallout package and an unrelated one. - private static readonly Regex nonFalloutPackageReferenceVariablePattern = new( - @"<(?:PackageReference|PackageVersion)\s+Include=""(?!(?:Nuke|Fallout)\.)[^""]+""[^>]*?Version=""\$\((?[^)]+)\)""", + // A PackageReference or central PackageVersion whose version is an MSBuild variable: + // Version="$(MyVar)". The Include and Version attributes can come in either order. + // Used to classify the variables (both Nuke.* and Fallout.* spellings count, because the files + // are classified before they are rewritten) and to redirect an ambiguous one. + private static readonly Regex variableVersionItemPattern = new( + @"<(?:PackageReference|PackageVersion)\b(?=[^>]*\bInclude=""(?[^""]+)"")(?=[^>]*\bVersion=""\$\((?[^)]+)\)"")[^>]*>", RegexOptions.Compiled); // MSBuild element/property names that begin with `Nuke` followed by an uppercase @@ -123,7 +116,7 @@ static string ReadOrEmpty(AbsolutePath path) { return path.ReadAllText(); } - catch (IOException) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { return string.Empty; } @@ -149,7 +142,7 @@ private static RewriteResult Rewrite(string original, string falloutVersion, Ver // keeps its pin. content = packageItemPattern.Replace(content, m => { - var item = nukeIncludePattern.Replace(m.Value, "Fallout."); + var item = nukeItemNamePattern.Replace(m.Value, "Fallout."); if (item == m.Value) { return m.Value; @@ -159,9 +152,9 @@ private static RewriteResult Rewrite(string original, string falloutVersion, Ver return literalVersionPattern.Replace(item, _ => falloutVersion); }); - // Pass 2 — namespace-only rewrites for anything Pass 1 didn't consume (ProjectReferences, - // Remove items, MSBuild properties). - content = packageReferencePattern.Replace(content, _ => + // Pass 2 — namespace-only rewrites for anything Pass 1 didn't consume (other item types, + // MSBuild properties). + content = nukeItemNamePattern.Replace(content, _ => { edits++; return "Fallout."; @@ -203,10 +196,10 @@ private static RewriteResult HandleMsBuildVariable(string falloutVersion, string edits += redirectEdits; - if (!variables.DefinesFalloutVersion) + // Only a file with a redirected reference gets the property. Every other file stays unchanged. + if (redirectEdits > 0 && !variables.DefinesFalloutVersion) { - content = EnsureFalloutVersionPropertyExists(content, variables.Ambiguous, VersionVariables.FalloutVersionVariable, falloutVersion, - ref edits); + content = EnsureFalloutVersionPropertyExists(content, VersionVariables.FalloutVersionVariable, falloutVersion, ref edits); } (content, int bumpEdits) = BumpVariableProperties(content, variables.ToBump, falloutVersion); @@ -235,6 +228,11 @@ private sealed class VersionVariables // 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. + // + // Ambiguity is decided by variable name over all files. A per-project property used by a + // Fallout package in project A and by an unrelated package in project B is ambiguous, so + // A is decoupled even though A's own definition isn't shared. The result is still correct, + // it only has more edits than needed. public static VersionVariables Collect(IEnumerable contents) { var result = new VersionVariables(); @@ -243,8 +241,14 @@ public static VersionVariables Collect(IEnumerable contents) foreach (var content in contents) { - falloutVariables.UnionWith(falloutPackageReferenceVariablePattern.Matches(content).Select(m => m.Groups["variable"].Value)); - nonFalloutVariables.UnionWith(nonFalloutPackageReferenceVariablePattern.Matches(content).Select(m => m.Groups["variable"].Value)); + foreach (Match match in variableVersionItemPattern.Matches(content)) + { + var include = match.Groups["include"].Value; + var isFalloutPackage = include.StartsWith("Nuke.", StringComparison.Ordinal) || + include.StartsWith("Fallout.", StringComparison.Ordinal); + (isFalloutPackage ? falloutVariables : nonFalloutVariables).Add(match.Groups["variable"].Value); + } + result.DefinesFalloutVersion |= falloutVersionPropertyPattern.IsMatch(content); } @@ -262,28 +266,29 @@ private static (string content, int edits) RedirectAmbiguousVariablesToFalloutVe { var edits = 0; - foreach (var variable in ambiguousVariables) + // Runs after the rename, so only Fallout.* items are redirected. Only the variable name + // inside `$(...)` is swapped. The rest of the item stays as written. + content = variableVersionItemPattern.Replace(content, m => { - // Matches `Version="$(variable)"` on a Fallout.* PackageReference only, capturing - // 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( - $@"(<(?:PackageReference|PackageVersion)\s+Include=""Fallout\.[^""]+""[^>]*?Version="")\$\({Regex.Escape(variable)}\)"); - - content = redirectPattern.Replace(content, m => + var variable = m.Groups["variable"]; + if (!m.Groups["include"].Value.StartsWith("Fallout.", StringComparison.Ordinal) || + !ambiguousVariables.Contains(variable.Value)) { - edits++; - return m.Groups[1].Value + $"$({falloutVersionVariable})"; - }); - } + return m.Value; + } + + edits++; + var offset = variable.Index - m.Index; + return m.Value.Remove(offset, variable.Length).Insert(offset, falloutVersionVariable); + }); return (content, edits); } private static string EnsureFalloutVersionPropertyExists( - string content, HashSet ambiguousVariables, string falloutVersionVariable, string falloutVersion, ref int edits) + string content, string falloutVersionVariable, string falloutVersion, ref int edits) { - if (ambiguousVariables.Count == 0 || content.Contains($"<{falloutVersionVariable}>")) + if (content.Contains($"<{falloutVersionVariable}>")) { return content; } diff --git a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs index 68659c757..44bdade00 100644 --- a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs +++ b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs @@ -637,6 +637,98 @@ public async Task Version_variable_shared_with_another_package_in_another_file_i (tempDirectory / "Version.props").ReadAllText().Should().Contain("4.0.0"); } + [Fact] + public async Task Files_without_a_redirected_reference_are_unchanged_when_a_variable_is_decoupled() + { + // Arrange + (tempDirectory / "Directory.Packages.props").WriteAllText(""" + + + true + + + + + + + """, eofLineBreak: false); + const string versionProps = """ + + + 4.0.0 + + + """; + (tempDirectory / "Version.props").WriteAllText(versionProps, eofLineBreak: false); + const string libraryProject = """ + + + net10.0 + + + """; + (tempDirectory / "src" / "Lib.csproj").WriteAllText(libraryProject, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + var packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); + packages.Should().Contain(""""""); + packages.Should().Contain("11.0.0"); + (tempDirectory / "Version.props").ReadAllText().Should().Be(versionProps); + (tempDirectory / "src" / "Lib.csproj").ReadAllText().Should().Be(libraryProject); + } + + [Fact] + public async Task Version_variable_before_include_is_decoupled() + { + // Arrange + (tempDirectory / "Directory.Packages.props").WriteAllText(""" + + + 4.0.0 + + + + + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + var packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); + packages.Should().Contain(""""""); + packages.Should().Contain(""""""); + packages.Should().Contain("4.0.0"); + packages.Should().Contain("11.0.0"); + } + + [Fact] + public async Task Version_range_is_kept_as_written() + { + // Arrange + (tempDirectory / "build" / "_build.csproj").WriteAllText(""" + + + + + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + var buildCsproj = (tempDirectory / "build" / "_build.csproj").ReadAllText(); + buildCsproj.Should().Contain(""""""); + buildCsproj.Should().Contain(""""""); + } + [Fact] public async Task Stale_version_override_is_pinned_to_the_current_fallout_version() { From 276ae8aa1d202299f3a9c30bda1fef94dcaf26e6 Mon Sep 17 00:00:00 2001 From: Alexander Nachtmann Date: Mon, 12 Oct 2026 12:46:19 +0200 Subject: [PATCH 4/7] Clarify MSBuild migration passes and cover imported version properties --- .../Common/MigrationFileOperations.cs | 38 +- src/Fallout.Migrate/Common/MsBuildPatterns.cs | 102 +++++ .../Common/VersionVariables.cs | 83 ++++ .../Steps/RewriteCsprojsStep.cs | 368 ++++++------------ .../MigrationFileOperationsSpecs.cs | 72 ++++ .../RewriteCsprojsStepSpecs.cs | 62 +++ 6 files changed, 469 insertions(+), 256 deletions(-) create mode 100644 src/Fallout.Migrate/Common/MsBuildPatterns.cs create mode 100644 src/Fallout.Migrate/Common/VersionVariables.cs create mode 100644 tests/Fallout.Migrate.Specs/MigrationFileOperationsSpecs.cs diff --git a/src/Fallout.Migrate/Common/MigrationFileOperations.cs b/src/Fallout.Migrate/Common/MigrationFileOperations.cs index 3311b7159..3bca9fae1 100644 --- a/src/Fallout.Migrate/Common/MigrationFileOperations.cs +++ b/src/Fallout.Migrate/Common/MigrationFileOperations.cs @@ -45,18 +45,48 @@ public static void ApplyRewrite( Func rewriter, Summary summary) { - string original; + if (TryRead(context, path, summary, out string original)) + { + ApplyRewrite(context, path, original, rewriter, summary); + } + } + + /// + /// Reads a file and reports an unreadable file as a warning. + /// + /// + /// Only file access failures are handled here. Exceptions from rewriting or writing a file + /// propagate to the caller. A failed read must be skipped, not classified as an empty file. + /// + public static bool TryRead(MigrationContext context, AbsolutePath path, Summary summary, out string content) + { try { - original = path.ReadAllText(); + content = path.ReadAllText(); + return true; } catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { summary.Warnings.Add($"could not read {RelativePath(context.RootDirectory, path)}: {ex.Message}"); - return; + content = string.Empty; + return false; } + } - var result = rewriter(original); + /// + /// Rewrites previously read content, records the edits, and writes them unless this is a dry run. + /// + /// + /// Use this overload when classification and rewriting must use the same file contents. + /// + public static void ApplyRewrite( + MigrationContext context, + AbsolutePath path, + string original, + Func rewriter, + Summary summary) + { + RewriteResult result = rewriter(original); if (result.EditCount == 0) { return; diff --git a/src/Fallout.Migrate/Common/MsBuildPatterns.cs b/src/Fallout.Migrate/Common/MsBuildPatterns.cs new file mode 100644 index 000000000..f708926fa --- /dev/null +++ b/src/Fallout.Migrate/Common/MsBuildPatterns.cs @@ -0,0 +1,102 @@ +using System.Text.RegularExpressions; + +namespace Fallout.Migrate.Common; + +/// +/// Text patterns used to migrate MSBuild files without changing their XML formatting. +/// +/// +/// These patterns are case-sensitive and match the supported MSBuild spellings, not arbitrary XML. +/// Attribute patterns expect double quotes with no whitespace around the equals sign. +/// +internal static class MsBuildPatterns +{ + /// + /// Finds the opening tag of a NuGet package element: PackageReference, + /// PackageVersion, or PackageDownload. + /// + /// + /// The match is the whole opening tag, including all attributes. It does not capture the package + /// name or version separately. A > inside an attribute value ends the match early. + /// + public static readonly Regex PackageItem = new( + @"<(?:PackageReference|PackageVersion|PackageDownload)\b[^>]*>", + RegexOptions.Compiled); + + /// + /// Finds the Nuke. prefix in an Include, Update, or Remove attribute. + /// + /// + /// Only the prefix is matched, and it must be followed by an uppercase letter. + /// The attribute name and the rest of the item name stay unchanged. + /// + public static readonly Regex NukeItemName = + new(@"(?<=\b(?:Include|Update|Remove)="")Nuke\.(?=[A-Z])", RegexOptions.Compiled); + + /// + /// Finds a literal version in a Version or VersionOverride attribute. + /// + /// + /// Quotes and optional exact-version brackets are outside the match, so [10.1.0] + /// keeps its brackets. MSBuild variable references and ranges containing commas or parentheses + /// are excluded. Replacing only one bound of a range would leave an invalid version. + /// + public static readonly Regex LiteralVersion = new( + @"(?<=\bVersion(?:Override)?=""\[?)(?!\$\()[^""\[\],()]+(?=\]?"")", + RegexOptions.Compiled); + + /// + /// Finds a PackageReference or PackageVersion opening tag whose + /// Version is a single MSBuild variable reference, such as $(ToolsVersion). + /// + /// + /// The whole tag is matched. The include group captures the package name, and the + /// variable group captures the name inside $(...). Attributes may appear in either + /// order. This does not match VersionOverride or PackageDownload. + /// A > inside an attribute value ends the tag match early. + /// + public static readonly Regex VariableVersionItem = new( + @"<(?:PackageReference|PackageVersion)\b(?=[^>]*\bInclude=""(?[^""]+)"")(?=[^>]*\bVersion=""\$\((?[^)]+)\)"")[^>]*>", + RegexOptions.Compiled); + + /// + /// Finds the Nuke prefix of known MSBuild properties that were renamed in Fallout. + /// + /// + /// Only the prefix is matched. The listed property suffixes limit the rename to known names, + /// including both element names and references such as $(NukeVersion). + /// Unrelated user-defined names such as NukeCustomProperty are excluded. + /// + public static readonly Regex NukePropertyName = new( + @"\bNuke(?=" + + "(?:Version|RootDirectory|ScriptDirectory|BaseDirectory|BaseNamespace|" + + "UseNestedNamespaces|RepositoryUrl|UpdateReferences|ContinueOnError|TaskTimeout|" + + "Timeout|TasksEnabled|DefaultExcludes|ExcludeBoot|ExcludeConfig|ExcludeLogs|" + + "ExcludeDirectoryBuild|ExcludeCi|SpecificationFiles|ExternalFiles|TasksAssembly|" + + "TasksDirectory)\\b)", + RegexOptions.Compiled); + + /// + /// Finds a complete NukeTelemetryVersion or FalloutTelemetryVersion property line. + /// + /// + /// The tag group requires matching opening and closing names. Indentation and a trailing + /// newline are included. The value must be on one line. Telemetry was removed from Fallout, + /// so the whole property is deleted rather than renamed. + /// + public static readonly Regex TelemetryVersionProperty = new( + @"^[ \t]*<(?(?:Nuke|Fallout)TelemetryVersion)>.*?>[ \t]*\r?\n?", + RegexOptions.Compiled | RegexOptions.Multiline); + + /// + /// Finds a self-closing System.Security.Cryptography.Xml package reference line. + /// + /// + /// The Include attribute must come first. Indentation and a trailing newline are included. + /// The migration removes this direct pin only from project files, allowing Fallout's transitive + /// dependency version to win. A pin in a shared props file is kept because it affects other projects. + /// + public static readonly Regex CryptographyXmlPackageReference = new( + @"^[ \t]*[ \t]*\r?\n?", + RegexOptions.Compiled | RegexOptions.Multiline); +} diff --git a/src/Fallout.Migrate/Common/VersionVariables.cs b/src/Fallout.Migrate/Common/VersionVariables.cs new file mode 100644 index 000000000..0be6d41d6 --- /dev/null +++ b/src/Fallout.Migrate/Common/VersionVariables.cs @@ -0,0 +1,83 @@ +using System; +using System.Collections.Generic; +using System.Text.RegularExpressions; + +namespace Fallout.Migrate.Common; + +/// +/// The version variable names found in the files of one migration run: those used only by +/// Nuke.*/Fallout.* packages and those shared with unrelated packages. +/// +internal sealed class VersionVariables +{ + public const string FalloutVersionVariable = "FalloutVersion"; + + /// + /// Finds an opening NukeVersion or FalloutVersion property tag. + /// + /// + /// Both spellings count because the rewrite renames NukeVersion to FalloutVersion. + /// The match is case-sensitive and permits whitespace before >, but no attributes. + /// + private static readonly Regex falloutVersionPropertyPattern = + new(@"<(?:Nuke|Fallout)Version\s*>", RegexOptions.Compiled); + + /// + /// Property names whose values should be updated to the current Fallout version. + /// + /// + /// Starts with FalloutVersion so an existing dedicated property is updated even when + /// no package references it before the rewrite. + /// + public HashSet ToBump { get; } = [FalloutVersionVariable]; + + /// + /// Variable names used by both Fallout packages and unrelated packages. + /// + /// + /// Updating these properties would change unrelated package versions. Instead, Fallout package + /// references are redirected to $(FalloutVersion) and the shared property value is kept. + /// + public HashSet Ambiguous { get; } = []; + + /// + /// Whether any file already defines FalloutVersion, including its old NukeVersion name. + /// + public bool DefinesFalloutVersion { get; private set; } + + /// + /// Classifies version variable names over all files before any of them are rewritten. + /// + /// + /// Classification uses names across the repository, without evaluating MSBuild imports or scopes. + /// A name used by Fallout in project A and by another package in project B is treated as shared, + /// even if the projects define separate properties. This can introduce extra reference changes, + /// but avoids updating unrelated package versions. + /// + public static VersionVariables Collect(IEnumerable fileContents) + { + var result = new VersionVariables(); + var falloutVariables = new HashSet(); + var nonFalloutVariables = new HashSet(); + + foreach (string content in fileContents) + { + foreach (Match match in MsBuildPatterns.VariableVersionItem.Matches(content)) + { + string packageName = match.Groups["include"].Value; + bool isFalloutPackage = packageName.StartsWith("Nuke.", StringComparison.Ordinal) || + packageName.StartsWith("Fallout.", StringComparison.Ordinal); + (isFalloutPackage ? falloutVariables : nonFalloutVariables).Add(match.Groups["variable"].Value); + } + + result.DefinesFalloutVersion |= falloutVersionPropertyPattern.IsMatch(content); + } + + foreach (string variableName in falloutVariables) + { + (nonFalloutVariables.Contains(variableName) ? result.Ambiguous : result.ToBump).Add(variableName); + } + + return result; + } +} diff --git a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs index 6be7c5ab5..0445d1a01 100644 --- a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs +++ b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.IO; using System.Linq; using System.Text.RegularExpressions; using System.Threading.Tasks; @@ -19,332 +18,197 @@ namespace Fallout.Migrate.Steps; /// internal sealed class RewriteCsprojsStep : IMigrationStep { - // Package items: PackageReference, central PackageVersion (Directory.Packages.props, #492) - // and PackageDownload. 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. NUKE-era pins (e.g. `Version="10.1.0"`) don't exist as Fallout.* packages and - // 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( - @"<(?:PackageReference|PackageVersion|PackageDownload)\b[^>]*>", - RegexOptions.Compiled); - - // The `Nuke.` prefix of an Include, Update or Remove value: `Include="Nuke.X"` → `Include="Fallout.X"`. - // Pass 1 applies it to package items. Pass 2 applies it to every item that pass 1 didn't touch. - private static readonly Regex nukeItemNamePattern = - new(@"(?<=\b(?:Include|Update|Remove)="")Nuke\.(?=[A-Z])", RegexOptions.Compiled); - - // The literal value of a Version or VersionOverride attribute. PackageDownload needs an exact - // range (`[10.1.0]`), so the brackets stay outside the match. A version range such as - // `[10.1.0,)` or `(10.1.0,11.0.0)` doesn't match and is kept as written: replacing only - // one bound would leave an invalid range. - private static readonly Regex literalVersionPattern = new( - @"(?<=\bVersion(?:Override)?=""\[?)(?!\$\()[^""\[\],()]+(?=\]?"")", - RegexOptions.Compiled); - - // A PackageReference or central PackageVersion whose version is an MSBuild variable: - // Version="$(MyVar)". The Include and Version attributes can come in either order. - // Used to classify the variables (both Nuke.* and Fallout.* spellings count, because the files - // are classified before they are rewritten) and to redirect an ambiguous one. - private static readonly Regex variableVersionItemPattern = new( - @"<(?:PackageReference|PackageVersion)\b(?=[^>]*\bInclude=""(?[^""]+)"")(?=[^>]*\bVersion=""\$\((?[^)]+)\)"")[^>]*>", - RegexOptions.Compiled); - - // MSBuild element/property names that begin with `Nuke` followed by an uppercase - // letter (e.g. ...). Limited to known consumer-facing names from - // P3.5b so we don't rewrite unrelated user-defined identifiers that happen to start - // with the literal "Nuke". - private static readonly Regex msBuildPropertyPattern = new( - @"\bNuke(?=" + - "(?:Version|RootDirectory|ScriptDirectory|BaseDirectory|BaseNamespace|" + - "UseNestedNamespaces|RepositoryUrl|UpdateReferences|ContinueOnError|TaskTimeout|" + - "Timeout|TasksEnabled|DefaultExcludes|ExcludeBoot|ExcludeConfig|ExcludeLogs|" + - "ExcludeDirectoryBuild|ExcludeCi|SpecificationFiles|ExternalFiles|TasksAssembly|" + - "TasksDirectory)\\b)", - RegexOptions.Compiled); - - // Strip the telemetry-version property entirely — telemetry was removed from Fallout - // (ADR-0010), so a migrated project must not carry a dead . - // Matches the whole element line (either legacy Nuke* or already-Fallout* spelling). - private static readonly Regex telemetryVersionPropertyPattern = new( - @"^[ \t]*<(?(?:Nuke|Fallout)TelemetryVersion)>.*?>[ \t]*\r?\n?", - RegexOptions.Compiled | RegexOptions.Multiline); - - // Strip explicit `System.Security.Cryptography.Xml` PackageReferences. NUKE-era projects - // often pinned this directly at an older major (e.g. 9.x). Fallout.Common 10.2.12+ transitively - // requires a newer version (10.0.6+) and the conflict trips NU1605 ("Detected package - // downgrade"). Removing the explicit pin lets the transitive version win, which is what the - // migrated project wants (#217). Matches a self-closing element with optional surrounding - // indentation + trailing newline. Applied to *.csproj only: a pin in a *.props file such as - // a root Directory.Build.props applies to every project in the repository, not just the build. - private static readonly Regex cryptographyXmlPackageRefPattern = new( - @"^[ \t]*[ \t]*\r?\n?", - RegexOptions.Compiled | RegexOptions.Multiline); - /// public Task ExecuteAsync(MigrationContext context, Summary summary) { - var files = MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.csproj") - .Concat(MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.props")) - .ToList(); + Dictionary fileContents = ReadFileContents(context, summary); - // A version variable can be used in one file and defined in another, for example - // `$(NukeVersion)` in Directory.Packages.props and `` in Version.props (#492). - // So the variables are classified over all files first, then every file is rewritten. - var variables = VersionVariables.Collect(files.Select(ReadOrEmpty)); + // A variable can be used in Directory.Packages.props and defined in an imported Version.props. + // Classify all readable files before rewriting any of them, using the same contents for both. + VersionVariables variables = VersionVariables.Collect(fileContents.Values); - foreach (var path in files) + foreach ((AbsolutePath path, string content) in fileContents) { MigrationFileOperations.ApplyRewrite( context, path, - content => Rewrite( - content, - context.FalloutVersion, - variables, - isProjectFile: path.ToString().EndsWith(".csproj", StringComparison.OrdinalIgnoreCase)), + content, + original => Rewrite(original, context.FalloutVersion, variables, path.HasExtension(".csproj")), summary); } return Task.CompletedTask; + } + + private static Dictionary ReadFileContents(MigrationContext context, Summary summary) + { + IEnumerable paths = MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.csproj") + .Concat(MigrationFileOperations.EnumerateFiles(context.RootDirectory, "*.props")); + var fileContents = new Dictionary(); - static string ReadOrEmpty(AbsolutePath path) + foreach (AbsolutePath path in paths) { - // An unreadable file is reported by ApplyRewrite; it contributes nothing here. - try + if (MigrationFileOperations.TryRead(context, path, summary, out string content)) { - return path.ReadAllText(); - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) - { - return string.Empty; + fileContents.Add(path, content); } } + + return fileContents; } /// - /// Rewrites content, replacing Nuke.* references and MSBuild - /// properties with their Fallout.* equivalents and stripping stale pins. + /// Applies the ordered rewrite passes to one MSBuild file and counts their edits. /// - /// The original .csproj or .props file content. - /// The Fallout version to pin into rewritten versioned references. - /// The version variables classified over all files. - /// true for a .csproj, false for a .props file. - /// The rewritten content and the number of edits made. private static RewriteResult Rewrite(string original, string falloutVersion, VersionVariables variables, bool isProjectFile) { - var edits = 0; - var content = original; + var result = new RewriteResult(original, 0); + result = RenamePackageItemsAndPinLiteralVersions(result, falloutVersion); + result = RenameRemainingItemNames(result); + result = RenameMsBuildProperties(result); + result = RemoveTelemetryVersionProperty(result); - // Pass 1 — package items: rename a Nuke.X Include and pin its literal Version/VersionOverride. - // One edit per item, however many attributes changed. An item that is already Fallout.X - // keeps its pin. - content = packageItemPattern.Replace(content, m => + if (isProjectFile) { - var item = nukeItemNamePattern.Replace(m.Value, "Fallout."); - if (item == m.Value) - { - return m.Value; - } - - edits++; - return literalVersionPattern.Replace(item, _ => falloutVersion); - }); + result = RemoveCryptographyXmlPackageReference(result); + } - // Pass 2 — namespace-only rewrites for anything Pass 1 didn't consume (other item types, - // MSBuild properties). - content = nukeItemNamePattern.Replace(content, _ => - { - edits++; - return "Fallout."; - }); + result = RedirectSharedVersionReferences(result, variables, falloutVersion); + result = UpdateVersionProperties(result, variables.ToBump, falloutVersion); + return result; + } - content = msBuildPropertyPattern.Replace(content, _ => + private static RewriteResult RenamePackageItemsAndPinLiteralVersions(RewriteResult previous, string falloutVersion) + { + int edits = 0; + string content = MsBuildPatterns.PackageItem.Replace(previous.Content, match => { - edits++; - return "Fallout"; - }); + string renamedItem = MsBuildPatterns.NukeItemName.Replace(match.Value, "Fallout."); + if (renamedItem == match.Value) + { + return match.Value; + } - // Pass 3 — strip the telemetry-version property (feature removed in ADR-0010). - content = telemetryVersionPropertyPattern.Replace(content, _ => - { + // Count one edit per item. Already migrated Fallout items keep their literal version. edits++; - return string.Empty; + return MsBuildPatterns.LiteralVersion.Replace(renamedItem, _ => falloutVersion); }); - // Pass 4 — strip the stale System.Security.Cryptography.Xml direct pin (project files only). - if (isProjectFile) - { - content = cryptographyXmlPackageRefPattern.Replace(content, _ => - { - edits++; - return string.Empty; - }); - } - - return HandleMsBuildVariable(falloutVersion, content, edits, variables); + return new RewriteResult(content, previous.EditCount + edits); } - // Pass 5 — decouple the variables ambiguously shared with non-Fallout packages via a dedicated - // $(FalloutVersion) property, and bump every variable that's exclusively Fallout's to the - // current Fallout version, in whichever file defines it. - private static RewriteResult HandleMsBuildVariable(string falloutVersion, string content, int edits, VersionVariables variables) - { - (content, int redirectEdits) = - RedirectAmbiguousVariablesToFalloutVersion(content, variables.Ambiguous, VersionVariables.FalloutVersionVariable); - - edits += redirectEdits; + private static RewriteResult RenameRemainingItemNames(RewriteResult previous) => + ReplaceAndCount(previous, MsBuildPatterns.NukeItemName, "Fallout."); - // Only a file with a redirected reference gets the property. Every other file stays unchanged. - if (redirectEdits > 0 && !variables.DefinesFalloutVersion) - { - content = EnsureFalloutVersionPropertyExists(content, VersionVariables.FalloutVersionVariable, falloutVersion, ref edits); - } + private static RewriteResult RenameMsBuildProperties(RewriteResult previous) => + ReplaceAndCount(previous, MsBuildPatterns.NukePropertyName, "Fallout"); - (content, int bumpEdits) = BumpVariableProperties(content, variables.ToBump, falloutVersion); - edits += bumpEdits; + private static RewriteResult RemoveTelemetryVersionProperty(RewriteResult previous) => + ReplaceAndCount(previous, MsBuildPatterns.TelemetryVersionProperty, string.Empty); - return new RewriteResult(content, edits); - } + private static RewriteResult RemoveCryptographyXmlPackageReference(RewriteResult previous) => + ReplaceAndCount(previous, MsBuildPatterns.CryptographyXmlPackageReference, string.Empty); - /// - /// The version variables found in the files of one migration run: those used only by Nuke.*/Fallout.* - /// packages (bumped), those also shared with an unrelated package (ambiguous), and whether any - /// file already defines the dedicated FalloutVersion property. - /// - private sealed class VersionVariables + private static RewriteResult ReplaceAndCount(RewriteResult previous, Regex pattern, string replacement) { - public const string FalloutVersionVariable = "FalloutVersion"; - - // The property is renamed from NukeVersion by msBuildPropertyPattern, so both spellings count. - private static readonly Regex falloutVersionPropertyPattern = - new(@"<(?:Nuke|Fallout)Version\s*>", RegexOptions.Compiled); - - public HashSet ToBump { get; } = [FalloutVersionVariable]; - public HashSet Ambiguous { get; } = []; - public bool DefinesFalloutVersion { get; private set; } - - // 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. - // - // Ambiguity is decided by variable name over all files. A per-project property used by a - // Fallout package in project A and by an unrelated package in project B is ambiguous, so - // A is decoupled even though A's own definition isn't shared. The result is still correct, - // it only has more edits than needed. - public static VersionVariables Collect(IEnumerable contents) + int edits = 0; + string content = pattern.Replace(previous.Content, _ => { - var result = new VersionVariables(); - var falloutVariables = new HashSet(); - var nonFalloutVariables = new HashSet(); - - foreach (var content in contents) - { - foreach (Match match in variableVersionItemPattern.Matches(content)) - { - var include = match.Groups["include"].Value; - var isFalloutPackage = include.StartsWith("Nuke.", StringComparison.Ordinal) || - include.StartsWith("Fallout.", StringComparison.Ordinal); - (isFalloutPackage ? falloutVariables : nonFalloutVariables).Add(match.Groups["variable"].Value); - } - - result.DefinesFalloutVersion |= falloutVersionPropertyPattern.IsMatch(content); - } - - foreach (var variable in falloutVariables) - { - (nonFalloutVariables.Contains(variable) ? result.Ambiguous : result.ToBump).Add(variable); - } + edits++; + return replacement; + }); - return result; - } + return new RewriteResult(content, previous.EditCount + edits); } - private static (string content, int edits) RedirectAmbiguousVariablesToFalloutVersion( - string content, HashSet ambiguousVariables, string falloutVersionVariable) + private static RewriteResult RedirectSharedVersionReferences( + RewriteResult previous, VersionVariables variables, string falloutVersion) { - var edits = 0; - - // Runs after the rename, so only Fallout.* items are redirected. Only the variable name - // inside `$(...)` is swapped. The rest of the item stays as written. - content = variableVersionItemPattern.Replace(content, m => + int edits = 0; + string content = MsBuildPatterns.VariableVersionItem.Replace(previous.Content, match => { - var variable = m.Groups["variable"]; - if (!m.Groups["include"].Value.StartsWith("Fallout.", StringComparison.Ordinal) || - !ambiguousVariables.Contains(variable.Value)) + Group variableName = match.Groups["variable"]; + if (!match.Groups["include"].Value.StartsWith("Fallout.", StringComparison.Ordinal) || + !variables.Ambiguous.Contains(variableName.Value)) { - return m.Value; + return match.Value; } + // Runs after renaming packages. Change only the variable name in Fallout references, + // leaving the shared property and the unrelated package references unchanged. edits++; - var offset = variable.Index - m.Index; - return m.Value.Remove(offset, variable.Length).Insert(offset, falloutVersionVariable); + int offset = variableName.Index - match.Index; + return match.Value.Remove(offset, variableName.Length).Insert(offset, VersionVariables.FalloutVersionVariable); }); - return (content, edits); + var result = new RewriteResult(content, previous.EditCount + edits); + + // Only a file with a redirected reference needs a property, and only if no file defines it. + return edits > 0 && !variables.DefinesFalloutVersion + ? InsertFalloutVersionProperty(result, falloutVersion) + : result; } - private static string EnsureFalloutVersionPropertyExists( - string content, string falloutVersionVariable, string falloutVersion, ref int edits) + private static RewriteResult InsertFalloutVersionProperty(RewriteResult previous, string falloutVersion) { - if (content.Contains($"<{falloutVersionVariable}>")) - { - return content; - } - - var propertyGroupIndex = content.IndexOf("", StringComparison.Ordinal); + string content = previous.Content; + int propertyGroupIndex = content.IndexOf("", StringComparison.Ordinal); if (propertyGroupIndex >= 0) { - edits++; - return content.Insert( + content = content.Insert( propertyGroupIndex + "".Length, - $"\n <{falloutVersionVariable}>{falloutVersion}"); + $"\n {falloutVersion}"); + return new RewriteResult(content, previous.EditCount + 1); } - // No PropertyGroup exists at all (e.g. a project relying solely on Directory.Build.props - // for properties) — synthesize one right after the opening tag so the newly - // introduced $(FalloutVersion) reference has somewhere to resolve from. - var projectTagStart = content.IndexOf("= 0 + // A file without an unconditional PropertyGroup needs one after the opening Project tag. + int projectTagStart = content.IndexOf("= 0 ? content.IndexOf('>', projectTagStart) : -1; if (projectTagEnd < 0) { - return content; + return previous; } - edits++; - return content.Insert( + content = content.Insert( projectTagEnd + 1, - $"\n \n <{falloutVersionVariable}>{falloutVersion}\n "); + $"\n \n {falloutVersion}\n "); + return new RewriteResult(content, previous.EditCount + 1); } - private static (string content, int edits) BumpVariableProperties(string content, HashSet variablesToBump, - string falloutVersion) + private static RewriteResult UpdateVersionProperties( + RewriteResult previous, HashSet variablesToBump, string falloutVersion) { - var edits = 0; + int edits = 0; + string content = previous.Content; - foreach (var variable in variablesToBump) + foreach (string variableName in variablesToBump) { - // Matches the text content of the ... property element itself - // (via lookbehind/lookahead, so the tags aren't part of the match and stay intact). - var pattern = $@"(?<=<{variable}\s*>)[^<]+(?=)"; - content = Regex.Replace(content, - pattern, - m => + content = CreatePropertyValuePattern(variableName).Replace(content, match => + { + if (match.Value == falloutVersion) { - if (m.Value == falloutVersion) - { - return m.Value; - } - - edits++; - return falloutVersion; - }); + return match.Value; + } + + edits++; + return falloutVersion; + }); } - return (content, edits); + return new RewriteResult(content, previous.EditCount + edits); } + + /// + /// Matches the text value inside a named MSBuild property element. + /// + /// + /// Lookarounds leave the opening and closing tags outside the match. Whitespace before + /// > is allowed, but attributes, empty values, and nested elements are not matched. + /// Property names and values are case-sensitive. + /// + private static Regex CreatePropertyValuePattern(string variableName) => + new($@"(?<=<{variableName}\s*>)[^<]+(?=)"); } diff --git a/tests/Fallout.Migrate.Specs/MigrationFileOperationsSpecs.cs b/tests/Fallout.Migrate.Specs/MigrationFileOperationsSpecs.cs new file mode 100644 index 000000000..fc52c1742 --- /dev/null +++ b/tests/Fallout.Migrate.Specs/MigrationFileOperationsSpecs.cs @@ -0,0 +1,72 @@ +using System; +using System.IO; +using Fallout.Common.IO; +using Fallout.Migrate.Common; +using FluentAssertions; +using Xunit; + +namespace Fallout.Migrate.Specs; + +public class MigrationFileOperationsSpecs : IDisposable +{ + private readonly AbsolutePath tempDirectory = AbsolutePath.Temp("fallout-migrate-test"); + + public MigrationFileOperationsSpecs() + { + tempDirectory.CreateDirectory(); + } + + public void Dispose() + { + tempDirectory.DeleteDirectory(); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Unreadable_files_are_reported_once_and_not_rewritten(bool pathIsDirectory) + { + // Arrange + AbsolutePath path = tempDirectory / "unreadable.props"; + if (pathIsDirectory) + { + path.CreateDirectory(); + } + + var context = new MigrationContext(tempDirectory, dryRun: false, TextWriter.Null); + var summary = new Summary(); + bool rewriterCalled = false; + + // Act + MigrationFileOperations.ApplyRewrite(context, path, content => + { + rewriterCalled = true; + return new RewriteResult(content, 0); + }, summary); + + // Assert + rewriterCalled.Should().BeFalse(); + summary.Warnings.Should().ContainSingle().Which.Should().StartWith("could not read unreadable.props:"); + summary.FilesChanged.Should().Be(0); + summary.EditCount.Should().Be(0); + } + + [Fact] + public void Rewrite_failures_propagate_instead_of_becoming_read_warnings() + { + // Arrange + AbsolutePath path = tempDirectory / "Version.props"; + path.WriteAllText("", eofLineBreak: false); + var context = new MigrationContext(tempDirectory, dryRun: false, TextWriter.Null); + var summary = new Summary(); + var failure = new IOException("rewrite failed"); + + // Act + Action rewrite = () => MigrationFileOperations.ApplyRewrite(context, path, _ => throw failure, summary); + + // Assert + rewrite.Should().Throw().Which.Should().BeSameAs(failure); + summary.Warnings.Should().BeEmpty(); + path.ReadAllText().Should().Be(""); + } +} diff --git a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs index 44bdade00..bda4748af 100644 --- a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs +++ b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs @@ -637,6 +637,68 @@ public async Task Version_variable_shared_with_another_package_in_another_file_i (tempDirectory / "Version.props").ReadAllText().Should().Contain("4.0.0"); } + [Theory] + [InlineData("FalloutVersion")] + [InlineData("NukeVersion")] + public async Task An_existing_version_property_in_another_file_is_reused(string propertyName) + { + // Arrange + (tempDirectory / "Directory.Packages.props").WriteAllText(""" + + + + + + + + """, eofLineBreak: false); + (tempDirectory / "build" / "_build.csproj").WriteAllText(""" + + + + net10.0 + + + + + + """, eofLineBreak: false); + (tempDirectory / "Version.props").WriteAllText($""" + + + <{propertyName}>10.1.0 + 4.0.0 + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + string packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); + packages.Should().Contain(""""""); + packages.Should().Contain(""""""); + packages.Should().NotContain(""); + string buildProject = (tempDirectory / "build" / "_build.csproj").ReadAllText(); + buildProject.Should().Contain(""""""); + buildProject.Should().NotContain(""); + (tempDirectory / "Version.props").ReadAllText().Should().Be(""" + + + 11.0.0 + 4.0.0 + + + """); + + // The new references and the existing property are stable on subsequent runs. + var secondRun = new Summary(); + await new RewriteCsprojsStep().ExecuteAsync(context, secondRun); + secondRun.EditCount.Should().Be(0); + secondRun.FilesChanged.Should().Be(0); + } + [Fact] public async Task Files_without_a_redirected_reference_are_unchanged_when_a_variable_is_decoupled() { From 167947cc5f4646be52a72262c8d8dda748a71310 Mon Sep 17 00:00:00 2001 From: Alexander Nachtmann Date: Tue, 13 Oct 2026 00:33:10 +0200 Subject: [PATCH 5/7] Report a missing file as a read warning and name the specs after their 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 --- .../Common/MigrationFileOperations.cs | 10 +++++++--- .../RewriteCsprojsStepSpecs.cs | 20 +++++++++---------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/Fallout.Migrate/Common/MigrationFileOperations.cs b/src/Fallout.Migrate/Common/MigrationFileOperations.cs index 3bca9fae1..1f3077a5a 100644 --- a/src/Fallout.Migrate/Common/MigrationFileOperations.cs +++ b/src/Fallout.Migrate/Common/MigrationFileOperations.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Text; using Fallout.Common.IO; namespace Fallout.Migrate.Common; @@ -55,14 +56,17 @@ public static void ApplyRewrite( /// Reads a file and reports an unreadable file as a warning. /// /// - /// Only file access failures are handled here. Exceptions from rewriting or writing a file - /// propagate to the caller. A failed read must be skipped, not classified as an empty file. + /// Only file access failures are handled here: an , such as a missing file, + /// and an , such as a directory or a denied permission. + /// Exceptions from rewriting or writing a file propagate to the caller. + /// A failed read must be skipped, not classified as an empty file. /// public static bool TryRead(MigrationContext context, AbsolutePath path, Summary summary, out string content) { try { - content = path.ReadAllText(); + // Not AbsolutePath.ReadAllText: it throws an ArgumentException for a missing file before reading. + content = File.ReadAllText(path, Encoding.UTF8); return true; } catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) diff --git a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs index bda4748af..555746ccd 100644 --- a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs +++ b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs @@ -427,7 +427,7 @@ public async Task Does_not_bump_variables_used_for_non_Fallout_packages() } [Fact] - public async Task Decouples_ambiguously_used_variables() + public async Task Shared_version_variable_is_redirected_to_FalloutVersion() { const string input = """ @@ -454,7 +454,7 @@ public async Task Decouples_ambiguously_used_variables() } [Fact] - public async Task Decouples_ambiguously_used_variables_even_when_no_property_group_found() + public async Task Shared_version_variable_is_redirected_when_the_file_has_no_property_group() { const string input = """ @@ -479,7 +479,7 @@ public async Task Decouples_ambiguously_used_variables_even_when_no_property_gro } [Fact] - public async Task Decouples_ambiguously_used_variables_when_a_property_group_but_no_variable_exists() + public async Task Shared_version_variable_is_redirected_when_the_file_has_an_empty_property_group() { const string input = """ @@ -599,7 +599,7 @@ public async Task Version_variable_defined_in_another_file_is_bumped() } [Fact] - public async Task Version_variable_shared_with_another_package_in_another_file_is_decoupled() + public async Task Version_variable_shared_across_files_is_redirected_to_FalloutVersion() { // Arrange (tempDirectory / "Directory.Packages.props").WriteAllText(""" @@ -631,7 +631,7 @@ public async Task Version_variable_shared_with_another_package_in_another_file_i await new RewriteCsprojsStep().ExecuteAsync(context, summary); // Assert - var packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); + string packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); packages.Should().Contain(""""""); packages.Should().Contain("11.0.0"); (tempDirectory / "Version.props").ReadAllText().Should().Contain("4.0.0"); @@ -700,7 +700,7 @@ public async Task An_existing_version_property_in_another_file_is_reused(string } [Fact] - public async Task Files_without_a_redirected_reference_are_unchanged_when_a_variable_is_decoupled() + public async Task Only_files_with_a_redirected_reference_get_the_FalloutVersion_property() { // Arrange (tempDirectory / "Directory.Packages.props").WriteAllText(""" @@ -735,7 +735,7 @@ public async Task Files_without_a_redirected_reference_are_unchanged_when_a_vari await new RewriteCsprojsStep().ExecuteAsync(context, summary); // Assert - var packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); + string packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); packages.Should().Contain(""""""); packages.Should().Contain("11.0.0"); (tempDirectory / "Version.props").ReadAllText().Should().Be(versionProps); @@ -743,7 +743,7 @@ public async Task Files_without_a_redirected_reference_are_unchanged_when_a_vari } [Fact] - public async Task Version_variable_before_include_is_decoupled() + public async Task Version_variable_before_include_is_redirected_to_FalloutVersion() { // Arrange (tempDirectory / "Directory.Packages.props").WriteAllText(""" @@ -762,7 +762,7 @@ public async Task Version_variable_before_include_is_decoupled() await new RewriteCsprojsStep().ExecuteAsync(context, summary); // Assert - var packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); + string packages = (tempDirectory / "Directory.Packages.props").ReadAllText(); packages.Should().Contain(""""""); packages.Should().Contain(""""""); packages.Should().Contain("4.0.0"); @@ -786,7 +786,7 @@ public async Task Version_range_is_kept_as_written() await new RewriteCsprojsStep().ExecuteAsync(context, summary); // Assert - var buildCsproj = (tempDirectory / "build" / "_build.csproj").ReadAllText(); + string buildCsproj = (tempDirectory / "build" / "_build.csproj").ReadAllText(); buildCsproj.Should().Contain(""""""); buildCsproj.Should().Contain(""""""); } From b4500df807dacc16ea07d8cdc9c8f41c024b23a0 Mon Sep 17 00:00:00 2001 From: Alexander Nachtmann Date: Tue, 13 Oct 2026 05:32:08 +0200 Subject: [PATCH 6/7] Write the MSBuild patterns as raw string literals and name their parts 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 --- src/Fallout.Migrate/Common/MsBuildPatterns.cs | 41 ++++++++++++------- .../Common/VersionVariables.cs | 6 +-- .../Steps/RewriteCsprojsStep.cs | 18 +++++--- 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/Fallout.Migrate/Common/MsBuildPatterns.cs b/src/Fallout.Migrate/Common/MsBuildPatterns.cs index f708926fa..9979565b6 100644 --- a/src/Fallout.Migrate/Common/MsBuildPatterns.cs +++ b/src/Fallout.Migrate/Common/MsBuildPatterns.cs @@ -20,7 +20,7 @@ internal static class MsBuildPatterns /// name or version separately. A > inside an attribute value ends the match early. /// public static readonly Regex PackageItem = new( - @"<(?:PackageReference|PackageVersion|PackageDownload)\b[^>]*>", + """<(?:PackageReference|PackageVersion|PackageDownload)\b[^>]*>""", RegexOptions.Compiled); /// @@ -31,7 +31,7 @@ internal static class MsBuildPatterns /// The attribute name and the rest of the item name stay unchanged. /// public static readonly Regex NukeItemName = - new(@"(?<=\b(?:Include|Update|Remove)="")Nuke\.(?=[A-Z])", RegexOptions.Compiled); + new("""(?<=\b(?:Include|Update|Remove)=")Nuke\.(?=[A-Z])""", RegexOptions.Compiled); /// /// Finds a literal version in a Version or VersionOverride attribute. @@ -42,7 +42,7 @@ internal static class MsBuildPatterns /// are excluded. Replacing only one bound of a range would leave an invalid version. /// public static readonly Regex LiteralVersion = new( - @"(?<=\bVersion(?:Override)?=""\[?)(?!\$\()[^""\[\],()]+(?=\]?"")", + """(?<=\bVersion(?:Override)?="\[?)(?!\$\()[^"\[\],()]+(?=\]?")""", RegexOptions.Compiled); /// @@ -56,24 +56,37 @@ internal static class MsBuildPatterns /// A > inside an attribute value ends the tag match early. /// public static readonly Regex VariableVersionItem = new( - @"<(?:PackageReference|PackageVersion)\b(?=[^>]*\bInclude=""(?[^""]+)"")(?=[^>]*\bVersion=""\$\((?[^)]+)\)"")[^>]*>", + """<(?:PackageReference|PackageVersion)\b(?=[^>]*\bInclude="(?[^"]+)")(?=[^>]*\bVersion="\$\((?[^)]+)\)")[^>]*>""", RegexOptions.Compiled); + /// The package name in a match. + public static Group PackageNameOf(Match variableVersionItem) => variableVersionItem.Groups["include"]; + + /// The version variable name, without $(...), in a match. + public static Group VariableNameOf(Match variableVersionItem) => variableVersionItem.Groups["variable"]; + + /// + /// The MSBuild properties Fallout renamed from Nuke* to Fallout*, without their prefix. + /// + private static readonly string[] RenamedNukePropertySuffixes = + [ + "Version", "RootDirectory", "ScriptDirectory", "BaseDirectory", "BaseNamespace", + "UseNestedNamespaces", "RepositoryUrl", "UpdateReferences", "ContinueOnError", "TaskTimeout", + "Timeout", "TasksEnabled", "DefaultExcludes", "ExcludeBoot", "ExcludeConfig", "ExcludeLogs", + "ExcludeDirectoryBuild", "ExcludeCi", "SpecificationFiles", "ExternalFiles", "TasksAssembly", + "TasksDirectory" + ]; + /// /// Finds the Nuke prefix of known MSBuild properties that were renamed in Fallout. /// /// - /// Only the prefix is matched. The listed property suffixes limit the rename to known names, - /// including both element names and references such as $(NukeVersion). + /// Only the prefix is matched. limits the rename to known + /// names, including both element names and references such as $(NukeVersion). /// Unrelated user-defined names such as NukeCustomProperty are excluded. /// public static readonly Regex NukePropertyName = new( - @"\bNuke(?=" + - "(?:Version|RootDirectory|ScriptDirectory|BaseDirectory|BaseNamespace|" + - "UseNestedNamespaces|RepositoryUrl|UpdateReferences|ContinueOnError|TaskTimeout|" + - "Timeout|TasksEnabled|DefaultExcludes|ExcludeBoot|ExcludeConfig|ExcludeLogs|" + - "ExcludeDirectoryBuild|ExcludeCi|SpecificationFiles|ExternalFiles|TasksAssembly|" + - "TasksDirectory)\\b)", + $"""\bNuke(?=(?:{string.Join('|', RenamedNukePropertySuffixes)})\b)""", RegexOptions.Compiled); /// @@ -85,7 +98,7 @@ internal static class MsBuildPatterns /// so the whole property is deleted rather than renamed. /// public static readonly Regex TelemetryVersionProperty = new( - @"^[ \t]*<(?(?:Nuke|Fallout)TelemetryVersion)>.*?>[ \t]*\r?\n?", + """^[ \t]*<(?(?:Nuke|Fallout)TelemetryVersion)>.*?>[ \t]*\r?\n?""", RegexOptions.Compiled | RegexOptions.Multiline); /// @@ -97,6 +110,6 @@ internal static class MsBuildPatterns /// dependency version to win. A pin in a shared props file is kept because it affects other projects. /// public static readonly Regex CryptographyXmlPackageReference = new( - @"^[ \t]*[ \t]*\r?\n?", + """^[ \t]*[ \t]*\r?\n?""", RegexOptions.Compiled | RegexOptions.Multiline); } diff --git a/src/Fallout.Migrate/Common/VersionVariables.cs b/src/Fallout.Migrate/Common/VersionVariables.cs index 0be6d41d6..3a6fefd87 100644 --- a/src/Fallout.Migrate/Common/VersionVariables.cs +++ b/src/Fallout.Migrate/Common/VersionVariables.cs @@ -20,7 +20,7 @@ internal sealed class VersionVariables /// The match is case-sensitive and permits whitespace before >, but no attributes. /// private static readonly Regex falloutVersionPropertyPattern = - new(@"<(?:Nuke|Fallout)Version\s*>", RegexOptions.Compiled); + new("""<(?:Nuke|Fallout)Version\s*>""", RegexOptions.Compiled); /// /// Property names whose values should be updated to the current Fallout version. @@ -64,10 +64,10 @@ public static VersionVariables Collect(IEnumerable fileContents) { foreach (Match match in MsBuildPatterns.VariableVersionItem.Matches(content)) { - string packageName = match.Groups["include"].Value; + string packageName = MsBuildPatterns.PackageNameOf(match).Value; bool isFalloutPackage = packageName.StartsWith("Nuke.", StringComparison.Ordinal) || packageName.StartsWith("Fallout.", StringComparison.Ordinal); - (isFalloutPackage ? falloutVariables : nonFalloutVariables).Add(match.Groups["variable"].Value); + (isFalloutPackage ? falloutVariables : nonFalloutVariables).Add(MsBuildPatterns.VariableNameOf(match).Value); } result.DefinesFalloutVersion |= falloutVersionPropertyPattern.IsMatch(content); diff --git a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs index 0445d1a01..c7c9cca5b 100644 --- a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs +++ b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs @@ -127,8 +127,8 @@ private static RewriteResult RedirectSharedVersionReferences( int edits = 0; string content = MsBuildPatterns.VariableVersionItem.Replace(previous.Content, match => { - Group variableName = match.Groups["variable"]; - if (!match.Groups["include"].Value.StartsWith("Fallout.", StringComparison.Ordinal) || + Group variableName = MsBuildPatterns.VariableNameOf(match); + if (!MsBuildPatterns.PackageNameOf(match).Value.StartsWith("Fallout.", StringComparison.Ordinal) || !variables.Ambiguous.Contains(variableName.Value)) { return match.Value; @@ -137,8 +137,7 @@ private static RewriteResult RedirectSharedVersionReferences( // Runs after renaming packages. Change only the variable name in Fallout references, // leaving the shared property and the unrelated package references unchanged. edits++; - int offset = variableName.Index - match.Index; - return match.Value.Remove(offset, variableName.Length).Insert(offset, VersionVariables.FalloutVersionVariable); + return ReplaceGroupValue(match, variableName, VersionVariables.FalloutVersionVariable); }); var result = new RewriteResult(content, previous.EditCount + edits); @@ -149,6 +148,15 @@ private static RewriteResult RedirectSharedVersionReferences( : result; } + /// + /// Replaces the text one group captured and keeps the rest of the matched text as written. + /// + private static string ReplaceGroupValue(Match match, Group group, string replacement) + { + int offset = group.Index - match.Index; + return match.Value.Remove(offset, group.Length).Insert(offset, replacement); + } + private static RewriteResult InsertFalloutVersionProperty(RewriteResult previous, string falloutVersion) { string content = previous.Content; @@ -210,5 +218,5 @@ private static RewriteResult UpdateVersionProperties( /// Property names and values are case-sensitive. /// private static Regex CreatePropertyValuePattern(string variableName) => - new($@"(?<=<{variableName}\s*>)[^<]+(?=)"); + new($"""(?<=<{variableName}\s*>)[^<]+(?=)"""); } From 8c30f3c0a7f7360ad9b09e6dfc4db5a69dd8f3cc Mon Sep 17 00:00:00 2001 From: Alexander Nachtmann Date: Wed, 14 Oct 2026 19:38:00 +0200 Subject: [PATCH 7/7] Keep line endings, escape property names, and strip a pin in any attribute 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 --- src/Fallout.Migrate/Common/MsBuildPatterns.cs | 4 +- .../Steps/RewriteCsprojsStep.cs | 16 ++-- .../RewriteCsprojsStepSpecs.cs | 87 +++++++++++++++++++ 3 files changed, 100 insertions(+), 7 deletions(-) diff --git a/src/Fallout.Migrate/Common/MsBuildPatterns.cs b/src/Fallout.Migrate/Common/MsBuildPatterns.cs index 9979565b6..2bafd815a 100644 --- a/src/Fallout.Migrate/Common/MsBuildPatterns.cs +++ b/src/Fallout.Migrate/Common/MsBuildPatterns.cs @@ -105,11 +105,11 @@ internal static class MsBuildPatterns /// Finds a self-closing System.Security.Cryptography.Xml package reference line. /// /// - /// The Include attribute must come first. Indentation and a trailing newline are included. + /// Attributes may appear in any order. Indentation and a trailing newline are included. /// The migration removes this direct pin only from project files, allowing Fallout's transitive /// dependency version to win. A pin in a shared props file is kept because it affects other projects. /// public static readonly Regex CryptographyXmlPackageReference = new( - """^[ \t]*[ \t]*\r?\n?""", + """^[ \t]*]*\bInclude="System\.Security\.Cryptography\.Xml")[^/]*/>[ \t]*\r?\n?""", RegexOptions.Compiled | RegexOptions.Multiline); } diff --git a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs index c7c9cca5b..04a306c7c 100644 --- a/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs +++ b/src/Fallout.Migrate/Steps/RewriteCsprojsStep.cs @@ -143,6 +143,8 @@ private static RewriteResult RedirectSharedVersionReferences( var result = new RewriteResult(content, previous.EditCount + edits); // Only a file with a redirected reference needs a property, and only if no file defines it. + // Each such file gets its own property. A property in one project file is not visible in + // another, and this step does not follow MSBuild imports to find a file that both import. return edits > 0 && !variables.DefinesFalloutVersion ? InsertFalloutVersionProperty(result, falloutVersion) : result; @@ -160,12 +162,13 @@ private static string ReplaceGroupValue(Match match, Group group, string replace private static RewriteResult InsertFalloutVersionProperty(RewriteResult previous, string falloutVersion) { string content = previous.Content; + string newLine = content.Contains("\r\n", StringComparison.Ordinal) ? "\r\n" : "\n"; int propertyGroupIndex = content.IndexOf("", StringComparison.Ordinal); if (propertyGroupIndex >= 0) { content = content.Insert( propertyGroupIndex + "".Length, - $"\n {falloutVersion}"); + $"{newLine} {falloutVersion}"); return new RewriteResult(content, previous.EditCount + 1); } @@ -182,7 +185,7 @@ private static RewriteResult InsertFalloutVersionProperty(RewriteResult previous content = content.Insert( projectTagEnd + 1, - $"\n \n {falloutVersion}\n "); + $"{newLine} {newLine} {falloutVersion}{newLine} "); return new RewriteResult(content, previous.EditCount + 1); } @@ -215,8 +218,11 @@ private static RewriteResult UpdateVersionProperties( /// /// Lookarounds leave the opening and closing tags outside the match. Whitespace before /// > is allowed, but attributes, empty values, and nested elements are not matched. - /// Property names and values are case-sensitive. + /// Property names and values are case-sensitive. The name comes from the file, so it is escaped. /// - private static Regex CreatePropertyValuePattern(string variableName) => - new($"""(?<=<{variableName}\s*>)[^<]+(?=)"""); + private static Regex CreatePropertyValuePattern(string variableName) + { + string name = Regex.Escape(variableName); + return new Regex($"""(?<=<{name}\s*>)[^<]+(?=)"""); + } } diff --git a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs index 555746ccd..e4f33f9ef 100644 --- a/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs +++ b/tests/Fallout.Migrate.Specs/RewriteCsprojsStepSpecs.cs @@ -244,6 +244,32 @@ public async Task Cryptography_xml_pin_in_a_props_file_is_kept() (tempDirectory / "Directory.Build.props").ReadAllText().Should().Be(input); } + [Fact] + public async Task Cryptography_xml_pin_with_version_before_include_is_stripped() + { + // Arrange + (tempDirectory / "build" / "_build.csproj").WriteAllText(""" + + + + + + + """, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + (tempDirectory / "build" / "_build.csproj").ReadAllText().Should().Be(""" + + + + + + """); + } + [Fact] public async Task Other_system_packages_are_left_alone() { @@ -769,6 +795,67 @@ public async Task Version_variable_before_include_is_redirected_to_FalloutVersio packages.Should().Contain("11.0.0"); } + [Theory] + [InlineData(""" + + + 10.1.0 + + + + + + + """)] + [InlineData(""" + + + + + + + """)] + public async Task FalloutVersion_property_uses_the_line_endings_of_the_file(string input) + { + // Arrange + (tempDirectory / "build" / "_build.csproj").WriteAllText(input.ReplaceLineEndings("\r\n"), eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + string buildProject = (tempDirectory / "build" / "_build.csproj").ReadAllText(); + buildProject.Should().Contain("11.0.0\r\n"); + buildProject.Replace("\r\n", string.Empty).Should().NotContain("\n"); + } + + [Fact] + public async Task Version_variable_with_regex_characters_in_its_name_is_matched_literally() + { + // Arrange + const string input = """ + + + 10.1.0 + 10.1.0 + + + + + + """; + (tempDirectory / "build" / "_build.csproj").WriteAllText(input, eofLineBreak: false); + + // Act + await new RewriteCsprojsStep().ExecuteAsync(context, summary); + + // Assert + string buildProject = (tempDirectory / "build" / "_build.csproj").ReadAllText(); + buildProject.Should().Contain("11.0.0"); + buildProject.Should().Contain("10.1.0"); + buildProject.Should().Contain(""""""); + } + [Fact] public async Task Version_range_is_kept_as_written() {