-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Create transport package for API docs #49969
Conversation
...work/Microsoft.Internal.Aspnetcore.DotNetApiDocs.Transport/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
...ore.DotNetApiDocs.Transport/src/Microsoft.Internal.Aspnetcore.DotNetApiDocs.Transport.csproj
Outdated
Show resolved
Hide resolved
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.
If it works, looks good to me :)
Can you please share a built package with me offline to make sure that the package layout is correct?
https://dev.azure.com/dnceng/internal/_build/results?buildId=2240773&view=results will produce an "Official" package we can inspect |
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.
Left some questions.
|
||
This file contains a list of all assemblies shipped from this repo, either via the Shared Framework or Nuget packages (or both). | ||
--> | ||
<Project> |
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.
Pardon my ignorance, but couldn't you just automatically collect the assemblies like we do in the runtime and extensions similar PRs? dotnet/extensions#4254
Or is there another reason that forced you to manually indicate them?
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.
We already have logic in the build that auto-generates files like these (see ProjectReferences.props, etc), so I just piggybacked off of that. We also don't allow explicit PackageReference
or ProjectReference
type references, instead using generic Reference
items which our build system automatically converts to the right type of reference. Doing it this way allows the project to plug into our system more easily.
@@ -0,0 +1,74 @@ | |||
<Project> | |||
<Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" /> |
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.
For my own education, why did you choose to use Sdk instead of NoTargets, like we did in runtime and extensions? dotnet/extensions#4254
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.
Because we rely on the reference resolution logic from the SDK to populate ReferenceAssemblyWithReferencePath
- this file is basically a stripped-down version of Microsoft.Aspnetcore.App.Ref.csproj
. The logic originates from there.
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.
nit: Microsoft.Build.NoTargets allows the use of ReferenceAssemblyWithReferencePath
by setting NoTargetsDoNotReferenceOutputAssemblies=false
.
@@ -0,0 +1,74 @@ | |||
<Project> |
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.
For my own education, why did you use a csproj and not a proj, like we did in runtime and extensions? dotnet/extensions#4254
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.
No strong reason, just matching previous repo patterns
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.
Does changing the extension automatically import additional includes? For example, CSharp-specific ones?
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.
Yes - I'm not sure if we'd get the SDK targets we need if I changed it to .proj 😆
Fixes #49847