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

Bump versions of maintenance-packages dependencies consumed in machin… #7274

Closed

Conversation

carlossanlop
Copy link
Member

dotnet/runtime depends on these packages that we are now publishing from dotnet/maintenance-packages:

  • Microsoft.Bcl.HashCode
  • System.Buffers
  • System.Memory
  • System.Runtime.CompilerServices.Unsafe

Bumping their versions to consume the new preview versions, available in the dotnet-libraries feed: https://dnceng.visualstudio.com/public/_artifacts/feed/dotnet-libraries

Related PRs:

Copy link

codecov bot commented Oct 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.90%. Comparing base (f385b06) to head (908acb5).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7274      +/-   ##
==========================================
+ Coverage   68.80%   68.90%   +0.09%     
==========================================
  Files        1461     1467       +6     
  Lines      272400   274380    +1980     
  Branches    28176    28642     +466     
==========================================
+ Hits       187436   189066    +1630     
- Misses      77729    77983     +254     
- Partials     7235     7331      +96     
Flag Coverage Δ
Debug 68.90% <ø> (+0.09%) ⬆️
production 63.38% <ø> (+0.07%) ⬆️
test 89.18% <ø> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 22 files with indirect coverage changes

Copy link
Member Author

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Update to latest versions

eng/Versions.props Outdated Show resolved Hide resolved
eng/Versions.props Outdated Show resolved Hide resolved
eng/Versions.props Outdated Show resolved Hide resolved
eng/Versions.props Outdated Show resolved Hide resolved
@carlossanlop
Copy link
Member Author

@michaelgsharp there are several dead-lettering failures. Can you take a look? Maybe the VM queues need updating?

@michaelgsharp
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@ericstj
Copy link
Member

ericstj commented Nov 4, 2024

System.IO.FileLoadException: Could not load file or assembly 'System.Memory, Version=4.0.1.2, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x8013104

The new version of System.Memory is 4.0.2.0. I would expect the test to auto-generate a bindingRedirect for this.

@carlossanlop
Copy link
Member Author

I found these two lines hardcoding the package version, which should now be 4.6.0:

#if NETCOREAPP
internal static readonly ReferenceAssemblies DefaultReferenceAssemblies = ReferenceAssemblies.Default
.AddPackages(ImmutableArray.Create(new PackageIdentity("System.Memory", "4.5.1")));
#else
internal static readonly ReferenceAssemblies DefaultReferenceAssemblies = ReferenceAssemblies.NetFramework.Net472.Default
.AddPackages(ImmutableArray.Create(new PackageIdentity("System.Memory", "4.5.1")));
#endif

@ericstj
Copy link
Member

ericstj commented Nov 4, 2024

Those are just for testing the analyzer.

Here the problem is due to RemoteExecutor.

The .dll.config has the correct redirect. If you download the helix payload and examine workitems\Microsoft.ML.CpuMath.UnitTests\Microsoft.ML.CpuMath.UnitTests.dll.config you can see

      <dependentAssembly>
        <assemblyIdentity name="System.Memory" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-4.0.2.0" newVersion="4.0.2.0" />
      </dependentAssembly>

But that .dll.config isn't being reused with RemoteExecutor.

@ericstj
Copy link
Member

ericstj commented Nov 4, 2024

This same issue was hit here: dotnet/runtime#104647 @ViktorHofer mentions that it should be handled with https://github.com/dotnet/arcade/blob/fc2f7ce8372a55725aab7b48c25bad7327a9769d/src/Microsoft.DotNet.RemoteExecutor/src/build/Microsoft.DotNet.RemoteExecutor.targets#L8-L33 -- it looks like ML.NET doesn't use that remote executor, but still uses it's own which may not have the fix. So that's the "right fix" here -- make the ML.NET remoteexecutor honor the tests redirects.

If I look at the assembly refs here, most are coming from the netstandard2.0 library which was pinned to 4.0.1.2. In the 4.5.5 version of System.Memory both the netstandard2.0 and net461 libraries had the same version. In the new package we kept netstandard2.0 pinned at 4.0.1.2 but bumped net4* to 4.0.2.0. I wonder if we should avoid doing this in servicing since it'll be introducing the need for a redirect in this scenario? I checked and the version that's inbox in netcoreapp2.1 is actually 4.1.0.0 so we don't need to pin the netstandard2.0 version -- so long as it remains lower than 4.1.0.0.

@michaelgsharp
Copy link
Member

We don't use the RemoteExecutor in arcade because it doens't support NetFX, so we still have our custom one. Let me take a look at @ViktorHofer's fix and see what it would take to adopt it as well.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 5, 2024

@michaelgsharp RemoteExecutor in Arcade supports .NET Framework. A lot of the tests in runtime target framework and depend on RemoteExecutor.

@michaelgsharp
Copy link
Member

Closing as #7295 takes care of it and the remote executor update.

@carlossanlop carlossanlop deleted the BumpMaintenancePackagesDeps branch November 11, 2024 20:47
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.

4 participants