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

Retire .NET 5 #69911

Merged
merged 35 commits into from
Oct 27, 2022
Merged

Retire .NET 5 #69911

merged 35 commits into from
Oct 27, 2022

Conversation

ArminShoeibi
Copy link
Contributor

Hello there folks.
This pull request fixes: #69028

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 27, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 27, 2022
@ghost
Copy link

ghost commented May 27, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Hello there folks.
This pull request fixes: #69028

Author: ArminShoeibi
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@dnfadmin
Copy link

dnfadmin commented May 27, 2022

CLA assistant check
All CLA requirements met.

@danmoseley
Copy link
Member

@ViktorHofer

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Changes to files that reside under eng/common/ shouldn't be made in this repo but instead in https://github.com/dotnet/arcade which then mirrors then changes back to dotnet/runtime and other repos.

@am11
Copy link
Member

am11 commented Jul 1, 2022

@ArminShoeibi, I have made a patch for the instances which I think are enough to accomplish the purpose of this PR: am11@b8c9946 (on top of your branch main...am11:feature/infrasturcture/retire-net5). Feel free to cherry-pick it and continue with testing.

@ArminShoeibi
Copy link
Contributor Author

@ArminShoeibi, I have made a patch for the instances which I think are enough to accomplish the purpose of this PR: am11@b8c9946 (on top of your branch main...am11:feature/infrasturcture/retire-net5). Feel free to cherry-pick it and continue with testing.

Okay, Thank you.

@ArminShoeibi
Copy link
Contributor Author

ArminShoeibi commented Jul 1, 2022

@ArminShoeibi, I have made a patch for the instances which I think are enough to accomplish the purpose of this PR: am11@b8c9946 (on top of your branch main...am11:feature/infrasturcture/retire-net5). Feel free to cherry-pick it and continue with testing.

Thank you for help, I will complete this ASAP.
I had some problems with generating this test again but I gave up.
https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/StaticTestGenerator/README.md

@ArminShoeibi ArminShoeibi changed the title Rev .NET 5 to net6.0 or $(NetCoreAppCurrent) Retire .NET 5 Jul 1, 2022
<Import Sdk="Microsoft.NET.Sdk" Project="Sdk.props" />

<PropertyGroup>
<MicrosoftDotNetBuildTasksInstallersTaskTargetFramework Condition="'$(MSBuildRuntimeType)' == 'Core'">net7.0</MicrosoftDotNetBuildTasksInstallersTaskTargetFramework>
<MicrosoftDotNetBuildTasksInstallersTaskTargetFramework Condition="'$(DotNetBuildFromSource)' == 'true'">net5.0</MicrosoftDotNetBuildTasksInstallersTaskTargetFramework>
<MicrosoftDotNetBuildTasksInstallersTaskTargetFramework Condition="'$(DotNetBuildFromSource)' == 'true'">net6.0</MicrosoftDotNetBuildTasksInstallersTaskTargetFramework>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@am11 Could you please check this?

Copy link
Member

Choose a reason for hiding this comment

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

This is for source-build, which is why I left it out from the patch. I think it can be updated later, when source-build requires it.

The end-to-end test for source build runs in dotnet/installer repo; first the PRs are merged in dotnet/runtime repo, dotnet/sdk is updated with latest runtime, and finally installer repo is updated with SDK where source-build happens in the CI. Therefore, it is not immediately obvious whether it is OK to make this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the information, I couldn't have done it without you 😍
Ok, I'll revert this.

Copy link
Member

Choose a reason for hiding this comment

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

The Installers package uses net7.0 in both the normal build and the source build: https://github.com/dotnet/arcade/blob/8e8cab10677494713f64937650d4996782223740/src/Microsoft.DotNet.Build.Tasks.Installers/build/Microsoft.DotNet.Build.Tasks.Installers.props#L3. Ideally we would not hardcode the tfm here at all, but we can do that in a separate change if we would like to.

Copy link
Contributor Author

@ArminShoeibi ArminShoeibi Jul 2, 2022

Choose a reason for hiding this comment

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

Thanks Viktor @ViktorHofer I changed it to 7.0

@ArminShoeibi
Copy link
Contributor Author

Changes to files that reside under eng/common/ shouldn't be made in this repo but instead in https://github.com/dotnet/arcade which then mirrors then changes back to dotnet/runtime and other repos.

@am11 Is it okay to revert these 2 files?
eng/common/SetupNugetSources.ps1
eng/common/SetupNugetSources.sh

@am11
Copy link
Member

am11 commented Jul 1, 2022

You don't have to revert eng/common in this PR, as long as you open a parallel PR in arcade repo with eng/common changes.
Regardless of the order these PRs are merged, changes will eventually synchronize.

@am11 am11 closed this Jul 1, 2022
@am11 am11 reopened this Jul 1, 2022
@ArminShoeibi
Copy link
Contributor Author

You don't have to revert eng/common in this PR, as long as you open a parallel PR in arcade repo with eng/common changes. Regardless of the order these PRs are merged, changes will eventually synchronize.

Okay Thanks.

@ArminShoeibi
Copy link
Contributor Author

ArminShoeibi commented Jul 2, 2022

You don't have to revert eng/common in this PR, as long as you open a parallel PR in arcade repo with eng/common changes. Regardless of the order these PRs are merged, changes will eventually synchronize.

Thanks, Done.
dotnet/arcade#9866

@am11
Copy link
Member

am11 commented Jul 2, 2022

Wasm tests are still failing. I suggest you revert all changes under src/tests. The WebAssembly SDK has many changes since .NET 5, so it is a non-travial change. You will need to investigate how the SDK targets were evolved and which properties to set to make it pass etc.

To revert:

git remote add dotnet https://github.com/dotnet/runtime
git fetch --all

git checkout dotnet/main src/tests
# commit and push

So lets revert src/tests, open a follow up issue for Blazor_net50 update for the future.

@ArminShoeibi
Copy link
Contributor Author

ArminShoeibi commented Jul 2, 2022

Thanks @am11 , Done. I'll open an issue after this PR.

@ArminShoeibi
Copy link
Contributor Author

Hi @ArminShoeibi - just checking in here. Are you still working on this?

Hello there
Yes, I'm going to sync my branch with main and after that I will resolve comments.

@ArminShoeibi
Copy link
Contributor Author

Hi @ArminShoeibi - just checking in here. Are you still working on this?

Could you please review my changes?
Thanks for your hints

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Looking good - I think we just have some minor comments left.

@@ -2,7 +2,7 @@
# This file should be removed as part of this issue: https://github.com/dotnet/arcade/issues/4080
#
# What the script does is iterate over all package sources in the pointed NuGet.config and add a credential entry
# under <packageSourceCredentials> for each Maestro managed private feed. Two additional credential
# under <packageSourceCredentials> for each Maestro managed private feed. Two additional credential
Copy link
Member

Choose a reason for hiding this comment

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

I would just revert these whitespace-only changes in eng/common.

@@ -80,7 +80,7 @@ public static int CompileNuget(BuildOptions options)
}

// This is not a reliable way of building the publish folder
string publishFolder = Path.Combine(appFolder, @"artifacts\Debug\net5.0\publish");
string publishFolder = Path.Combine(appFolder, @"artifacts\Debug\net6.0\publish");
Copy link
Member

Choose a reason for hiding this comment

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

@trylek I'm not sure what the expected tfm corresponds to here. From what I can tell, it is running dotnet new without specifying a framework, so it will be whatever the default is for the dotnet it picked up? Is just changing this to net6.0 fine here?

Copy link
Contributor

Choose a reason for hiding this comment

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

it will be whatever the default is for the dotnet it picked up?

if so, then this will work here too:

GetCsprojTemplate($"net{version.Major}.{version.Minor}")

Version version = Environment.Version;
string _framework = $"net{version.Major}.{version.Minor}"
DotnetCli.New(.. -f _framework)
string publishFolder = Path.Combine(appFolder, $@"artifacts\Debug\{_framework}\..

for net5 and above. netcore3.1's l.t.s will end in mid dec, main branch can consider it retired as well

Copy link
Member

Choose a reason for hiding this comment

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

Looks reasonable / better than the current hard-coding - let's do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, Thanks @kasperk81 @elinor-fung

@elinor-fung
Copy link
Member

@ArminShoeibi could you resolve the merge conflicts when you get the chance?

Other than that, I believe #69911 (comment) is the remaining thing to address.

@ArminShoeibi
Copy link
Contributor Author

@ArminShoeibi could you resolve the merge conflicts when you get the chance?

Other than that, I believe #69911 (comment) is the remaining thing to address.

Sure, I'll do it today.

@@ -58,7 +58,9 @@ public static int CompileNuget(BuildOptions options)
string appFolder = Path.Combine(nugetOutputFolder, $"{package}.TestApp");
Directory.CreateDirectory(appFolder);

int exitCode = DotnetCli.New(appFolder, "console", nugetLog);
Version version = Environment.Version;
string targetFramework = $"net{version.Major}.{version.Minor}";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kasperk81 @elinor-fung if you folks have any recommendations about this variable name I'm all ears.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Thanks, @ArminShoeibi!

@elinor-fung elinor-fung merged commit f4a6849 into dotnet:main Oct 27, 2022
@ArminShoeibi ArminShoeibi deleted the arminshoeibi/rev-net5 branch October 27, 2022 19:43
@ArminShoeibi
Copy link
Contributor Author

@am11 Thanks for your help.

@am11
Copy link
Member

am11 commented Oct 27, 2022

Thanks for your contribution! :)

@ghost ghost locked as resolved and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review .NET 5 usages
8 participants