Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Automatically reference assembly packs for .NET Framework #10981

Merged
merged 3 commits into from
Mar 27, 2020
Merged

Automatically reference assembly packs for .NET Framework #10981

merged 3 commits into from
Mar 27, 2020

Conversation

sfoslund
Copy link
Member

Fixes #4009

@sfoslund sfoslund requested a review from a team March 24, 2020 22:32
dsplaisted
dsplaisted approved these changes Mar 24, 2020
View reviewed changes
Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

💯

...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets Outdated
</GetReferenceAssemblyPaths>

<ItemGroup>
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" IsImplicitlyDefined="true" Condition="'$(_FullFrameworkReferenceAssemblyPaths)' == ''"/>
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a property here instead of hardcoding 1.0.0 for the version number. How about MicrosoftNETFrameworkReferenceAssembliesLatestPackageVersion?

Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm that you tested restore in VS from a clean project?

My concern is: does VS use a design-time build to extract PackageReference when it does restore, or does it use only evaluated state, which wouldn't see this?

Copy link
Member

Choose a reason for hiding this comment

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

VS does use design-time builds. They would like to be able to restore based only on evaluation, but we add PackageReference and PackageDownload items in targets as a core part of the build for .NET Core, so it's not currently feasible for Sdk-style projects.

rainersigwald
rainersigwald reviewed Mar 25, 2020
View reviewed changes
...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets Outdated
</GetReferenceAssemblyPaths>

<ItemGroup>
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" IsImplicitlyDefined="true" Condition="'$(_FullFrameworkReferenceAssemblyPaths)' == ''"/>
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm that you tested restore in VS from a clean project?

My concern is: does VS use a design-time build to extract PackageReference when it does restore, or does it use only evaluated state, which wouldn't see this?

...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets Outdated
</GetReferenceAssemblyPaths>

<ItemGroup>
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" IsImplicitlyDefined="true" Condition="'$(_FullFrameworkReferenceAssemblyPaths)' == ''"/>
Copy link
Member

Choose a reason for hiding this comment

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

What if the project has an existing reference to this? Does this conflict or supersede?

Copy link
Member Author

Choose a reason for hiding this comment

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

It supersedes, I added test coverage in the latest update: fb3772a#diff-8ba7e75c46a5fe96b147fd79709da01eR116

@sfoslund sfoslund force-pushed the AutoRefTargetPack branch from fb3772a to c6b2f33 Compare March 26, 2020 18:01
@richlander
Copy link
Member

Cool! Just saw this.

Here are two projects that I'll want to test/convert:

@sfoslund sfoslund merged commit 206ec63 into dotnet:master Mar 27, 2020
@sfoslund sfoslund deleted the AutoRefTargetPack branch March 27, 2020 15:29
@sfoslund sfoslund mentioned this pull request Apr 10, 2020
Merged
@andrew-boyarshin andrew-boyarshin mentioned this pull request Apr 24, 2020
Closed
@dsplaisted dsplaisted mentioned this pull request Jan 21, 2021
Open
@dsplaisted dsplaisted mentioned this pull request Oct 6, 2022
Closed
@lytico lytico mentioned this pull request Oct 25, 2022
Closed
@drewnoakes drewnoakes mentioned this pull request Apr 13, 2023
Closed
@0xced 0xced mentioned this pull request Jun 7, 2023
Merged
@LonghronShen LonghronShen mentioned this pull request Feb 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsplaisted dsplaisted dsplaisted approved these changes

@rainersigwald rainersigwald rainersigwald approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automatically reference .NET Framework reference assembly packages if needed
4 participants
@sfoslund @richlander @dsplaisted @rainersigwald