Skip to content
This repository was archived by the owner on Feb 25, 2021. It is now read-only.

Switch to embedded PackageIcon #1896

Closed
wants to merge 2 commits into from
Closed

Switch to embedded PackageIcon #1896

wants to merge 2 commits into from

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Sep 23, 2019

Part of aspnet/AspNetCore-Internal#3146

Local build passes the smell test with these changes. I'm still unsure on whether or not we need to include a reference to the package icon in the .nuspec - the "old way" did so automatically, the "new way" does not (https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#packing-an-icon-image-file). CC @nkolev92 who may know more about that question.

@wtgodbe wtgodbe requested a review from a team September 23, 2019 22:14
@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 23, 2019

Looks like we don't need to worry about the .nuspec, as per dotnet/ef6#1286 (comment)

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

@wtgodbe please let me know when the missing property is in the .nuspec files and you've confirmed the final packages

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 24, 2019

With the last change, this now includes the icon metadata in all .nuspec's, including the manually generated ones - @dougbu PTAL

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Looks good but should target 'release/3.0'

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 24, 2019

Closing in favor of #1897

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants