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

Retarget to .NET 9 #10451

Closed

Conversation

f-alizada
Copy link
Contributor

@f-alizada f-alizada commented Jul 26, 2024

Fixes #10280

Context

.NET 9 is in preview 6.
Consuming the latest Arcade + retargeting the the msbuild core version to .net 9.

Changes Made

Testing

src/Tasks/ManifestUtil/ManifestReader.cs Show resolved Hide resolved
src/Tasks/ManifestUtil/PathUtil.cs Show resolved Hide resolved
src/Utilities/README.md Outdated Show resolved Hide resolved
src/Shared/FileMatcher.cs Outdated Show resolved Hide resolved
src/Shared/FileMatcher.cs Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
src/Build.UnitTests/BackEnd/BuildManager_Tests.cs Outdated Show resolved Hide resolved
@f-alizada f-alizada requested a review from rainersigwald August 2, 2024 16:13
@f-alizada f-alizada marked this pull request as ready for review August 2, 2024 16:13
@f-alizada f-alizada requested a review from a team as a code owner August 2, 2024 16:13
@f-alizada f-alizada requested a review from a team August 2, 2024 16:13
@@ -98,7 +91,6 @@
<Dependency Name="Microsoft.Net.Compilers.Toolset" Version="4.11.0-3.24378.3">
<Uri>https://github.com/dotnet/roslyn</Uri>
<Sha>5e3a11e2e7f952da93f9d35bd63a2fa181c0608b</Sha>
<SourceBuild RepoName="roslyn" ManagedOnly="true" />
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After new arkade sdk was consumed, the source build was failing on this one, with errors: https://dev.azure.com/dnceng-public/public/_build/results?buildId=762782&view=logs&j=c316b76e-5b95-5e79-2740-62e92f13eebd&t=e33df8ad-5872-5105-0672-039423de0ed3 Only place elements on source-build intermediate nupkgs.

Copy link
Member

Choose a reason for hiding this comment

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

The correct fix is to add the corresponding soucebuild intermediate dependency (ideally right after this dependency)

    <Dependency Name="Microsoft.SourceBuild.Intermediate.roslyn" Version="4.11.0-3.24378.3">
      <Uri>https://github.com/dotnet/roslyn</Uri>
      <Sha>5e3a11e2e7f952da93f9d35bd63a2fa181c0608b</Sha>
      <SourceBuild RepoName="roslyn" ManagedOnly="true" />
    </Dependency>

Then delete the change to https://github.com/dotnet/msbuild/pull/10451/files#diff-f88ce4026366ce74fb1a981785c542757617abf4393a33ca34465a5e9a0dd343

global.json Outdated
"vs": {
"version": "17.8.0"
},
"xcopy-msbuild": "17.8.5"
},
"msbuild-sdks": {
"Microsoft.DotNet.Arcade.Sdk": "8.0.0-beta.24376.1"
"Microsoft.DotNet.Arcade.Sdk": "9.0.0-beta.24379.1"
Copy link
Member

Choose a reason for hiding this comment

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

Have we confirmed with dnceng that this is what they want for our LTS branch?

Copy link
Contributor Author

@f-alizada f-alizada Aug 2, 2024

Choose a reason for hiding this comment

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

[Fact]
public void TryConvertToLongGivenDoubleWithLongMaxValue()
[WindowsFullFrameworkOnlyFact]
public void TryConvertToLongGivenDoubleWithLongMaxValueFramework()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand this test change. Can you elaborate on the reasoning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test previously introduced here 0f6ed65
Testing different behaviour on different archtitecture.
However after moving to .net 9 the precision is not lost hence we are receveing the expected outcome of what was written:

result.ShouldBeTrue();
actual.ShouldBe(longMaxValue);

This is true only for .NET 9 target, and the Framework left as it is.

fs.ReadExactly(keyBytes, 0, fileLength);
#else
#pragma warning disable CA2022 // Avoid inexact read with 'Stream.Read'
// TODO: Read the count of read bytes and check if it matches the expected length, if not raise an exception
fs.Read(keyBytes, 0, fileLength);
Copy link
Member

Choose a reason for hiding this comment

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

I'd still prefer a polyfill here so the source can just unconditionally say ReadExactly

Copy link
Member

Choose a reason for hiding this comment

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

Let's track that than. It feels that it'd be outside of scope of retargetting change.

<!-- For ease of logging the "not supported on Core" message, these tasks are a
TaskRequiresFramework on netstandard/netcore. Since the type is sealed there,
that shouldn't cause any implementation problems since no one can derive
from it and try to call TaskExtension.Log. -->
Copy link
Member

Choose a reason for hiding this comment

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

Please preserve the comments where the suppressions aren't removed.

src/Tasks/CompatibilitySuppressions.xml Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Excluding the changes in eng folder (I haven't reviewed those) and the commented stuff it looks OK (great job!).
The VS insertion looks OK as well (both tests failures are expected and known, and this change even brings some nice perf improvements - 👏).

The only thing missing for me for signoff (appart from commented stuff) is a list of manual changes to eng folder that has been done beyond arcade consuming (if any beyond already described and #10451 (comment))

dotnet_diagnostic.IDE0100.severity = suggestion

# File header should match the template, making it error since couple of files met in the code base without any header
dotnet_diagnostic.IDE0073.severity = error
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Comment on lines 635 to 643
if (data.Mode.HasValue)
{
_env.SetEnvironmentVariable("MSBUILD_SDKREFERENCE_PROPERTY_EXPANSION_MODE", data.Mode.ToString());
}
else
{
_env.SetEnvironmentVariable("MSBUILD_SDKREFERENCE_PROPERTY_EXPANSION_MODE", null);
}

Copy link
Member

Choose a reason for hiding this comment

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

Can nullcheck operator be used here?

Suggested change
if (data.Mode.HasValue)
{
_env.SetEnvironmentVariable("MSBUILD_SDKREFERENCE_PROPERTY_EXPANSION_MODE", data.Mode.ToString());
}
else
{
_env.SetEnvironmentVariable("MSBUILD_SDKREFERENCE_PROPERTY_EXPANSION_MODE", null);
}
_env.SetEnvironmentVariable("MSBUILD_SDKREFERENCE_PROPERTY_EXPANSION_MODE", data.Mode.?ToString());

<Left>ref/net8.0/Microsoft.Build.dll</Left>
<Right>ref/net8.0/Microsoft.Build.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
<DiagnosticId>PKV006</DiagnosticId>
Copy link
Member

Choose a reason for hiding this comment

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

This might deserve a comment

src/Build/CompatibilitySuppressions.xml Show resolved Hide resolved
fs.ReadExactly(keyBytes, 0, fileLength);
#else
#pragma warning disable CA2022 // Avoid inexact read with 'Stream.Read'
// TODO: Read the count of read bytes and check if it matches the expected length, if not raise an exception
fs.Read(keyBytes, 0, fileLength);
Copy link
Member

Choose a reason for hiding this comment

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

Let's track that than. It feels that it'd be outside of scope of retargetting change.

@rainersigwald
Copy link
Member

Excluding the changes in eng folder

You can't skip eng/, only eng/common! We have scary stuff in there :)

@f-alizada
Copy link
Contributor Author

f-alizada commented Aug 5, 2024

Closing this PR with respect to the new PR with more clear history and less changes: #10484
A lot of learnings from here :) Thank you everyone for review

@f-alizada f-alizada closed this Aug 5, 2024
@JanKrivanek JanKrivanek mentioned this pull request Aug 6, 2024
2 tasks
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.

Retarget to .NET 9
4 participants