-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Retire .NET 5 #69911
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsHello there folks.
|
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.
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.
cff0377
to
c6879ad
Compare
@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. |
584859e
to
6d547fa
Compare
Thank you for help, I will complete this ASAP. |
src/workloads/workloads.csproj
Outdated
<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> |
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.
@am11 Could you please check 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.
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.
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.
Thanks for the information, I couldn't have done it without you 😍
Ok, I'll revert 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.
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.
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.
Thanks Viktor @ViktorHofer I changed it to 7.0
@am11 Is it okay to revert these 2 files? |
You don't have to revert |
Okay Thanks. |
Thanks, Done. |
73e2486
to
84bdf1e
Compare
src/installer/managed/Microsoft.NET.HostModel/Bundle/TargetInfo.cs
Outdated
Show resolved
Hide resolved
Wasm tests are still failing. I suggest you revert all changes under 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 |
Thanks @am11 , Done. I'll open an issue after this PR. |
a3231bf
to
03433cc
Compare
Hello there |
…tion_CurrentDirectoryIsIgnored
Could you please review my changes? |
src/installer/managed/Microsoft.NET.HostModel/Bundle/TargetInfo.cs
Outdated
Show resolved
Hide resolved
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.
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 |
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 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"); |
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.
@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?
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.
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
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.
Looks reasonable / better than the current hard-coding - let's do that.
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.
Done, Thanks @kasperk81 @elinor-fung
src/installer/managed/Microsoft.NET.HostModel/Bundle/Manifest.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Elinor Fung <[email protected]>
@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}"; |
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.
@kasperk81 @elinor-fung if you folks have any recommendations about this variable name I'm all ears.
Co-authored-by: kasperk81 <[email protected]>
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.
Thanks, @ArminShoeibi!
@am11 Thanks for your help. |
Thanks for your contribution! :) |
Hello there folks.
This pull request fixes: #69028