- 
                Notifications
    You must be signed in to change notification settings 
- Fork 715
Update to .NET 10 SDK and Arcade 10 #10075
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
Conversation
| Setting as a draft as I'm queuing an internal build to see if official build works | 
| Does this mean we could target  | 
| 
 If we wanted to, yes 😃 | 
| We should also run an internal build, and a full azdo build with tests to validate this. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades the repository to use the .NET 10 SDK and Arcade 10, updating various build scripts, dependency configurations, and CI/CD pipeline templates.
- Update target framework in csproj and global.json to net10.0 and update dotnet/arcade versions.
- Modify build scripts (bash and PowerShell) to reflect new property names and parameters (e.g. DotNetBuild and DotNetBuildFromVMR).
- Adjust dependency versions and SHA sums in files like Version.Details.xml and Versions.props, plus update NuGet and workflow settings for .NET10.
Reviewed Changes
Copilot reviewed 56 out of 58 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description | 
|---|---|
| tools/GenerateTestSummary/GenerateTestSummary.csproj | Updated target framework from net9.0 to net10.0 | 
| global.json | Updated dotnet version and tool versions to .NET 10 SDK preview | 
| eng/common/darc-init.sh | Updated package source URL for arcade services | 
| eng/Version.Details.xml | Updated Arcade dependency versions and corresponding SHA sums | 
| eng/common/build.sh | Changed MSBuild property names (DotNetBuildRepo → DotNetBuild) and added DotNetBuildFromVMR | 
| Various workflow and CI templates | Updated pipelines and NuGet.config to reference .NET 10 sources | 
Comments suppressed due to low confidence (3)
eng/Version.Details.xml:153
- Confirm that the updated dependency versions and SHA sums in Version.Details.xml are correct and that they match the intended Arcade 10 releases.
    </Dependency>
eng/common/build.sh:248
- Ensure that updating the property name from 'DotNetBuildRepo' to 'DotNetBuild' and introducing 'DotNetBuildFromVMR' in build.sh is aligned with the rest of the build system to prevent configuration mismatches.
    /p:Build=$build \
| Let’s gooo! | 
| New internal build - https://dev.azure.com/dnceng/internal/_build/results?buildId=2738295&view=results | 
| @joperezr would this work fine with VS/VS code? | 
`AzureFunctionsEndToEnd.AppHost.csproj : error NU1010: The following PackageReference items do not define a corresponding PackageVersion item: Aspire.Hosting.Azure.AppContainers. Projects using Central Package Management must declare PackageReference and PackageVersion items with matching names. For more information, visit https://aka.ms/nuget/cpm/gettingstarted `
…he corresponding test change
| Pushed some fixes. New internal build - https://dev.azure.com/dnceng/internal/_build/results?buildId=2738413&view=results | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!!
| @joperezr could you please review my additional changes to the PR before merging? | 
# Conflicts: # eng/clipack/Common.projitems
| <AdditionalProperties Include="SelfContained=true" /> | ||
|  | ||
| <!-- Windows pdb aren't really needed any more. See https://github.com/dotnet/arcade/issues/15724 | ||
| As the pdb file is in the same location for all the RIDs it causes arcade's `_DeployPortableSymbolsToSymStore` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the right fix here to use different publish locations for different rids?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Publish directories are separate, and even the build dirs for the .dll but not for the .pdb! The .pdb files wouldn't be different per-rid so I think them being in the same location makes sense. We could fix this in arcade somehow, but apparently this whole thing (windows pdbs) is expected to be removed anyway. See dotnet/arcade#15724 and dotnet/arcade#15736 .
| <DotNetSdkPreviousVersionForTesting>8.0.406</DotNetSdkPreviousVersionForTesting> | ||
| <!-- dotnet 10.0 versions for running tests - used for templates tests --> | ||
| <DotNetSdkNextVersionForTesting>10.0.100-preview.5.25277.114</DotNetSdkNextVersionForTesting> | ||
| <DotNetSdkCurrentVersionForTesting>9.0.200</DotNetSdkCurrentVersionForTesting> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not the current version? Should we rename the property instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These names are arbitrary really. We are following Previous, Current, Next to follow 3 active versions. 9.0 being Current works here allowing Previous=8.0 and Current=10.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the same nomenclature as dotnet/runtime:
$(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum)
Current = 10
Previous = 9
Minimum = 8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe that only makes sense if we are on 10 as well. So maybe having Next be 10 until we are version 10...
| { templateName, extraArgs, TestSdk.Current, TestTargetFramework.Current, null }, | ||
| // Current SDK, Next TFM | ||
| { templateName, extraArgs, TestSdk.Current, TestTargetFramework.Next, "The current .NET SDK does not support targeting .NET 10.0" }, | ||
| { templateName, extraArgs, TestSdk.Current, TestTargetFramework.Next, null }, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep the current TFM actually be current (meaning 10.x) and then removing the expected error? (which is what I had) That way we are no validating that with a .NET 10 SDK you can build an aspire app targeting 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on what is the missing case here, or the incorrect one?
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>net9.0</TargetFramework> | ||
| <TargetFramework>net10.0</TargetFramework> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a property for this? like $(NetCoreAppCurrent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, not sure if I'd block merging for this, so if this is the only remaining feedback I'd vote for doing this in the next PR
Description
Upgrading to use the .NET 10 SDK as well as Arcade 10. This will be helpful for some signing things we are doing plus taking advantage of new features in the SDK.
cc: @radical
Checklist