-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Inline package descriptions and remove descriptions.json files across the repo #46306
Conversation
Tagging subscribers to this area: @ViktorHofer Issue Details
Contributes to converting pkgprojs to csproj (couldn't fine an issue).
|
17518fe
to
169e922
Compare
src/libraries/Microsoft.Bcl.AsyncInterfaces/Directory.Build.props
Outdated
Show resolved
Hide resolved
69f38df
to
4af3066
Compare
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.
Skimmed through. Overall looks good. 2 notes:
I wonder if the support for this should be in the SDK we use to build packages.
We need to update arcade to remove obsolete stuff:
https://github.com/dotnet/arcade/blob/740732d1c0ddfb18e614bb93f93518f74d89bdca/src/Microsoft.DotNet.Build.Tasks.Packaging/src/build/Packaging.targets#L73
And we need to error out if there is no Description set for a package.
We aren't using any target/task and only setting properties. What would you imagine to be handled by that SDK? |
I'm not planning to remove logic from that SDK as the goal is to replace the entire SDK by a new one. I submitted a PR yesterday to disable some of it's functionality to allow the modifications in this PR: dotnet/arcade@de751cf. That SDK isn't used by dotnet/runtime anymore and @jkoritzinsky will remove it as soon as all consumers have switched to the new one.
I didn't know that this is a requirement. We have numerous conditions in master already that check on if '$(Description)' == '' which made me believe that we allow empty descriptions. EDIT: While iterating on this PR I noticed that the current infra already protects against this. If no If we want to force people setting a description, then this could be easily be achieved via an InitialTarget (which its designed use-case is validation :)). I would not add this as part of this PR as I would condition it on |
I've seen that it is pretty common for people to add a new package and miss the description and the error is pretty helpful. So I think we should add the validation as part of this PR and then update once the IsPackable work is done. We need to disable/remove the task that validates and gets the package description from the descriptions.json file though. |
The existing validation is already sufficient. It throws if neither
That's why I believe we can merge this as-is and then immediately follow-up on the
As said, I already opted-out of that target via a switch and it will only throw if both |
Hello @ViktorHofer! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Validated the nuspecs offline for all different subsets and all looks correct now. |
Please review by commits.
PackageDescription
msbuild property. Storing the data in the leaf's Directory.Build.props file to ensure that both the current .pkgproj and the .csproj files will respect it.I'm validating the change right now offline by comparing the generated nuspecs.
Contributes to converting pkgprojs to csproj (couldn't fine an issue).