Manually backflow VMR's main #50780

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

Open

premun wants to merge 5 commits into dotnet:main from premun:prvysoky/backflow-main

Member

premun commented Sep 12, 2025

Backflowing dotnet/dotnet@5d79341

dotnet/arcade-services#5231

premun requested a review from a team as a code owner

September 12, 2025 12:23

Copilot AI review requested due to automatic review settings

September 12, 2025 12:23

premun requested a review from a team as a code owner

September 12, 2025 12:23

Contributor

github-actions bot commented Sep 12, 2025

This PR is targeting main, which is now for .NET 11-facing work. If you intended to target .NET 10, either retarget this PR to release/10.0.1xx or make sure you backport the change to release/10.0.1xx after merging. See #50394 for more details.

github-actions bot added the Area-Infrastructure label

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Pull Request Overview

This PR performs a manual backflow of changes from VMR's main branch, primarily updating dependency versions across the dotnet/sdk repository from rc.2 to rc.1 versions while maintaining compatibility. The changes include version updates, some code refactoring to remove nullable references, and improvements to build configurations.

Key changes include:

Updating dependency versions from 25427 build to 25461 build across multiple packages
Removing #nullable disable directives and updating property declarations
Refactoring Razor SDK serialization code for better type safety

Reviewed Changes

Copilot reviewed 17 out of 52 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs	Removes nullable annotations and refactors property declarations, updates runtime pack logic
src/RazorSdk/Tool/Json/*.cs	Updates JSON serialization for TagHelper descriptors with improved type safety
src/Layout/redist/targets/*.targets	Updates portable runtime identifier configurations and removes platform-specific conditions
global.json, eng/Version.Details.xml, eng/Version.Details.props	Updates Arcade SDK and dependency package versions
src/Cli/dotnet/*.cs	Updates command line action implementations
eng/common/.	Updates source indexing and SBOM generation package versions

src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

                               {
-                                  var knownFrameworkReferenceRuntimePackRuntimeIdentifiers = selectedPack.RuntimePackRuntimeIdentifiers.Split(';');
+                                  var knownFrameworkReferenceRuntimePackRuntimeIdentifiers = selectedRuntimePack?.RuntimePackRuntimeIdentifiers.Split(';');

Preview

Copilot AI Sep 12, 2025

Potential null reference exception. The code uses null-conditional operator ?. but then immediately calls Split(';') on the result, which could be null. This should either check for null before calling Split or use a safe pattern.

Suggested change

      
                                var knownFrameworkReferenceRuntimePackRuntimeIdentifiers = selectedRuntimePack?.RuntimePackRuntimeIdentifiers.Split(';');
          
                                var knownFrameworkReferenceRuntimePackRuntimeIdentifiers = (selectedRuntimePack?.RuntimePackRuntimeIdentifiers ?? string.Empty).Split(';');

Copilot uses AI. Check for mistakes.

Member Author

premun Sep 12, 2025 •

edited

Loading

@baronfel fyi some usefully-looking tips here from Mr.Copilot towards the file you've been working with recently

Contributor

KalleOlaviNiemitalo Sep 12, 2025

This indeed risks a null reference exception, but Copilot explains it wrong.

selectedRuntimePack?.RuntimePackRuntimeIdentifiers.Split(';')

means

selectedRuntimePack is null ? null : (selectedRuntimePack.RuntimePackRuntimeIdentifiers.Split(';'))

so, if selectedRuntimePack is null, then Split is not called, and NullReferenceException is not thrown from there; knownFrameworkReferenceRuntimePackRuntimeIdentifiers just becomes (string[])null. However, that then causes the foreach loop immediately below to throw NullReferenceException when it tries to enumerate the array.

src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

                                   foreach (var runtimeIdentifier in knownFrameworkReferenceRuntimePackRuntimeIdentifiers)
                                   {
-                                      foreach (var runtimePackNamePattern in selectedPack.RuntimePackNamePatterns.Split(';'))
+                                      foreach (var runtimePackNamePattern in selectedRuntimePack?.RuntimePackNamePatterns.Split(';'))

Preview

Copilot AI Sep 12, 2025

Similar null reference issue as above. The null-conditional operator is used but Split is called on a potentially null result.

Suggested change

      
                                    foreach (var runtimePackNamePattern in selectedRuntimePack?.RuntimePackNamePatterns.Split(';'))
          
                                    foreach (var runtimePackNamePattern in (selectedRuntimePack?.RuntimePackNamePatterns ?? string.Empty).Split(';'))

Copilot uses AI. Check for mistakes.

src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

Comment on lines +978 to +979

		runtimePackVersion = knownRuntimePack?.LatestRuntimeFrameworkVersion;
		return knownRuntimePack?.LatestRuntimeFrameworkVersion;

Preview

Copilot AI Sep 12, 2025

Inconsistent null handling. The code assigns and returns the same nullable value without null checking, which could propagate null values unexpectedly.

Suggested change

      
                                    runtimePackVersion = knownRuntimePack?.LatestRuntimeFrameworkVersion;
          
                                    return knownRuntimePack?.LatestRuntimeFrameworkVersion;
          
                                    runtimePackVersion = knownRuntimePack.LatestRuntimeFrameworkVersion ?? knownFrameworkReference.DefaultRuntimeFrameworkVersion;
          
                                    return knownRuntimePack.LatestRuntimeFrameworkVersion ?? knownFrameworkReference.DefaultRuntimeFrameworkVersion;

Copilot uses AI. Check for mistakes.

src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

                                   {
-                                      runtimePackVersion = knownPack2.LatestRuntimeFrameworkVersion;
+                                      runtimePackVersion = knownRuntimePack?.LatestRuntimeFrameworkVersion;

Preview

Copilot AI Sep 12, 2025

Same issue as above - assigning potentially null value without proper null handling.

Suggested change

      
                                    runtimePackVersion = knownRuntimePack?.LatestRuntimeFrameworkVersion;
          
                                    runtimePackVersion = knownRuntimePack.LatestRuntimeFrameworkVersion ?? knownFrameworkReference.DefaultRuntimeFrameworkVersion;

Copilot uses AI. Check for mistakes.

src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

                       {
-                          string? value = frameworkReference?.GetMetadata("TargetLatestRuntimePatch");
+                          string value = frameworkReference?.GetMetadata("TargetLatestRuntimePatch");

Preview

Copilot AI Sep 12, 2025

The variable value could be null but is used in subsequent string operations without null checking.

Copilot uses AI. Check for mistakes.

src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs

                       {
-                          string? requestedVersion = frameworkReference?.GetMetadata("RuntimeFrameworkVersion");
+                          string requestedVersion = frameworkReference?.GetMetadata("RuntimeFrameworkVersion");

Preview

Copilot AI Sep 12, 2025

Similar issue - requestedVersion could be null but is used without proper null checking in the following code.

Copilot uses AI. Check for mistakes.

eng/common/sdk-task.sh Show resolved Hide resolved

Member Author

premun commented Sep 12, 2025

@marcpopMSFT @SimonZhao888 @mmitche @akoeplinger I still had to manually backflow as SDK is the place where the metadata was messed up (so the manual forward flow was not enough).

The changes here mostly come from this VMR commit: dotnet/dotnet@73c9eac

I expect this PR might suffer from similar problems like the 10.0.1xx one so

We should fix those in [release/10.0.1xx] Source code updates from dotnet/dotnet #50648
Then merge release/10.0.1xx into main
Merge main into this PR

build-analysis bot mentioned this pull request

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

Member

marcpopMSFT commented Sep 12, 2025

Lot of failures are C:\h\w\AA3B0949\p\d\sdk\10.0.100-ci\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(119,5): error MSB4018: System.ArgumentNullException: Value cannot be null. (Parameter 'key')

I think we need baronfel's change that was in 10.0.1xx

Member

SimonZhao888 commented Sep 15, 2025

Here is baronfel's commit: Fix process framework references crash when null RID is passed

Member Author

premun commented Sep 15, 2025 •

edited

Loading

@marcpopMSFT that change was already part of the branch here though as it merged into main

build-analysis bot mentioned this pull request

GenerateBundle Task fails writing to singlefilehost.exe #50784

Open

premun added 4 commits

September 15, 2025 13:43


          Manually backflow VMR's main

10f37f5

Backflowing dotnet/dotnet@5d79341

dotnet/arcade-services#5231


          Revert modes

c6e4528


          Revert modes

85825e6


          Fix the build

f2bc305

marcpopMSFT force-pushed the prvysoky/backflow-main branch from 57b6a3a to f2bc305 Compare

September 15, 2025 20:43

Member

marcpopMSFT commented Sep 15, 2025

That change has flowed to main outside this PR now so rebasing and rerunning to see if that helps.

Member Author

premun commented Sep 16, 2025

I merged main into the PR here before so probably not going to change much


          Merge branch 'main' into prvysoky/backflow-main

8a577a1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Infrastructure