Recognizes version variables and updates the version accordingly#478
Recognizes version variables and updates the version accordingly#478IT-VBFK wants to merge 1 commit into
Conversation
2e73450 to
1391641
Compare
f157a46 to
40c5ed9
Compare
There was a problem hiding this comment.
Pull request overview
Updates Fallout.Migrate’s .csproj rewrite logic to handle projects that pin Fallout/Nuke package versions via MSBuild properties (e.g., Version="$(NukeVersion)"), aligning with issue #477’s migration expectations.
Changes:
- Refines the “inline literal version” rewrite to avoid consuming
Version="$(...)"so variable-based versions can be handled separately. - Adds a new pass that discovers version variables used by PackageReferences and bumps the corresponding property values to the target Fallout version.
- Extends specs to cover
NukeVersion→FalloutVersionrenaming and arbitrary version-variable bumping behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Fallout.Migrate/CsprojRewriter.cs |
Adds variable-aware version handling and bumps property-based versions to the target Fallout version. |
tests/Fallout.Migrate.Specs/CsprojRewriterSpecs.cs |
Adds/adjusts specs to validate migration behavior for version variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
da9ece9 to
d3fc9b9
Compare
d3fc9b9 to
f3ff5e1
Compare
|
@copilot review |
f3ff5e1 to
2fb66d4
Compare
2fb66d4 to
51c3c6b
Compare
0550dd3 to
8e438a1
Compare
|
Do not merge, please (meaning now). I rebase the commits later to better commits :) |
fb768bd to
32902ef
Compare
|
@avidenic Can you do the final review.. mine isn't worth it (person from IT-VBFK == ITaluone) ;) |
|
As I'm cleaning up the migration, please wait with the merge after which I've taken a look. |
avidenic
left a comment
There was a problem hiding this comment.
Good improvements, some small issues to look at. Some should probably go into follow up issues / PRs. Or we can decide we're fine and just add docs about gaps.
80cc687 to
a3240c0
Compare
avidenic
left a comment
There was a problem hiding this comment.
Substantial improvements. There are still some decisions to be made though.
dennisdoomen
left a comment
There was a problem hiding this comment.
I'd like to up our game here, even for our migration code, as that is going to be existing for years to come. So I'm going to be a bit more critical on the code review level. Don't get me wrong. We love all these contributions, but Nuke's code hasn't been particularly well factored and documented. So bear with me.
🔧 The PRs should explain what they are trying to do. Just a reference to an old (potentially convoluted) Nuke issue isn't enough anymore.
📝 The new code in this PR is IMO too long, too cryptic and has too little documentation.
|
Ok.. I propose to start over with small focused steps Ok @dennisdoomen? |
Yeah, and base it on #507 |
|
Cool I couldn't imagine how far your migration refactoring could go 😂😂 |
02e6f48 to
75c7270
Compare
75c7270 to
f29e707
Compare
f29e707 to
cb4fb9d
Compare
Summary
Update
Fallout.Migrateso that package versions referenced through MSBuild properties are migrated to the target Fallout version.Previously, the
.csprojrewriter handled package references with literal versions:However, projects that shared a version through an MSBuild property were only partially migrated:
The package IDs could be renamed to
Fallout.*, while the property and its value remained on the old NUKE version.This left the migrated project in an inconsistent state and could cause package restore or build failures.
Closes #477.
What changed
Distinguish literal versions from property-based versions
The inline package-version rewrite now excludes version values beginning with an MSBuild property expression such as:
This prevents the existing literal-version rewrite from replacing the property reference itself with an inline version.
Property-based version management therefore remains property-based after migration.
For example:
is processed by the property-aware migration flow instead of being flattened into:
Detect version properties used by package references
A new rewrite pass discovers MSBuild properties referenced by
PackageReferenceversion attributes:The referenced property names are deduplicated and used to locate the corresponding property elements in the project file.
Update referenced property values
For each discovered version property, the rewriter updates its value to the requested Fallout version.
A NUKE-specific version property is migrated completely.
Before:
After:
The existing MSBuild-property migration renames
NukeVersiontoFalloutVersion, while the new rewrite updates the property's value.Preserve custom property names
Projects are not required to name their shared version property
NukeVersion.When a custom property is used, its name is preserved and only its value is updated.
Before:
After:
This keeps the project's existing version-management structure intact while ensuring that migrated Fallout packages use the correct version.
Leave unrelated properties untouched
Only properties referenced by a
PackageReferenceversion attribute are selected for updating.Other version-like properties in the same project are left unchanged, avoiding broad replacements based solely on property names or values.
Migration behavior
$(NukeVersion)FalloutVersion, and the property value is updatedTests
Added regression coverage for the scenarios reported in #477:
NukeVersionproperty is renamed toFalloutVersion.$(NukeVersion)are updated to use$(FalloutVersion).Result
Running
fallout-migratenow produces a consistent.csprojfor projects that manage NUKE package versions through MSBuild properties.Both the package references and the property supplying their version are migrated automatically, without requiring users to manually repair the project afterward.
🤖 helped out here :D