Skip to content

Port Microsoft.Extensions.* from pkgproj to dotnet pack #48385

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
Closed
Anipik wants to merge 6 commits into from
Closed

Port Microsoft.Extensions.* from pkgproj to dotnet pack #48385

Anipik wants to merge 6 commits into from

Conversation

@Anipik
Copy link
Contributor

@Anipik Anipik commented Feb 17, 2021 •
edited
Loading

Design Changes

  • no more pkgproj
  • separate symbol nuspec files
  • minor change in metadata like no owners (Metadata not supported by the nuget pack)
  • nuspec name contains package version
  • no more pkgpath files (not sure why we were producing it in the first place)
  • xml files comes from the build and not from intellisense package (cc @safern is that fine ?)
  • no more package reports
  • removed implicit framework assembly references from the package
  • symbol package path will be same as package path
  • exclude="Build,Analyzers" for all dependencies

Validation changes

  • Closure verification in the all config
  • Package validation during build packages is no longer done as it was not doing any substantial work for extensions packages.

@ghost
Copy link

ghost commented Feb 17, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ViktorHofer
ViktorHofer reviewed Feb 17, 2021
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 17, 2021
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 17, 2021
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 17, 2021
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 17, 2021
View reviewed changes
ViktorHofer
ViktorHofer reviewed Feb 17, 2021
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Can you please also inline the PackageDescription from the leaf's Directory.Build.props file into the csproj files that are now packable?

@ViktorHofer
Copy link
Member

ViktorHofer commented Feb 17, 2021

no more pkgpath files (not sure why we were producing it in the first place)

What's a pkgpath file?

minor change in metadata like no owners (Metadata not supported by the nuget pack)

Can you please share a diff of the nuspecs before and after?

eerhardt
eerhardt reviewed Feb 17, 2021
View reviewed changes
eerhardt
eerhardt reviewed Feb 17, 2021
View reviewed changes
...crosoft.Extensions.Caching.Abstractions/src/Microsoft.Extensions.Caching.Abstractions.csproj
<PropertyGroup>
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
<EnableDefaultItems>true</EnableDefaultItems>
<IsPackable>true</IsPackable>
Copy link
Member

@eerhardt eerhardt Feb 17, 2021

Choose a reason for hiding this comment

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

Can we instead default this in a common props/targets file?

<IsPackable Condition="ProjectName.StartsWith('Microsoft.Extensions.') AND $(IsSourceProject)">true</IsPackable>

Copy link
Member

@ViktorHofer ViktorHofer Mar 23, 2021

Choose a reason for hiding this comment

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

IMO I would prefer this property to be explicit

@Anipik
Copy link
Contributor Author

Anipik commented Feb 18, 2021

Sample diff, we will have a different nuspec file for symbols packages.

 <EF><BB><BF><?xml version="1.0" encoding="utf-8"?>
-<package xmlns="http://schemas.microsoft.com/packaging/2013/01/nuspec.xsd">
-  <metadata minClientVersion="2.12">
+<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd">
+  <metadata>
     <id>Microsoft.Extensions.Configuration.Binder</id>
     <version>6.0.0-dev</version>
     <title>Microsoft.Extensions.Configuration.Binder</title>
     <authors>Microsoft</authors>
-    <owners>microsoft,dotnetframework</owners>
     <requireLicenseAcceptance>false</requireLicenseAcceptance>
     <license type="expression">MIT</license>
     <licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>
     <icon>Icon.png</icon>
     <projectUrl>https://github.com/dotnet/runtime</projectUrl>
-    <iconUrl>http://go.microsoft.com/fwlink/?LinkID=288859</iconUrl>
     <description>Functionality to bind an object to data in configuration providers for Microsoft.Extensions.Configuration.</description>
     <releaseNotes>https://go.microsoft.com/fwlink/?LinkID=799421</releaseNotes>
     <copyright><C2><A9> Microsoft Corporation. All rights reserved.</copyright>
-    <serviceable>true</serviceable>
     <repository type="git" url="git://github.com/dotnet/runtime" commit="0000000000000000000000000000000000000000" />
     <dependencies>
       <group targetFramework=".NETFramework4.6.1">
-        <dependency id="Microsoft.Extensions.Configuration.Abstractions" version="6.0.0-dev" />
+        <dependency id="Microsoft.Extensions.Configuration.Abstractions" version="6.0.0-dev" exclude="Build,Analyzers" />
       </group>
       <group targetFramework=".NETStandard2.0">
-        <dependency id="Microsoft.Extensions.Configuration.Abstractions" version="6.0.0-dev" />
+        <dependency id="Microsoft.Extensions.Configuration.Abstractions" version="6.0.0-dev" exclude="Build,Analyzers" />
       </group>
     </dependencies>
-    <frameworkAssemblies>
-      <frameworkAssembly assemblyName="mscorlib" targetFramework=".NETFramework4.6.1" />
-      <frameworkAssembly assemblyName="System" targetFramework=".NETFramework4.6.1" />
-      <frameworkAssembly assemblyName="System.Core" targetFramework=".NETFramework4.6.1" />
-    </frameworkAssemblies>
   </metadata>
   <files>
-    <file src="C:\Users\anagniho\.nuget\packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21105.12\tools\Assets\DotNetPackageIcon.png" target="Icon.png" exclude="" />
-    <file src="C:\git\runtime5\artifacts\bin\Microsoft.Extensions.Configuration.Binder\net461-Debug\Microsoft.Extensions.Configuration.Binder.dll" target="lib\net461" exclude="" />
-    <file src="C:\git\runtime5\artifacts\bin\Microsoft.Extensions.Configuration.Binder\net461-Debug\Microsoft.Extensions.Configuration.Binder.pdb" target="lib\net461" exclude="" />
-    <file src="C:\Users\anagniho\.nuget\packages\microsoft.private.intellisense\5.0.0-preview-20201009.2\IntellisenseFiles\net\1033\Microsoft.Extensions.Configuration.Binder.xml" target="lib\net461" exclude="" />
-    <file src="C:\git\runtime5\artifacts\bin\Microsoft.Extensions.Configuration.Binder\netstandard2.0-Debug\Microsoft.Extensions.Configuration.Binder.dll" target="lib\netstandard2.0" exclude="" />
-    <file src="C:\git\runtime5\artifacts\bin\Microsoft.Extensions.Configuration.Binder\netstandard2.0-Debug\Microsoft.Extensions.Configuration.Binder.pdb" target="lib\netstandard2.0" exclude="" />
-    <file src="C:\Users\anagniho\.nuget\packages\microsoft.private.intellisense\5.0.0-preview-20201009.2\IntellisenseFiles\net\1033\Microsoft.Extensions.Configuration.Binder.xml" target="lib\netstandard2.0" exclude="" />
-    <file src="C:\git\runtime5\LICENSE.TXT" target="LICENSE.TXT" exclude="" />
-    <file src="C:\git\runtime5\THIRD-PARTY-NOTICES.TXT" target="THIRD-PARTY-NOTICES.TXT" exclude="" />
+    <file src="C:\git\runtime4\artifacts\bin\Microsoft.Extensions.Configuration.Binder\net461-Debug\Microsoft.Extensions.Configuration.Binder.dll" target="lib\net461\Microsoft.Extensions.Configuration.Binder.dll" />
+    <file src="C:\git\runtime4\artifacts\bin\Microsoft.Extensions.Configuration.Binder\net461-Debug\Microsoft.Extensions.Configuration.Binder.xml" target="lib\net461\Microsoft.Extensions.Configuration.Binder.xml" />
+    <file src="C:\git\runtime4\artifacts\bin\Microsoft.Extensions.Configuration.Binder\netstandard2.0-Debug\Microsoft.Extensions.Configuration.Binder.dll" target="lib\netstandard2.0\Microsoft.Extensions.Configuration.Binder.dll" />
+    <file src="C:\git\runtime4\artifacts\bin\Microsoft.Extensions.Configuration.Binder\netstandard2.0-Debug\Microsoft.Extensions.Configuration.Binder.xml" target="lib\netstandard2.0\Microsoft.Extensions.Configuration.Binder.xml" />
+    <file src="C:\Users\anagniho\.nuget\packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21105.12\tools\Assets\DotNetPackageIcon.png" target="Icon.png" />
+    <file src="C:\git\runtime4\LICENSE.TXT" target="LICENSE.TXT" />
+    <file src="C:\git\runtime4\THIRD-PARTY-NOTICES.TXT" target="THIRD-PARTY-NOTICES.TXT" />
   </files>
 </package>
\ No newline at end of file

ViktorHofer
ViktorHofer reviewed Feb 18, 2021
View reviewed changes
...ies/Microsoft.Extensions.Configuration.Xml/src/Microsoft.Extensions.Configuration.Xml.csproj
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Configuration.FileExtensions\src\Microsoft.Extensions.Configuration.FileExtensions.csproj" />
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.FileProviders.Abstractions\src\Microsoft.Extensions.FileProviders.Abstractions.csproj" />
<ProjectReference Include="$(LibrariesProjectRoot)System.Security.Cryptography.Xml\src\System.Security.Cryptography.Xml.csproj" />
<ProjectReference Include="$(LibrariesProjectRoot)System.Security.Cryptography.Xml\src\System.Security.Cryptography.Xml.csproj" Condition="!$(TargetFramework.StartsWith('net4'))" />
Copy link
Member

@ViktorHofer ViktorHofer Feb 18, 2021

Choose a reason for hiding this comment

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

Suggested change
<ProjectReference Include="$(LibrariesProjectRoot)System.Security.Cryptography.Xml\src\System.Security.Cryptography.Xml.csproj" Condition="!$(TargetFramework.StartsWith('net4'))" />
<ProjectReference Include="$(LibrariesProjectRoot)System.Security.Cryptography.Xml\src\System.Security.Cryptography.Xml.csproj" Condition="'$(TargetFramework)' == 'netstandard2.0'" />

ViktorHofer
ViktorHofer reviewed Feb 18, 2021
View reviewed changes
...s/System.ServiceProcess.ServiceController/ref/System.ServiceProcess.ServiceController.csproj
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\System.Diagnostics.EventLog\ref\System.Diagnostics.EventLog.csproj" />
<ProjectReference Include="..\..\System.Diagnostics.EventLog\ref\System.Diagnostics.EventLog.csproj" Condition="!$(TargetFramework.StartsWith('net4'))" />
Copy link
Member

@ViktorHofer ViktorHofer Feb 18, 2021

Choose a reason for hiding this comment

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

Suggested change
<ProjectReference Include="..\..\System.Diagnostics.EventLog\ref\System.Diagnostics.EventLog.csproj" Condition="!$(TargetFramework.StartsWith('net4'))" />
<ProjectReference Include="..\..\System.Diagnostics.EventLog\ref\System.Diagnostics.EventLog.csproj" Condition="'$(TargetFramework)' == 'netstandard2.0'" />

ViktorHofer
ViktorHofer reviewed Feb 18, 2021
View reviewed changes
src/libraries/libraries-packages.proj

<PropertyGroup>
<TraversalGlobalProperties>BuildAllProjects=true</TraversalGlobalProperties>
<TraversalGlobalProperties>BuildAllProjects=true;BuildTargetFramework=</TraversalGlobalProperties>
Copy link
Member

@ViktorHofer ViktorHofer Feb 18, 2021

Choose a reason for hiding this comment

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

why is this change necessary?

@ViktorHofer
Copy link
Member

ViktorHofer commented Feb 18, 2021

Curious, why is <Serviceable>true</Serviceable> removed in the nuspec? Arcade sets it to true: https://github.com/dotnet/arcade/blob/99b9f805da070d301d76eac578a46dd75d9e5f7c/src/Microsoft.DotNet.Arcade.Sdk/tools/ProjectDefaults.props#L14. Is it omitted in case it's true?

This was referenced Feb 26, 2021
Closed
Merged
Base automatically changed from master to main March 1, 2021 09:07
@ghost
Copy link

ghost commented Mar 22, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Design Changes

  • no more pkgproj
  • separate symbol nuspec files
  • minor change in metadata like no owners (Metadata not supported by the nuget pack)
  • nuspec name contains package version
  • no more pkgpath files (not sure why we were producing it in the first place)
  • xml files comes from the build and not from intellisense package (cc @safern is that fine ?)
  • no more package reports
  • removed implicit framework assembly references from the package
  • symbol package path will be same as package path
  • exclude="Build,Analyzers" for all dependencies

Validation changes

  • Closure verification in the all config
  • Package validation during build packages is no longer done as it was not doing any substantial work for extensions packages.
Author: Anipik
Assignees: -
Labels:

area-Infrastructure-libraries
Milestone: -

@ViktorHofer ViktorHofer added the enhancement Product code improvement that does NOT require public API changes/additions label Mar 23, 2021
ViktorHofer
ViktorHofer reviewed Mar 23, 2021
View reviewed changes
eng/packaging.props
</PropertyGroup>

<PropertyGroup>
<NuSpecOutputPath Condition="'$(NuSpecOutputPath)' == ''">$([MSBuild]::NormalizeDirectory('$(ArtifactsPackagesDir)', 'specs'))</NuSpecOutputPath>
Copy link
Member

@ViktorHofer ViktorHofer Mar 23, 2021

Choose a reason for hiding this comment

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

Are nuspecs generated by default by NuGet when invoking their Pack task?

Copy link
Contributor Author

@Anipik Anipik Mar 24, 2021

Choose a reason for hiding this comment

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

Yes, by default its in the obj directory.

@ericstj
Copy link
Member

ericstj commented Apr 15, 2021

What's the status of this PR? It would have been nice to have as it would save some complexity in #51064.

@Anipik
Copy link
Contributor Author

Anipik commented Apr 15, 2021

Its currently blocked on this NuGet/NuGet.Client#3980 i need to fix the tests in the nuget pr

@ericstj
Copy link
Member

ericstj commented Apr 16, 2021

Its currently blocked on this NuGet/NuGet.Client#3980 i need to fix the tests in the nuget pr

Do we have an issue on our side somewhere describing why we need that or how we intend to use it? Trying to connect the dots here.

@ViktorHofer
Copy link
Member

ViktorHofer commented Apr 16, 2021

@Anipik can't you just in the meantime override the property via a target? There are places where we already do stuff in a target fo packaging related settings/validations.

@Anipik Anipik mentioned this pull request Apr 23, 2021
Merged
@Anipik
Copy link
Contributor Author

Anipik commented Apr 23, 2021

closing this in favor of #51765

@Anipik Anipik closed this Apr 23, 2021
@Anipik Anipik deleted the portExtensions branch April 30, 2021 01:09
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ViktorHofer ViktorHofer ViktorHofer left review comments

@eerhardt eerhardt eerhardt left review comments

@ericstj ericstj Awaiting requested review from ericstj

@safern safern Awaiting requested review from safern

Assignees

No one assigned

Labels

area-Infrastructure-libraries enhancement Product code improvement that does NOT require public API changes/additions

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@Anipik @ViktorHofer @ericstj @eerhardt @safern @karelz @maryamariyan