Skip to content

fix: pass assembly version properties to dotnet pack#5274

Merged
thomhurst merged 1 commit intomainfrom
fix/pack-assembly-version-properties
Mar 28, 2026
Merged

fix: pass assembly version properties to dotnet pack#5274
thomhurst merged 1 commit intomainfrom
fix/pack-assembly-version-properties

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • DisableGitVersionTask=true was added in perf: disable GitVersion MSBuild task globally #5266 to eliminate 153+ GitVersion process spawns per build, but PackTUnitFilesModule was not updated to pass version properties explicitly
  • dotnet pack rebuilds assemblies without AssemblyVersion/FileVersion/InformationalVersion, resulting in default versions (1.0.0.0) in the NuGet packages
  • Consumers hit FileNotFoundException at runtime due to strong-name version mismatch (e.g. looking for TUnit.Core, Version=1.19.74.0 but finding 1.0.0.0)
  • Now passes AssemblyVersion, FileVersion, InformationalVersion, and Version to dotnet pack, matching what the CI build step already does

Reproducer: https://github.com/thomhurst/Dekaf/actions/runs/23673435711/job/68971685700?pr=647

Test plan

  • CI builds and packs successfully
  • Verify NuGet package assemblies have correct AssemblyVersion (not 1.0.0.0)
  • Consumer projects can load TUnit without FileNotFoundException

DisableGitVersionTask=true was added in #5266 but PackTUnitFilesModule
was not updated to pass version properties explicitly. This caused
dotnet pack to rebuild assemblies with default AssemblyVersion (1.0.0.0),
creating strong-name version mismatches at runtime in consumers
(FileNotFoundException for TUnit.Core).
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: fix: pass assembly version properties to dotnet pack

Summary

This is a correct and focused bug fix. The root cause analysis is accurate — disabling DisableGitVersionTask globally in #5266 was the right performance call, but dotnet pack rebuilds assemblies as part of its process, and without the version properties being forwarded explicitly, they default to 1.0.0.0. This produces a strong-name version mismatch that breaks runtime loading for any consumer using strong-named assemblies. The fix is straightforward and mirrors exactly what the CI build step was already doing.

No prior review comments exist on this PR.


What's Good

Completeness of the fix. All four properties that affect assembly and package identity are forwarded: AssemblyVersion, FileVersion, InformationalVersion, and Version. This matches the set passed during dotnet build in the CI workflow, making the two steps symmetric and consistent. Forgetting any one of these (especially AssemblyVersion) would reproduce the original runtime failure.

Symmetry with the CI build step. The GitHub Actions workflow already passes this exact set of properties to dotnet build. This PR brings dotnet pack into alignment, which is the right mental model — pack should never diverge from build in how it stamps version information.

Removal of the now-stale comment. The old comment claimed "GitVersion.MsBuild handles AssemblyVersion/FileVersion automatically" — that was only true when DisableGitVersionTask=false. Removing it prevents future confusion about why the properties need to be explicit.


Observations and Suggestions

1. Beta packages get a modified PackageVersion but Version/AssemblyVersion always use SemVer (not the beta-suffixed value).

For beta packages, packageVersion is "{SemVer}-beta", but Version is set to version.SemVer! (no suffix). This means the NuGet package metadata shows PackageVersion=1.x.x-beta while the embedded Version MSBuild property is 1.x.x. In most cases this is intentional — you want AssemblyVersion to be a clean numeric value, not a pre-release string. But it's worth confirming this is deliberate, since MSBuild's Version property influences the AssemblyVersion default if AssemblyVersion is not set independently.

Since AssemblyVersion is now set explicitly, there's no functional issue here. Just making sure the intent is documented or understood.

2. The !-null-forgiving operators suppress potential null warnings on all four fields.

new KeyValue("AssemblyVersion", version.AssemblySemVer!),
new KeyValue("FileVersion", version.AssemblySemFileVer!),
new KeyValue("InformationalVersion", version.InformationalVersion!),
new KeyValue("Version", version.SemVer!),

These match the existing pattern used elsewhere (e.g. version.SemVer! in the packageVersion calculation above, and in other modules). This is consistent, though a defensive null-check or a guard-clause at the top of ExecuteAsync on versionResult.ValueOrDefault would make failures more diagnosable if GitVersion ever returns partial data. This is a low-priority suggestion given the existing code style.

3. Minor: IncludeSource = project == Sourcy.DotNet.Projects.TUnit_Templates ? false : true (pre-existing).

This is unrelated to the PR but visible in context — != Sourcy.DotNet.Projects.TUnit_Templates or project is not Sourcy.DotNet.Projects.TUnit_Templates would be more idiomatic than ? false : true. Not a blocker and out of scope for this fix.


Verdict

The fix is correct, minimal, and addresses the exact regression introduced by #5266. The approach (explicitly forwarding version properties to dotnet pack) is the right solution — it's the standard pattern for this class of problem when GitVersion MSBuild integration is disabled. Approved as-is.

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: fix: pass assembly version properties to dotnet pack

This is a correct and focused bug fix. No prior review comments exist on this PR.

Root cause is accurately identified. Disabling DisableGitVersionTask globally in #5266 was the right performance call, but dotnet pack rebuilds assemblies as part of its process. Without version properties forwarded explicitly, they default to 1.0.0.0, producing a strong-name version mismatch that breaks runtime loading for consumers.

What is good:

Completeness of the fix - all four properties affecting assembly and package identity are forwarded: AssemblyVersion, FileVersion, InformationalVersion, and Version. This exactly mirrors what the CI build step passes to dotnet build, making the two steps symmetric.

Removal of the now-stale comment - the old comment claimed 'GitVersion.MsBuild handles AssemblyVersion/FileVersion automatically', which was only true when DisableGitVersionTask=false. Removing it prevents future confusion.

Observations:

  1. Beta packages receive a modified PackageVersion ('{SemVer}-beta') but Version is always set to version.SemVer (no suffix). This is likely intentional since AssemblyVersion must be a clean numeric value - and since AssemblyVersion is now set explicitly, MSBuild's Version default no longer matters. Worth confirming this asymmetry is deliberate if it wasn't documented before.

  2. The null-forgiving operators (!) on all four fields are consistent with the existing pattern throughout the module (e.g. version.SemVer! is already used above). A guard-clause at the top of ExecuteAsync on versionResult.ValueOrDefault would make failures more diagnosable if GitVersion ever returns partial data, but this is low priority given existing code style.

Verdict: The fix is correct, minimal, and uses the right approach - explicitly forwarding version properties to dotnet pack is the standard solution when GitVersion MSBuild integration is disabled. The change is symmetric with what the CI build step already does. No concerns.

@thomhurst thomhurst merged commit 3bfcb58 into main Mar 28, 2026
14 of 16 checks passed
@thomhurst thomhurst deleted the fix/pack-assembly-version-properties branch March 28, 2026 00:58
@claude claude bot mentioned this pull request Mar 28, 2026
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant