Clarify netstandard2.0 reference-only support in MSBuild packages#14409
Clarify netstandard2.0 reference-only support in MSBuild packages#14409JanKrivanek wants to merge 1 commit into
Conversation
6574090 to
d148758
Compare
|
/review |
|
✅ Expert Code Review (command) completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR improves documentation and warning message clarity. Overall it looks good with one substantive concern about the warning message length.
Findings
Medium: Warning message is excessively long (eng/packaging.targets)
The new warning text is ~600 characters. MSBuild warnings are typically shown in IDE error lists and CI logs where long messages get truncated or become hard to scan. The old message was ~280 characters. Consider keeping the first 1-2 sentences as the warning text and directing users to documentation for full guidance (e.g., append a docs link).
This is not a breaking change since the warning code and triggering conditions are unchanged - only text is modified. Users with WarnAsError who already suppressed this warning will be unaffected.
Low: Version table accuracy (documentation/consuming-nuget-package.md)
The table states MSBuild 18.x ships lib/net10.0. This is forward-looking and accurate given .NET 10 is the current development target.
Documentation Quality
The new documentation section is well-structured, technically accurate, and covers both the extension-author and standalone-app scenarios with actionable guidance. The PackageReference example with PrivateAssets and ExcludeAssets is helpful.
Verdict
No blocking issues. The main suggestion is to shorten the warning message and link to documentation instead of embedding the full explanation inline.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
awmgmcpg
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
Generated by Expert Code Review (command) for #14409 · 43.2 AIC · ⌖ 7.37 AIC · ⊞ 4.5K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
This PR clarifies how the Microsoft.Build.* NuGet packages support netstandard2.0 as a reference-only (compile-time) asset, while shipping runtime implementations only for the band’s single .NET TFM (plus net472). This is intended to reduce confusion when projects compile successfully on other TFMs but fail at runtime due to missing runtime assets.
Changes:
- Updated the
NETStandardCompatErrorwarning text to explicitly explain the “compile-only ref/netstandard2.0, no runtime lib/ for that TFM” model and what it implies at runtime. - Added documentation describing the “one runtime per band” packaging model and how/why
ref/netstandard2.0is provided withoutlib/netstandard2.0.
Show a summary per file
| File | Description |
|---|---|
| eng/packaging.targets | Expands the existing compatibility warning to explicitly describe reference-only netstandard2.0 and host-provided runtime behavior. |
| documentation/consuming-nuget-package.md | Adds a new section documenting runtime-asset-per-band behavior and the purpose/limitations of ref/netstandard2.0. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
| The MSBuild packages are the build engine that ships **inside** the .NET SDK and | ||
| Visual Studio. They are not general-purpose libraries meant to be redistributed | ||
| and run on an arbitrary runtime. This shapes how the packages are laid out and | ||
| which target frameworks they "support." |
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Thanks for the clarification @JanKrivanek!
There was a problem hiding this comment.
I approved too soon. I thought this was updating a skill before I saw the targets change.
For context: The existing message is shared with other repositories, mainly BCL packages. I'm OK with stepping away from that to provide more clarity. What I dislike is the quite verbose message now.
@rainersigwald and I were also talking about only emitting that warning when runtime assets are consumed, meaning when someone does not specify ExcludeAssets="runtime" / IncludeAssets="compile;build;buildTransitive".
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
@rainersigwald and I were also talking about only emitting that warning when runtime assets are consumed, meaning when someone does not specify
ExcludeAssets="runtime"/IncludeAssets="compile;build;buildTransitive".
Honestly I would contemplate warning in that case unconditionally . ..
| <Target Name="$(_NETStandardCompatErrorFileTarget)" | ||
| Condition="'%24(SuppressTfmSupportBuildWarnings)' == ''"> | ||
| <Warning Text="$(PackageId) $(PackageVersion) doesn't support %24(TargetFramework) and has not been tested with it. Consider upgrading your TargetFramework to %(NETStandardCompatError.Supported) or later. You may also set <%3BSuppressTfmSupportBuildWarnings>%3Btrue<%3B/SuppressTfmSupportBuildWarnings>%3B in the project file to ignore this warning and attempt to run in this unsupported configuration at your own risk." /> | ||
| <Warning Text="$(PackageId) $(PackageVersion) does not ship a runtime assembly for %24(TargetFramework). It provides a runtime implementation only for %(NETStandardCompatError.Supported) or later (and .NET Framework), plus a netstandard2.0 reference assembly that is for compilation only. Your project will compile against that reference assembly, but no runtime assembly is copied to your output, so the assembly will fail to load at run time unless an MSBuild host provides it (for example when your code runs as an MSBuild task, logger, or analyzer inside MSBuild). If your code runs standalone, retarget to %(NETStandardCompatError.Supported) or later. If it is a compile-only MSBuild extension loaded by an MSBuild host, reference this package with compile-only assets such as PrivateAssets=all and ExcludeAssets=runtime, then set <%3BSuppressTfmSupportBuildWarnings>%3Btrue<%3B/SuppressTfmSupportBuildWarnings>%3B to acknowledge this configuration and silence this warning." /> |
There was a problem hiding this comment.
I don't like the "no runtime assembly is copied to your output, so the assembly will fail to load at run time unless an MSBuild host provides it" -- that's the normal case. I can't think of a supported case for copying the assemblies locally, actually . . .
Summary
Docs/messaging only — no engine or packaging-behavior change.
The
Microsoft.Build.*packages ship a compile-onlyref/netstandard2.0reference assembly (nolib/netstandard2.0) and a runtime asset only for the band's single .NET TFM (net10.0today) plusnet472. Projects on other TFMs (e.g.net8.0) compile but get no runtime asset, which is easily misread as a broken package.eng/packaging.targets— clarified theNETStandardCompatErrorwarningdocumentation/consuming-nuget-package.md— documented the one-runtime-per-band model and the compile-onlyref/netstandard2.0design.Prompted by confusion in microsoft/testfx#9998.
(Coauthored by Copilot)