-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add targets to generate the list of shared framework assemblies from project property #7510
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
… the IsAspNetCoreApp project property
…s in the shared framework
Directory.Build.props
Outdated
@@ -154,6 +154,7 @@ | |||
<Import Project="eng\Dependencies.props" /> | |||
<Import Project="eng\PatchConfig.props" /> | |||
<Import Project="eng\ProjectReferences.props" /> | |||
<Import Project="src\Framework\Microsoft.AspNetCore.App.props" /> |
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.
Not the greatest location for common properties. Why not keep it in eng?
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.
Wasn't really thinking about that - i just left the file where it was. I'll move to eng/...that's a better place for it.
but in some edge cases, this is not reasonable. | ||
--> | ||
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'"> | ||
<_FrameworkProvidedReference Include="System.Buffers" /> |
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.
I don't completely understand this. Is this just to avoid producing errors about referencing System.Buffers
?
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.
Can we flow this information via ExternalAspNetCoreAppReference somehow?
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.
Correct. A few signalR assemblies target netstandard2.0 and need the System.Buffers reference, but this dependency is knocked out in netcoreapp3.0. I wanted to provide a way for us to require explicit action to put new assemblies into the shared framework.
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.
I updated the name and comments to clarify that this is a "compilation only reference". This list should be fairly stable since netstandard2.0 is set in stone. The only thing which might change is what is in netcoreapp3.0.
@@ -75,6 +80,27 @@ | |||
<Reference Remove="@(Reference)" /> | |||
</ItemGroup> | |||
|
|||
<ItemDefinitionGroup> | |||
<Reference> | |||
<SharedSources>false</SharedSources> |
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.
Can this be conditional on reference name? Or can we define this on LatestPackageReference
definition level?
These packages are never going to be consumed in any other way so it would be nice to avoid declaring SharedSources on every reference
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.
I would love to do this, but I don't know of a simple way in MSBuild to write an expression that does this: Reference.Where(i => i.Identity.EndsWith(".Sources")).SetMetadata('PrivateAssets', 'All')
. We might be able to do this as an MSBuild task, but every other approach I've tried is expensive computationally.
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.
You can set SharedSource metadata in https://github.com/aspnet/AspNetCore/pull/7510/files#diff-51f57d74bf38501ecf1b2a82e10577d7R121. Should be enough for the purposes you are using it for.
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.
That's where I would put it, but this isn't valid MSBuild syntax...
<Reference>
<PrivateAssets Condition="%(Identity.EndsWith('.Sources'))">All</PrivateAssets>
</Reference>
Let me play with some MSBuild intrinsic functions and see if I can find a workaround.
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.
Minor comments
This will help later on when implementing reference assemblies,
FrameworkReference
, and to add checks which assert things references are used correctly. (see prototype in natemcmaster/AspNetCore@vs2019...local-fx)Changes:
IsAspNetCoreApp
, in the .csproj file of assemblies which are part of the shared framework.Reviewer: Split into several commits to make this easier to review