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

[main] Update dependencies from dotnet/runtime #41014

Merged
merged 11 commits into from
May 21, 2024
Merged

[main] Update dependencies from dotnet/runtime #41014

merged 11 commits into from
May 21, 2024

Conversation

dotnet-maestro[bot]
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented May 20, 2024 •
edited
Loading

This pull request updates the following dependencies

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

  • Subscription: cedddd63-79f5-4e7e-6d46-08dc434c4948
  • Build: 20240521.1
  • Date Produced: May 21, 2024 11:23:14 AM UTC
  • Commit: 2bfd1b44200de244a66b7f4a7604be1f2d5585bc
  • Branch: refs/heads/main

@dotnet-maestro
Update dependencies from https://github.com/dotnet/runtime build 2024…
8856e11 
…0519.2

Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.Logging , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging.Console , Microsoft.NET.HostModel , Microsoft.NET.ILLink.Tasks , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.Platforms , System.CodeDom , System.Reflection.MetadataLoadContext , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.ServiceProcess.ServiceController , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.9.0 , VS.Redist.Common.NetCore.TargetingPack.x64.9.0 , Microsoft.SourceBuild.Intermediate.runtime.linux-x64
 From Version 9.0.0-preview.5.24268.2 -> To Version 9.0.0-preview.5.24269.2
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-CodeFlow untriaged Request triage from a team member labels May 20, 2024
@dotnet-maestro DotNet Maestro
Copy link
Contributor Author

Notification for subscribed users from https://github.com/dotnet/runtime:

@dotnet/dnr-codeflow

Action requested: Please take a look at this failing automated dependency-flow pull request's checks; failures may be related to changes which originated in your repo.

  • This pull request contains changes from your source repo (https://github.com/dotnet/runtime) and seems to have failed checks in this PR. Please take a peek at the failures and comment if they seem relevant to your changes.
  • If you're being tagged in this comment it is due to an entry in the related Maestro Subscription of the Build Asset Registry. If you feel this entry has added your GitHub login or your GitHub team in error, please update the subscription to reflect this.
  • For more details, please read the Arcade Darc documentation

@jeffschwMSFT
Copy link
Member

@jozkee can you take a look at these failures? Perhaps related to the following:

dotnet/runtime#101499

@ericstj
Copy link
Member

ericstj commented May 20, 2024

I had a look, SDK is still on an old compiler. I'm updating it and testing that it fixes the problem.

dotnet-maestro bot and others added 2 commits May 20, 2024 16:15
@dotnet-maestro
Update dependencies from https://github.com/dotnet/runtime build 2024…
bbf5817 
…0520.2

Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.Logging , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging.Console , Microsoft.NET.HostModel , Microsoft.NET.ILLink.Tasks , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.Platforms , System.CodeDom , System.Reflection.MetadataLoadContext , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.ServiceProcess.ServiceController , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.9.0 , VS.Redist.Common.NetCore.TargetingPack.x64.9.0 , Microsoft.SourceBuild.Intermediate.runtime.linux-x64
 From Version 9.0.0-preview.5.24268.2 -> To Version 9.0.0-preview.5.24270.2
@ericstj
Update SDK to latest public build
858a2e7
@ericstj
Copy link
Member

ericstj commented May 20, 2024

Fixing the new compiler warnings this introduced...

@ericstj ericstj requested a review from a team as a code owner May 20, 2024 18:27
333fred
333fred approved these changes May 20, 2024
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Test change LGTM

@ericstj
Copy link
Member

ericstj commented May 20, 2024

RazorTools tests seem to known timeout issue. #40074

Unified build failure is

   CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [D:\a\_work\1\vmr\src\wpf\src\Microsoft.DotNet.Wpf\src\System.Windows.Input.Manipulations\System.Windows.Input.Manipulations.csproj]
    CSC : error CS0656: Missing compiler required member 'System.Runtime.InteropServices.MemoryMarshal.CreateReadOnlySpan' [D:\a\_work\1\vmr\src\wpf\src\Microsoft.DotNet.Wpf\src\UIAutomation\UIAutomationTypes\UIAutomationTypes.csproj]

So these are problems building WPF - likely with the new toolset. 🤔
Here's the binlog: https://artprodcus3.artifacts.visualstudio.com/A6fcc92e5-73a7-4f88-8d13-d9045b45fb27/cbb18261-c48f-4abb-8651-8cdcb5474649/_apis/artifact/cGlwZWxpbmVhcnRpZmFjdDovL2RuY2VuZy1wdWJsaWMvcHJvamVjdElkL2NiYjE4MjYxLWM0OGYtNGFiYi04NjUxLThjZGNiNTQ3NDY0OS9idWlsZElkLzY4MTc1NS9hcnRpZmFjdE5hbWUvV2luZG93c194NjRfQnVpbGRMb2dzX0F0dGVtcHQx0/content?format=file&subPath=%2Fsrc%2Fwpf%2Fartifacts%2Flog%2FRelease%2Fsource-inner-build.binlog

It seems these WPF projects only pass a subset of references:

The method the compiler is complaining about is in System.Memory:
https://github.com/dotnet/runtime/blob/24562bcabefaea5e03c74d01e4df8fc7c112a13a/src/libraries/System.Memory/ref/System.Memory.cs#L701

That's not passed in:
image

@jozkee
Copy link
Member

jozkee commented May 20, 2024 •
edited
Loading

@ericstj I stepped into this issue as well on dotnet/runtime, you need to add a reference to System.Memory, see dotnet/runtime#100898 (comment). One of the new params ROS<T> APIs is now being used, that's what's triggering the build error.

@build-analysis build-analysis bot mentioned this pull request May 20, 2024
Open
@ericstj
Copy link
Member

ericstj commented May 20, 2024

Right but we cannot do that because it's building WPF, which is not part of this repo. So those need to be added here:
dotnet/wpf#9135

Then that hash of WPF needs to flow to this repo before it can update the compiler. I wish there was a way to workaround this @ViktorHofer @premun @MichaelSimons in case they have ideas.

I wonder if MemoryMarshal should be pushed down into System.Runtime if the compiler depends on it like this (demanding rather than lighting up in its presense)? cc @jaredpar for his thoughts.

@333fred
Copy link
Member

333fred commented May 20, 2024

The compiler does not demand its presence; the problem is that it really is being used. An existing codepath is now binding to a params ReadOnlySpan<T> overload, and these APIs are necessary to use that overload.

@ericstj
Copy link
Member

ericstj commented May 20, 2024

The compiler does not demand its presence; the problem is that it really is being used. An existing codepath is now binding to a params ReadOnlySpan overload, and these APIs are necessary to use that overload.

Ah, that makes more sense. I couldn't connect the dots to the syntax that was causing this. Would make it much easier if I got source and line info with the error.

@333fred 333fred mentioned this pull request May 20, 2024
Open
@333fred
Copy link
Member

333fred commented May 20, 2024

Ah, that makes more sense. I couldn't connect the dots to the syntax that was causing this. Would make it much easier if I got source and line info with the error.

I agree, filed dotnet/roslyn#73596 for this.

@ericstj ericstj requested review from a team as code owners May 20, 2024 23:15
@ericstj
Copy link
Member

ericstj commented May 20, 2024

Looks like the change to WPF has further to flow than I thought. Trying out a patch file.

@ericstj
Copy link
Member

ericstj commented May 21, 2024

The patch file worked but I need more than these two projects- that unblocked more that fail. Will need to see if I can repro the failure in WPF build to get all projects.

@jaredpar
Copy link
Member

The compiler does not demand its presence; the problem is that it really is being used. An existing codepath is now binding to a params ReadOnlySpan overload, and these APIs are necessary to use that overload.

Agree that is the cause here.

It seems these WPF projects only pass a subset of references:

Based on the binlog view I agree. This is a common source of problems with moving to new .NET SDK. Essentially customers who were compiling without the complete reference graph, and getting away with it, get bit because a new feature, bug fix, etc ... cause the compiler to read members that require the missing references. Requiring the extra references is generally a "by design" resolution. Can see other instances of this we've hit in the past.

If I could go back to C# 1.0, I'd force a warning whenever compilations had incomplete reference graphs. It is just inviting interesting behaviors like this and saving virtually nothing. 😦

@v-wuzhai v-wuzhai mentioned this pull request May 21, 2024
Merged
@ViktorHofer
Copy link
Member

There are more cases. Submitted dotnet/wpf#9140

dotnet-maestro bot and others added 2 commits May 21, 2024 11:43
@dotnet-maestro
Update dependencies from https://github.com/dotnet/runtime build 2024…
6c888d6 
…0521.1

Microsoft.Extensions.DependencyModel , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.Logging , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging.Console , Microsoft.NET.HostModel , Microsoft.NET.ILLink.Tasks , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.Platforms , System.CodeDom , System.Reflection.MetadataLoadContext , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.ServiceProcess.ServiceController , System.Text.Encoding.CodePages , VS.Redist.Common.NetCore.SharedFramework.x64.9.0 , VS.Redist.Common.NetCore.TargetingPack.x64.9.0 , Microsoft.SourceBuild.Intermediate.runtime.linux-x64
 From Version 9.0.0-preview.5.24268.2 -> To Version 9.0.0-preview.5.24271.1
@ViktorHofer
Create 0001-more-system-memory-fixes.patch
29b0e02
ViktorHofer added a commit that referenced this pull request May 21, 2024
@ViktorHofer
Simplify scenario-tests test invocation & define dependencies
dec5515 
1. Simplify scenario-tests test invocation by avoiding
   the intermediate NoTargets wrapper project.
2. Declare the dependency on extract-sdk-archive.proj
   in the test projects directly to avoid race
   conditions. #41014
   failed because the scenario-tests already ran
   but the SDK test folder wasn't yet ready.
@ViktorHofer ViktorHofer mentioned this pull request May 21, 2024
Merged
@ViktorHofer
Copy link
Member

Another one: dotnet/wpf#9143

@ericstj
Copy link
Member

ericstj commented May 21, 2024

If I could go back to C# 1.0, I'd force a warning whenever compilations had incomplete reference graphs.

Here it's not a gap in the reference graph. For example https://github.com/dotnet/runtime/pull/101499/files#diff-cec8e6f471b4193246bdc0107b0dd7cbe131fb7fd189b288b37269c333d1171dR2170 is in System.Runtime and all its API is in System.Runtime (since it's the core assembly) but to call it you need to also reference System.Memory. This is why I think we should push MemoryMarshal down to System.Runtime.

@jaredpar
Copy link
Member

Here it's not a gap in the reference graph.

Ah okay. It's the impl class we're calling so not strictly a gap.

This is why I think we should push MemoryMarshal down to System.Runtime.

This seems like a reasonable solution. At the same time, customers that are manually subsetting the .NET reference (taking parts of it and not all) are playing with fire. There are other scenarios that can break when this is done. I'd separately be curious why WPF is doing this cause it's fragile to changes like this.

@ViktorHofer ViktorHofer merged commit a543ce7 into main May 21, 2024
27 of 28 checks passed
@ViktorHofer ViktorHofer deleted the darc-main-1c9b3f30-7d52-4f86-9b33-513c27badc0b branch May 21, 2024 15:54
@ericstj ericstj mentioned this pull request May 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelSimons MichaelSimons MichaelSimons left review comments

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@333fred 333fred 333fred approved these changes

Assignees
No one assigned
Labels
Area-CodeFlow untriaged Request triage from a team member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jeffschwMSFT @ericstj @jozkee @333fred @jaredpar @ViktorHofer @MichaelSimons