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.

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

[master] Update dependencies from dotnet/runtime #14574

Merged

Conversation

dotnet-maestro[bot]
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented Nov 17, 2020

This pull request updates the following dependencies

From https://github.com/dotnet/runtime

  • Subscription: ea219f43-0754-4d2f-576e-08d76e1d56cb
  • Build: 20201216.14
  • Date Produced: 12/17/2020 8:00 AM
  • Commit: be27e294a12514638bb5337c7fe5cc72d93a443a
  • Branch: refs/heads/master

…1117.3

Microsoft.NETCore.Platforms , System.CodeDom , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.DotNetHostResolver , Microsoft.NETCore.App.Ref , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.6.0
 From Version 6.0.0-alpha.1.20560.10 -> To Version 6.0.0-alpha.1.20567.3
…1117.4

Microsoft.NETCore.Platforms , System.CodeDom , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.DotNetHostResolver , Microsoft.NETCore.App.Ref , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.6.0
 From Version 6.0.0-alpha.1.20560.10 -> To Version 6.0.0-alpha.1.20567.4
@ViktorHofer
Copy link
Member

cc @jkoritzinsky

…1117.16

Microsoft.NETCore.Platforms , System.CodeDom , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.DotNetHostResolver , Microsoft.NETCore.App.Ref , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.6.0
 From Version 6.0.0-alpha.1.20560.10 -> To Version 6.0.0-alpha.1.20567.16
@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 18, 2020

error NU1102: (NETCORE_ENGINEERING_TELEMETRY=Restore) Unable to find package Microsoft.NETCore.DotNetAppHost with version (>= 6.0.0-alpha.1.20567.16)

…1118.2

Microsoft.NETCore.Platforms , System.CodeDom , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.DotNetHostResolver , Microsoft.NETCore.App.Ref , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.6.0
 From Version 6.0.0-alpha.1.20560.10 -> To Version 6.0.0-alpha.1.20568.2
@ViktorHofer
Copy link
Member

@jkoritzinsky any idea why the DotNetAppHost package is missing?

@mmitche
Copy link
Member

mmitche commented Nov 18, 2020

@ViktorHofer @jkoritzinsky It was removed here dotnet/runtime@a505c46

/cc @Anipik

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 18, 2020

I'm relatively certain this is because of Jeremy's shared framework sdk change:

image

@mmitche
Copy link
Member

mmitche commented Nov 18, 2020

I'm relatively certain this is because of Jeremy's shared framework sdk change:

image

Ahh yeah that commit is in between but wasn't built.

@jkoritzinsky
Copy link
Member

That was unintentional. I thought I kept all of the legacy host packages. Apparently accidentally dropped one. I’ll add it back.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 18, 2020

When you say legacy package, do you mean that dotnet/sdk should use another one ideally?

@jkoritzinsky
Copy link
Member

By legacy I mean a package that used to be used in product construction before .NET Core 3.0. If the SDK still has a usage for the packages, then they’re the right ones to use.

…1118.7

Microsoft.NETCore.Platforms , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.DotNetHostResolver , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , System.CodeDom , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.6.0
 From Version 6.0.0-alpha.1.20560.10 -> To Version 6.0.0-alpha.1.20568.7
@ViktorHofer
Copy link
Member

Do we need to update the System.Text.Json version being used in the sdk?

src\Resolvers\Microsoft.DotNet.MSBuildSdkResolver\Microsoft.DotNet.MSBuildSdkResolver.csproj(0,0): error NU1605: (NETCORE_ENGINEERING_TELEMETRY=Restore) Detected package downgrade: System.Text.Json from 6.0.0-alpha.1.20568.7 to 4.7.2. Reference the package directly from the project to select a different version. 
 Microsoft.DotNet.MSBuildSdkResolver -> Microsoft.DotNet.Cli.Utils -> Microsoft.Extensions.DependencyModel 6.0.0-alpha.1.20568.7 -> System.Text.Json (>= 6.0.0-alpha.1.20568.7) 
 Microsoft.DotNet.MSBuildSdkResolver -> System.Text.Json (>= 4.7.2)

@jkoritzinsky
Copy link
Member

cc: @dsplaisted

…1119.2

Microsoft.NETCore.Platforms , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.DotNetHostResolver , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , System.CodeDom , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.6.0
 From Version 6.0.0-alpha.1.20560.10 -> To Version 6.0.0-alpha.1.20569.2
@dsplaisted
Copy link
Member

@ericstj @joperezr Is Microsoft.Extensions.DependencyModel supposed to be depending on the latest version of System.Text.Json?

We've had to be careful about how we handle these dependencies to ensure that they can load correctly for MSBuild tasks running on Full Framework.

@ericstj
Copy link
Member

ericstj commented Nov 19, 2020

In 5.0.0 Microsoft.Extensions.DependencyModel depended on System.Text.Json 5.0.0:
https://www.nuget.org/packages/Microsoft.Extensions.DependencyModel/
image

In 6.0 it depends on the 6.0.0 version.

@joperezr
Copy link
Member

Yeah, as @ericstj explained by default in dotnet/runtime repo all dependencies between live-built packages are to the live-built version (latest version) after a re-baseline of the repo, which we do after every big release. That means that now that we have switched dotnet/runtime repo to target 6.0, we have re-baselined all live-built packages to use 6.0.0 as well.

@dsplaisted
Copy link
Member

The restore failure is happening when the TargetFramework for the project is net6.0. This insertion is bringing in a version of the Microsoft.Extensions.DependencyModel which has dependencies on the System.Text.Encodings.Web and System.Text.Json packages for net6.0, where the previous version of the package had no package dependencies in for net6.0. This is because the old version of the package had an empty framework dependency group for net5.0:

<group targetFramework="net5.0" />

The new package doesn't have this group, so NuGet ends up selecting the .NETCoreApp2.1 package dependencies.

This seems like an unintentional change and it is breaking us. @ericstj / @joperezr Can you investigate this change or find the right person to do so? Thanks!

@ViktorHofer
Copy link
Member

@ericstj @joperezr can you please take a look and check if the added dependencies are right?

@ericstj
Copy link
Member

ericstj commented Nov 24, 2020

Microsoft.Extensions.DependencyModel which has dependencies on the System.Text.Encodings.Web and System.Text.Json packages for net6.0

Indeed I can see this here: https://www.nuget.org/packages/Microsoft.Extensions.DependencyModel/
image

This is controlled by the inboxOn calculations by the packaging system to drop the package references where these are inbox. @Anipik we need to add inbox here (and likely others) for 6.0.0: https://github.com/dotnet/runtime/blob/2e8c4c78d5dc628afd94053a008ab203e648a2b9/src/libraries/pkg/baseline/packageIndex.json#L6629
Opened dotnet/runtime#45163 to track this.

@dsplaisted why would you be referencing System.Text.Json package at all on net5.0 or net6.0. It's inbox, just use the one in the framework, it's what's going to win from conflict resolution anyway.

<PackageReference Include="System.Text.Json" Version="4.7.2" Condition="'$(UseSystemTextJson)'=='True'"/>

You could do that to unblock yourself here.

@dsplaisted
Copy link
Member

@ericstj Good point, I think b3c9c3f should fix this

@mmitche
Copy link
Member

mmitche commented Dec 16, 2020

Aside - is there a way to stop the bot picking up new runtime versions?

In the original Maestro, we had a label that could be applied to the PR - Maestro-Stop-Updating. I'm not sure if the current version has the ability or not.

No, but the subscription can be disabled in the interim.

@danmoseley
Copy link
Member

Can someone who deals with File IO take a look at this?

I will look as soon as I'm out of meetings today, if nobody does ahead of time. It would be surprising if something so basic broke; I don't see any relevant recent changes in a quick look.

@dsplaisted
Copy link
Member

Can someone who deals with File IO take a look at this?

I will look as soon as I'm out of meetings today, if nobody does ahead of time. It would be surprising if something so basic broke; I don't see any relevant recent changes in a quick look.

@danmosemsft I created a simple repro:

static void Main(string[] args)
{
    foreach (var subdir in new DirectoryInfo(args[0]).GetDirectories())
    {
        Console.WriteLine(subdir.FullName);
    }
}

I get different output between .NET 5 and .NET 6 (Shared Framework 6.0.0-alpha.1.20614.10). .NET 5:

/mnt/c/git/cli
/mnt/c/git/CsWinRT
/mnt/c/git/demo
/mnt/c/git/docs
...

.NET 6

cli
CsWinRT
demo
docs
...

@danmoseley
Copy link
Member

Looking at that now.

@eerhardt
Copy link
Member

I have a suspicion that the subdir.FullName issue is a crossgen problem. I rebuilt System.IO.FileSystem locally using the same SHA as that build, and I use my local .dll (which isn't crossgen'd) and the problem doesn't repro.

@jeffschwMSFT
Copy link
Member

@mangod9

@danmoseley
Copy link
Member

As @eerhardt pointed out - here's the repro:

dan@LAPTOP-P6UJDVTA:~/1$ cat nuget.config
<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <!--To inherit the global NuGet package sources remove the <clear/> line below -->
    <clear />
    <add key="nuget" value="https://api.nuget.org/v3/index.json" />
<add key="dotnet6" value="https://dnceng.pkgs.visualstudio.com/public/_packaging/dotnet6/nuget/v3/index.json" />
  </packageSources>
</configuration>
dan@LAPTOP-P6UJDVTA:~/1$ cat 1.csproj
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
   <RuntimeIdentifier>linux-x64</RuntimeIdentifier>
    <RuntimeFrameworkVersion>6.0.0-alpha.1.20614.10</RuntimeFrameworkVersion>
  </PropertyGroup>

  <ItemGroup>
    <KnownFrameworkReference Remove="Microsoft.AspNetCore.App" />
  </ItemGroup>

</Project>
dan@LAPTOP-P6UJDVTA:~/1$ cat Program.cs
using System;
using System.IO;

class Program
{
    static void Main(string[] args)
    {
        foreach (var subdir in new DirectoryInfo("/home/dan/1").GetDirectories())
        {
            Console.WriteLine(subdir.FullName);
        }
    }
}
dan@LAPTOP-P6UJDVTA:~/1$ export COMPlus_ReadyToRun=1
dan@LAPTOP-P6UJDVTA:~/1$ dotnet run
obj
bin
dan@LAPTOP-P6UJDVTA:~/1$ export COMPlus_ReadyToRun=0
dan@LAPTOP-P6UJDVTA:~/1$ dotnet run
/home/dan/1/obj
/home/dan/1/bin
dan@LAPTOP-P6UJDVTA:~/1$

@danmoseley
Copy link
Member

Should we be running unit tests against crossgenned bits? As far as I can see, this would break the unit tests.

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 16, 2020

Should we be running unit tests against crossgenned bits? As far as I can see, this would break the unit tests.

For libraries we don't today as we don't crossgen in the libraries build but in the installer. Kind of tracked via dotnet/runtime#487

@danmoseley
Copy link
Member

For libraries we don't today as we don't crossgen in the libraries build but in the installer. Kind of tracked via dotnet/runtime#487

Right, it's come up several times. This would essentially be a move to catch crossgen, not libraries bugs - so I think @mangod9 would be the best person to decide what the priority should be. In this case, we were fortunate that SDK tests found it -- as far as I know the bits we ship to customers never have the libraries teams' tests run on them at all.

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 16, 2020

the bits we ship to customers never have the libraries teams' tests run on them at all.

Correct, we don't run automated tests for official builds.

This would essentially be a move to catch crossgen, not libraries bugs - so I think @mangod9 would be the best person to decide what the priority should be.

We might want to do it sooner or later anyway as it reduces complexity and duplication of our test bootstrapping logic. @jkoritzinsky and I were looking at using the bundle that currently lives in the installer subset of dotnet/runtime for that.

@danmoseley
Copy link
Member

My vote would be sooner rather than later as upstack tests probably only verify 10% of what our unit tests do (since it's not their job) - it feels like we are relying on previews to discover such bugs, and we want previews to be more predictable.

@ViktorHofer
Copy link
Member

Absolutely agree. Will make sure that this item is prioritized accordingly.

@mangod9
Copy link
Member

mangod9 commented Dec 16, 2020

+@trylek, who will soon be working on compiling libraries using cg2. Would be good to ensure that we can run library tests on compiled bits. Thx.

@mangod9
Copy link
Member

mangod9 commented Dec 16, 2020

As for the subdir.FullName issue, does it repro on windows too?

@dsplaisted
Copy link
Member

As for the subdir.FullName issue, does it repro on windows too?

No, it appears only to repro on Mac and Linux

@mangod9
Copy link
Member

mangod9 commented Dec 17, 2020

I looked at this to narrow down when things broke, and it appears possibly related to this change: dotnet/runtime#45625. Looks to have been rolled back recently since it was causing other failures as part of this PR: dotnet/runtime#46172

this is possibly the cause:

This PR blocks crossgenning UnmangedCallersOnly methods in crossgen1 because the custom attribute parsing code is not currently included in crossgen1 and I didn't want to fight to get that hooked up since we're looking at deprecating/removing crossgen1 in the .NET 6 time frame anyway.

@jkoritzinsky possibly this should wait till we get cg2 rolled out broadly ?

@mangod9
Copy link
Member

mangod9 commented Dec 17, 2020

@jkotas fyi

dotnet-maestro bot and others added 2 commits December 17, 2020 12:39
…1216.14

Microsoft.NETCore.Platforms , Microsoft.NETCore.DotNetHostResolver , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , System.CodeDom , System.Reflection.MetadataLoadContext , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.6.0
 From Version 6.0.0-alpha.1.20560.10 -> To Version 6.0.0-alpha.1.20616.14
@jkotas
Copy link
Member

jkotas commented Dec 17, 2020

@jkoritzinsky possibly this should wait till we get cg2 rolled out broadly ?

@jkotas fyi

This is not common. There are number of other less common cases that cg2 handles better than cg1. One more or less won't make a huge difference. I think this is fine.

@jeffschwMSFT
Copy link
Member

@mangod9 @jkoritzinsky what is our plan to get this addressed? Are we going to take the latest runtime which contains the revert?

@akoeplinger
Copy link
Member

akoeplinger commented Dec 17, 2020

Are we going to take the latest runtime which contains the revert?

The latest runtime bump from about three hours ago already contains the fix: dotnet/runtime#46176

@mangod9
Copy link
Member

mangod9 commented Dec 17, 2020

Yeah build 6.0.0-alpha.1.20616.14 doesnt repro the issue anymore.

@dotnet-maestro dotnet-maestro bot merged commit 21a2f4e into master Dec 17, 2020
@dotnet-maestro dotnet-maestro bot deleted the darc-master-4a612234-0bdd-45db-8d7f-6c5bed075213 branch December 17, 2020 17:01
@dsplaisted
Copy link
Member

It passed CI and merged! 🎉🎉🎉

Thanks folks.

@danmoseley
Copy link
Member

@marek-safar @jeffschwMSFT...tag! Which of you takes the next one.

@jeffschwMSFT
Copy link
Member

I can be it. Given the holidays, it will likely be early next year. :)

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

Successfully merging this pull request may close these issues.