[7.1.0-preview1] Fix TFM Attributes#4254
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a release-build regression where multi-target builds could embed an incorrect TargetFrameworkAttribute by removing the BuildForRelease=true override that collapsed TargetFrameworkMonikerAssemblyAttributesPath to a shared path.
Changes:
- Remove the
TargetFrameworkMonikerAssemblyAttributesPathoverride fromsrc/Directory.Build.propsforBuildForRelease=truebuilds. - Update the repo solution (
.slnx) to include aDirectory.Build.targetsentry under/src/.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient.slnx | Adds Directory.Build.targets to the solution items under /src/. |
| src/Directory.Build.props | Removes the release-only override that could cause cross-TFM overwrites of generated assembly-attribute files. |
paulmedynski
left a comment
There was a problem hiding this comment.
The TFM removal looks good. Not sure what the targets file in the solution is for, but it won't hold this PR up.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4254 +/- ##
==========================================
- Coverage 66.02% 64.23% -1.79%
==========================================
Files 277 272 -5
Lines 42988 65783 +22795
==========================================
+ Hits 28382 42257 +13875
- Misses 14606 23526 +8920
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Investigation and fix was done via 🤖 Here's it's explanation of the issue and the proposed solution:
We investigated the TFM mismatch seen in the Microsoft.Data.SqlClient release artifacts and package. The short version is that this does not look like a nuspec bug or an ESRP signing bug.
What we verified:
We traced this to a recent release-only build change in src/Directory.Build.props. Specifically, when BuildForRelease=true, we override
TargetFrameworkMonikerAssemblyAttributesPath. In the normal build path, that property resolves to separate per-TFM files like:
With BuildForRelease=true, it instead collapses to a shared path:
That means multiple target frameworks can reuse/overwrite the same generated assembly-attributes file, which explains how the final binaries can end up with the wrong TargetFrameworkAttribute while still otherwise being the correct target-specific build.
Recommendation:
I would not treat this as safe to ignore, even for a preview release. It is probably not catastrophic for NuGet asset selection or normal runtime behavior, because the actual binaries and references appear correct. But it is still a real release-quality issue:
My recommendation is to fix this before release if at all possible. The likely fix is to remove or move that TargetFrameworkMonikerAssemblyAttributesPath override so it is evaluated later, after per-target intermediate paths are established. If schedule pressure is extreme, I would at minimum block on a quick validation build after changing that override, because this looks like a deterministic regression introduced by the release-only build path rather than a benign display issue.
Ben again, I don't even think we need this file at all, but I do not have the bandwidth at the moment to validate it will still report as deterministic if I just delete it. But the 🤖 says this is safer, so we can go with it for now.