-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Remove EFCore.Analyzers.nuspec and use MSBuild packaging #37456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fdd6351 to
4ab0b0b
Compare
|
Verified that the nupkg produced by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses an issue where BaGet server rejected the Microsoft.EntityFrameworkCore.Analyzers package due to backslash path separators in the nuspec file. The solution eliminates the hand-authored nuspec file and migrates to MSBuild-based packaging, which uses forward slashes consistently.
Key Changes:
- Removed the manually authored
EFCore.Analyzers.nuspecfile - Migrated all packaging configuration to
EFCore.Analyzers.csprojusing MSBuild properties and<None>items - Changed path separators from backslashes to forward slashes for package paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/EFCore.Analyzers/EFCore.Analyzers.nuspec | Removed hand-authored nuspec file that contained backslash path separators causing BaGet rejection |
| src/EFCore.Analyzers/EFCore.Analyzers.csproj | Added MSBuild packaging configuration including analyzer DLL/PDB packaging and readme file, with forward slash paths; includes commented-out analyzer-specific properties |
Head branch was pushed to by a user without write access
| <!-- Package the analyzer DLL and PDB in the analyzers/dotnet/cs directory --> | ||
| <ItemGroup> | ||
| <None Include="$(OutputPath)$(AssemblyName).dll" Pack="true" PackagePath="analyzers/dotnet/cs" /> | ||
| <None Include="$(OutputPath)$(AssemblyName).pdb" Pack="true" PackagePath="analyzers/dotnet/cs" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .pdb shouldn't be included in the main package, only in the symbols one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch.
Getting around this proved remarkably difficult: when copying files into the package manually, it doesn't seem possible to make a distinction between the main nupkg and the symbol nupkg. Take a look at the slightly hacky thing I came up with; if you think that's bad we can always keep the current nuspec-based way and just move the README.md to the standard base location (no more slashes needed).
BTW I'm a bit unsure what a PDB/symbol package is even good for, with analyzers. As this isn't a build dependency nobody is going to debug into it from an EF application (not sure it's possible to debug into an analyzer in general...). It feels like it may be fine to simply not have a symbol package/omit the PDB; it also feels maybe OK to just include the PDB in the main package too (it's just 16k).
Finally, I'm noticing that in general, we're still building the old/legacy symbol package format (symbols.nupkg) rather than the new snupkg. I suspect maybe with snupkg this is all simple and just works - I'm not sure if there's a plan to switch to the new format at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting around this proved remarkably difficult: when copying files into the package manually, it doesn't seem possible to make a distinction between the main nupkg and the symbol nupkg. Take a look at the slightly hacky thing I came up with; if you think that's bad we can always keep the current nuspec-based way and just move the README.md to the standard base location (no more slashes needed).
Looks good enough
BTW I'm a bit unsure what a PDB/symbol package is even good for, with analyzers. As this isn't a build dependency nobody is going to debug into it from an EF application (not sure it's possible to debug into an analyzer in general...). It feels like it may be fine to simply not have a symbol package/omit the PDB; it also feels maybe OK to just include the PDB in the main package too (it's just 16k).
Including symbols is a requirement even if their usage is implausible, I am not sure about where we stand on shipping them in the main package.
Finally, I'm noticing that in general, we're still building the old/legacy symbol package format (symbols.nupkg) rather than the new snupkg. I suspect maybe with snupkg this is all simple and just works - I'm not sure if there's a plan to switch to the new format at some point.
61aa00d to
8e3a338
Compare
Plan: Remove EFCore.Analyzers.nuspec and use MSBuild packaging
analyzers/dotnet/cs/pathanalyzers/dotnet/cs/pathSummary
Successfully eliminated the EFCore.Analyzers.nuspec file and configured MSBuild packaging. The generated package now has:
docs/PACKAGE.mdinstead ofdocs\PACKAGE.md)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.