Use dotnet pack for other Microsoft.extensions* projects #52084

Anipik · 2021-04-29T23:56:03Z

Related to #51765

removes pkgprojs
enable using dotnet pack task

src/libraries/Microsoft.Extensions.Logging/src/Microsoft.Extensions.Logging.csproj

...t.Extensions.HostFactoryResolver/src/Microsoft.Extensions.HostFactoryResolver.Sources.csproj

src/libraries/Directory.Build.props

ViktorHofer · 2021-04-30T12:43:24Z

@Anipik please add the nuspec diff for all the packages that you are touching as part of this PR.

Anipik · 2021-04-30T17:15:44Z

@Anipik please add the nuspec diff for all the packages that you are touching as part of this PR.

do u want me to attach all the diffs ? most of them are non-interseting and similar to each other

ViktorHofer · 2021-04-30T17:46:21Z

do u want me to attach all the diffs ? most of them are non-interseting and similar to each other

They don't need to inlined as a comment on GH, uploading them in a zip would suffice as well. Usually for minor packaging changes we wouldn't be interested in the nuspec diff but for such a fundamental change a diff makes sense. See it as an additional safeguard to avoid an unexpected regression. It might also help people who come back to this PR in the future to better understand the change.

Anipik · 2021-04-30T18:11:24Z

https://gist.github.com/Anipik/712f51a231dd08899dc4def131933743

here is the diff for all the extensions nuspec.

Anipik · 2021-05-04T04:19:01Z

@ViktorHofer @safern @ericstj can you review this one ?

ViktorHofer · 2021-05-04T09:07:13Z

Thanks for the diff. Here are the things that stand out:

<licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>. LicenseUrl (and the msbuild property PackageLicenseUrl) is deprecated. From a search I couldn't find the place that sets it.
Please again move the PackageDescription properties from the Directory.Build.props file into the src project.
Why are build and analyzer items excluded? exclude="Build,Analyzers"
Is it expected that the xml files are included from the package cache? .nuget\packages\microsoft.private.intellisense\5.0.0-preview-20201009.2\IntellisenseFiles\net\1033\Microsoft.Extensions.Caching.Abstractions.xml
Do we still want/need the servicable element? It's currently removed.

I recommend that we do another review of the produced diff after these points are discussed / addressed.

ericstj · 2021-05-04T15:39:46Z

Why are build and analyzer items excluded? exclude="Build,Analyzers"

That's the default behavior of Pack. I agree it's a bit annoying. I'm OK if we remove it from our packages, or leave as is to be more "normal".

Is it expected that the xml files are included from the package cache? .nuget\packages\microsoft.private.intellisense\5.0.0-preview-20201009.2\IntellisenseFiles\net\1033\Microsoft.Extensions.Caching.Abstractions.xml

CC @safern @carlossanlop. I believe we need to have some form of state that drives this. If a library has done the work to move intellisense to triple-slash then that should be used (new model). If not, then the microsoft.private.intellisense package should be used (old model). So long as we aren't changing it with this PR it is OK.

Do we still want/need the servicable element? It's currently removed.

Yes we want it. It's what causes .NETCore to write breadcrumbs for servicing of OOBs.

Anipik · 2021-05-04T16:55:59Z

Do we still want/need the servicable element? It's currently removed.

dotnet pack add this to the symbols nuspec file and set it to true for the symbols package.

sample one.

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd">
  <metadata>
    <id>Microsoft.Extensions.Caching.Abstractions</id>
    <version>6.0.0-dev</version>
    <authors>Microsoft</authors>
    <license type="expression">MIT</license>
    <licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>
    <icon>Icon.png</icon>
    <projectUrl>https://dot.net/</projectUrl>
    <description>Caching abstractions for in-memory cache and distributed cache.

Commonly Used Types:
Microsoft.Extensions.Caching.Distributed.IDistributedCache
Microsoft.Extensions.Caching.Memory.IMemoryCache</description>
    <releaseNotes>https://go.microsoft.com/fwlink/?LinkID=799421</releaseNotes>
    <copyright>© Microsoft Corporation. All rights reserved.</copyright>
    <serviceable>true</serviceable>
    <repository type="git" url="https://github.com/dotnet/runtime" commit="0000000000000000000000000000000000000000" />
    <dependencies>
      <group targetFramework=".NETFramework4.6.1">
        <dependency id="Microsoft.Extensions.Primitives" version="6.0.0-dev" exclude="Build,Analyzers" />
      </group>
      <group targetFramework=".NETStandard2.0">
        <dependency id="Microsoft.Extensions.Primitives" version="6.0.0-dev" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
  <files>
    <file src="C:\git\runtime4\artifacts\bin\Microsoft.Extensions.Caching.Abstractions\net461-Debug\Microsoft.Extensions.Caching.Abstractions.pdb" target="lib\net461\Microsoft.Extensions.Caching.Abstractions.pdb" />
    <file src="C:\git\runtime4\artifacts\bin\Microsoft.Extensions.Caching.Abstractions\netstandard2.0-Debug\Microsoft.Extensions.Caching.Abstractions.pdb" target="lib\netstandard2.0\Microsoft.Extensions.Caching.Abstractions.pdb" />
    <file src="C:\git\runtime4\artifacts\bin\Microsoft.Extensions.Caching.Abstractions\net461-Debug\Microsoft.Extensions.Caching.Abstractions.dll" target="lib\net461\Microsoft.Extensions.Caching.Abstractions.dll" />
    <file src="C:\git\runtime4\artifacts\bin\Microsoft.Extensions.Caching.Abstractions\netstandard2.0-Debug\Microsoft.Extensions.Caching.Abstractions.dll" target="lib\netstandard2.0\Microsoft.Extensions.Caching.Abstractions.dll" />
    <file src="C:\Users\anagniho\.nuget\packages\microsoft.private.intellisense\5.0.0-preview-20201009.2\IntellisenseFiles\net\1033\Microsoft.Extensions.Caching.Abstractions.xml" target="lib\net461\Microsoft.Extensions.Caching.Abstractions.xml" />
    <file src="C:\Users\anagniho\.nuget\packages\microsoft.private.intellisense\5.0.0-preview-20201009.2\IntellisenseFiles\net\1033\Microsoft.Extensions.Caching.Abstractions.xml" target="lib\netstandard2.0\Microsoft.Extensions.Caching.Abstractions.xml" />
    <file src="C:\Users\anagniho\.nuget\packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21222.1\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>

if we want it to set for non-symbols package we will probably have to ask the nuget team to do this.

ViktorHofer · 2021-05-04T17:10:08Z

@Anipik can't you set <Serviceable /> which is passed in to the PackTask? https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets#L256

That's the default behavior of Pack. I agree it's a bit annoying.

Understood. Whatever the default is, I'm ok with that.

Anipik · 2021-05-04T17:13:08Z

Is it expected that the xml files are included from the package cache? .nuget\packages\microsoft.private.intellisense\5.0.0-preview-20201009.2\IntellisenseFiles\net\1033\Microsoft.Extensions.Caching.Abstractions.xml

i checked with @safern and we are currently fine using the xml files from the nuget

https://licenses.nuget.org/MIT. LicenseUrl (and the msbuild property PackageLicenseUrl) is deprecated. From a search I couldn't find the place that sets it.

pack task does this, it appends the $(PackageLicenseExpression) to the default url. we will need to use the $(PackageLiscenseFile) property in order to point it to the licensefile in the package https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#packing-a-license-expression-or-a-license-file

@Anipik can't you set which is passed in to the PackTask? https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets#L256

we are already setting that property to true

ViktorHofer · 2021-05-04T17:18:15Z

@Anipik can't you set which is passed in to the PackTask? https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets#L256

@dotnet/nuget-team as we want to keep the serviceable attribute on our nuspecs, is there a way to do so via the PackTask? I see the Serviceable property being honored but based on @Anipik's diff, the attribute is removed from the non symbol packages.

ViktorHofer · 2021-05-04T17:21:55Z

pack task does this, it appends the $(PackageLicenseExpression) to the default url. we will need to use the $(PackageLiscenseFile) property in order to point it to the licensefile in the package https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#packing-a-license-expression-or-a-license-file

I just tried this out in a sample app and I can't see a licenseurl being added to the nuspec. Am I missing something?

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <PackageLicenseExpression>MIT</PackageLicenseExpression>
  </PropertyGroup>

</Project>

results in this nuspec:

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd">
  <metadata>
    <id>licenseurl</id>
    <version>1.0.0</version>
    <authors>licenseurl</authors>
    <owners></owners>
    <requireLicenseAcceptance>false</requireLicenseAcceptance>
    <license type="expression">MIT</license>
    <description>Package Description</description>
    <dependencies>
      <group targetFramework="net6.0" />
    </dependencies>
  </metadata>
</package>

Anipik · 2021-05-04T17:37:23Z

I can see the license url

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd">
  <metadata>
    <id>ConsoleApp29</id>
    <version>6.0.0</version>
    <authors>ConsoleApp29</authors>
    <license type="expression">MIT</license>
    <licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>
    <description>Package Description</description>
    <dependencies>
      <group targetFramework=".NETCoreApp3.1">
        <dependency id="NuGet.Frameworks" version="5.10.0-preview.2.7185" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
  <files>
    <file src="C:\Users\anagniho\source\repos\ConsoleApp29\bin\Debug\netcoreapp3.1\ConsoleApp29.runtimeconfig.json" target="lib\netcoreapp3.1\ConsoleApp29.runtimeconfig.json" />
    <file src="C:\Users\anagniho\source\repos\ConsoleApp29\bin\Debug\netcoreapp3.1\ConsoleApp29.dll" target="lib\netcoreapp3.1\ConsoleApp29.dll" />
  </files>
</package>

can you share the binlog ?

ViktorHofer · 2021-05-04T17:46:17Z

Shared the binlog offline with Anirudh.

ericstj · 2021-05-04T18:24:46Z

@dotnet/nuget-team as we want to keep the serviceable attribute on our nuspecs, is there a way to do so via the PackTask? I see the Serviceable property being honored but based on @Anipik's diff, the attribute is removed from the non symbol packages.

This didn't repro for me. Dotnet pack on this gives me a serviceable element in the nuspec.

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <Serviceable>true</Serviceable>
  </PropertyGroup>
</Project>

Anipik · 2021-05-04T18:34:58Z

I just tried this out in a sample app and I can't see a licenseurl being added to the nuspec. Am I missing something?

Talked with @nkolev92, he pointed out that this is a fallback for older clients (older than dev16) and should not affect anything if we have a license element as well

Anipik · 2021-05-04T18:50:44Z

This didn't repro for me. Dotnet pack on this gives me a serviceable element in the nuspec.

it could probably linked to my machine because i checked the the package produced in the internal builds and it has servicable=true https://dev.azure.com/dnceng/public/_build/results?buildId=1116205&view=artifacts&pathAsName=false&type=publishedArtifacts

package name = Microsoft.Extensions.Configuration.EnvironmentVariables.

ericstj · 2021-05-04T19:16:31Z

could probably linked to my machine

I gave it a try by pulling down your branch and building a couple packages and confirm that serviceable is in the nuspec.

ericstj · 2021-05-04T19:18:06Z

One thing that comes to mind with respect to this effort: a lot of this metadata is validated at publish time. It might be prudent to do an early dry run to catch anything that might block publishing. IIRC there is some async validation build that's supposed to catch this: we should look at it.

ViktorHofer · 2021-05-04T19:24:55Z

One thing that comes to mind with respect to this effort: a lot of this metadata is validated at publish time. It might be prudent to do an early dry run to catch anything that might block publishing. IIRC there is some async validation build that's supposed to catch this: we should look at it.

@ericstj see https://github.com/dotnet/core-eng/pull/12993 which covers how validation issues will be communicated.

ViktorHofer · 2021-05-04T19:26:52Z

@Anipik LGTM in general. Can you please add a fresh diff of the nuspecs with a recent NuGet client so that we can see that the licenseUrl element isn't present but the serviceable element is? Thanks

ericstj · 2021-05-04T22:29:30Z

see dotnet/core-eng#12993 which covers how validation issues will be communicated.

Will that be active to catch an issue as soon as this is in main and before we ship preview5? If not, let's manually check the results once this is in.

ViktorHofer · 2021-05-05T17:11:55Z

Will that be active to catch an issue as soon as this is in main and before we ship preview5? If not, let's manually check the results once this is in.

Nope, that will go live in June. I agree that we should review the produced packages from an official build post-merge.

...ncyInjection/tests/TrimmingTests/Microsoft.Extensions.DependencyInjection.TrimmingTests.proj

src/libraries/Directory.Build.props

safern · 2021-05-05T21:11:05Z

CC @safern @carlossanlop. I believe we need to have some form of state that drives this. If a library has done the work to move intellisense to triple-slash then that should be used (new model). If not, then the microsoft.private.intellisense package should be used (old model). So long as we aren't changing it with this PR it is OK.

Yes, this PR is actually making that easier for projects that no longer depend on a pkgproj.

I would expect we will introduce a new property to specify <UseDocsFromTripleSlash> or something like that and if that is set to true, use the generated ones by the compiler rather than the ones from the intellisense package.

ericstj · 2021-05-06T17:22:00Z

Great to see this merged. Thanks for the great team work here! 🚀 🚀 🚀

michellemcdaniel · 2021-05-07T21:47:36Z

@ericstj @ViktorHofer Notifications won't be live for preview5, likely, but you can monitor Validate-DotNet manually if you need to (Runtime runs: https://dev.azure.com/dnceng/internal/_build?definitionId=827&tagFilter=validation-runtime-.NET%206). We run nuget metadata validation in the NuGet Validation step in the Validation ring. See here for last night's run (it's green).

Anipik requested review from ViktorHofer, ericstj and safern April 29, 2021 23:56

dotnet-issue-labeler bot added the area-Meta label Apr 29, 2021

safern reviewed Apr 30, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.Logging/src/Microsoft.Extensions.Logging.csproj Outdated Show resolved Hide resolved

Anipik closed this Apr 30, 2021

Anipik deleted the port2 branch April 30, 2021 01:09

Anipik restored the port2 branch April 30, 2021 01:14

Anipik reopened this Apr 30, 2021

ViktorHofer reviewed Apr 30, 2021

View reviewed changes

...t.Extensions.HostFactoryResolver/src/Microsoft.Extensions.HostFactoryResolver.Sources.csproj Show resolved Hide resolved

ViktorHofer reviewed Apr 30, 2021

View reviewed changes

src/libraries/Directory.Build.props Outdated Show resolved Hide resolved

ericstj mentioned this pull request May 5, 2021

Move Logging Generator and attribute to Logging.Abstractions #52256

Merged

1 task

Anipik added 5 commits May 5, 2021 10:10

remove pkgprojs from other extensions projects

35afcaa

make resolver non shipping

3eb779d

add comments

4e2b7e4

move packageDescripition to src project

277b456

Fix linker tests

3dcdf0b

revert ectensions change because of the new generator

0cf8918

ViktorHofer reviewed May 5, 2021

View reviewed changes

...ncyInjection/tests/TrimmingTests/Microsoft.Extensions.DependencyInjection.TrimmingTests.proj Outdated Show resolved Hide resolved

Anipik added 2 commits May 5, 2021 10:16

set is packable to false for logging abstactions.

1de3bf3

remove pkgproj for the extensions.logging as we moved the gen out of it

eb0082b

ViktorHofer reviewed May 5, 2021

View reviewed changes

src/libraries/Directory.Build.props Outdated Show resolved Hide resolved

add isSrcProject condition

957ea34

ViktorHofer reviewed May 5, 2021

View reviewed changes

src/libraries/Directory.Build.props Outdated Show resolved Hide resolved

Anipik added 2 commits May 5, 2021 11:06

Update Directory.Build.props

047f186

Update Directory.Build.props

c119b97

ericstj mentioned this pull request May 5, 2021

Extra package dependencies appearing for current framework. #52318

Closed

ViktorHofer approved these changes May 6, 2021

View reviewed changes

ViktorHofer merged commit 3897ee5 into dotnet:main May 6, 2021

dougbu mentioned this pull request May 7, 2021

[main] Update dependencies from dotnet/efcore dotnet/runtime dotnet/aspnetcore#32461

Merged

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dotnet pack for other Microsoft.extensions* projects #52084

Use dotnet pack for other Microsoft.extensions* projects #52084

Anipik commented Apr 29, 2021

ViktorHofer commented Apr 30, 2021

Anipik commented Apr 30, 2021

ViktorHofer commented Apr 30, 2021

Anipik commented Apr 30, 2021

Anipik commented May 4, 2021

ViktorHofer commented May 4, 2021

ericstj commented May 4, 2021

Anipik commented May 4, 2021

ViktorHofer commented May 4, 2021 •

edited

Loading

Anipik commented May 4, 2021

ViktorHofer commented May 4, 2021

ViktorHofer commented May 4, 2021

Anipik commented May 4, 2021

ViktorHofer commented May 4, 2021

ericstj commented May 4, 2021

Anipik commented May 4, 2021

Anipik commented May 4, 2021

ericstj commented May 4, 2021

ericstj commented May 4, 2021

ViktorHofer commented May 4, 2021

ViktorHofer commented May 4, 2021

ericstj commented May 4, 2021

ViktorHofer commented May 5, 2021

safern commented May 5, 2021

ericstj commented May 6, 2021

michellemcdaniel commented May 7, 2021 •

edited

Loading

Use dotnet pack for other Microsoft.extensions* projects #52084

Use dotnet pack for other Microsoft.extensions* projects #52084

Conversation

Anipik commented Apr 29, 2021

ViktorHofer commented Apr 30, 2021

Anipik commented Apr 30, 2021

ViktorHofer commented Apr 30, 2021

Anipik commented Apr 30, 2021

Anipik commented May 4, 2021

ViktorHofer commented May 4, 2021

ericstj commented May 4, 2021

Anipik commented May 4, 2021

ViktorHofer commented May 4, 2021 • edited Loading

Anipik commented May 4, 2021

ViktorHofer commented May 4, 2021

ViktorHofer commented May 4, 2021

Anipik commented May 4, 2021

ViktorHofer commented May 4, 2021

ericstj commented May 4, 2021

Anipik commented May 4, 2021

Anipik commented May 4, 2021

ericstj commented May 4, 2021

ericstj commented May 4, 2021

ViktorHofer commented May 4, 2021

ViktorHofer commented May 4, 2021

ericstj commented May 4, 2021

ViktorHofer commented May 5, 2021

safern commented May 5, 2021

ericstj commented May 6, 2021

michellemcdaniel commented May 7, 2021 • edited Loading

ViktorHofer commented May 4, 2021 •

edited

Loading

michellemcdaniel commented May 7, 2021 •

edited

Loading