Skip to content

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

Merged
merged 5 commits into from
Feb 13, 2019

Conversation

natemcmaster
Copy link
Contributor

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:

  • Add support for a property, IsAspNetCoreApp, in the .csproj file of assemblies which are part of the shared framework.
  • Remove unused dependencies
  • Remove reference which have become part of 'netcoreapp3.0'

Reviewer: Split into several commits to make this easier to review

@natemcmaster natemcmaster requested a review from pakrym February 12, 2019 21:15
@Eilon Eilon added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label Feb 12, 2019
@@ -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" />
Copy link
Contributor

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?

Copy link
Contributor Author

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" />
Copy link
Contributor

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 ?

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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>
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@pakrym pakrym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments

@jkotalik jkotalik removed their request for review February 13, 2019 04:22
@natemcmaster natemcmaster merged commit 3fd8a97 into dotnet:master Feb 13, 2019
@natemcmaster natemcmaster deleted the aspnetcoreapp branch February 13, 2019 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants