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

Update TFM to net7.0 for source-build #4742

Merged
merged 9 commits into from Sep 7, 2022
Merged

Conversation

lbussell
Copy link
Contributor

@lbussell lbussell commented Aug 1, 2022

Bug

Fixes: NuGet/Home#12000

Regression? Last working version: Not a regression.

Description

I changed all source-build conditioned occurrences of net6.0 TFMs to net7.0.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@lbussell lbussell requested a review from a team as a code owner August 1, 2022 22:45
@ghost ghost added the Community PRs created by someone not in the NuGet team label Aug 1, 2022
@lbussell
Copy link
Contributor Author

lbussell commented Aug 1, 2022

cc @crummel

<NETCoreTargetFramework Condition="'$(DotNetBuildFromSource)' == 'true'">net6.0</NETCoreTargetFramework>
<NETCoreTestTargetFramework>net6.0</NETCoreTestTargetFramework>
<NETCoreTargetFramework Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0</NETCoreTargetFramework>
<NETCoreTestTargetFramework>net7.0</NETCoreTestTargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Please don't change the target framework in non source build scenarios.
This can cause our builds to fail.

@nkolev92
Copy link
Member

nkolev92 commented Aug 1, 2022

@lbussell
Can you please keep the PR template and create an issue in the home repo as per https://github.com/NuGet/NuGet.Client/blob/dev/CONTRIBUTING.md#contributing-step-by-step

@lbussell
Copy link
Contributor Author

lbussell commented Aug 2, 2022

Can you please keep the PR template and create an issue in the home repo

@nkolev92, I filed an issue, updated the PR body with the template, and addressed your review comments - is there a reason the PR validation isn't running?

@nkolev92
Copy link
Member

nkolev92 commented Aug 2, 2022

Thanks for the updates @lbussell.

The CI doesn't run automatically for non-NuGet members. I've applied the label and kicked off the validation.

@aortiz-msft aortiz-msft self-requested a review August 4, 2022 20:24
aortiz-msft
aortiz-msft previously approved these changes Aug 4, 2022
dominoFire
dominoFire previously approved these changes Aug 5, 2022
Copy link
Contributor

@dominoFire dominoFire left a comment

Choose a reason for hiding this comment

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

One question, which should not block source-build changes.

@dominoFire
Copy link
Contributor

dominoFire commented Aug 10, 2022

@lbussell There are some compiler errors:

D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\FrameworkConstants.cs(15,40): error RS0016: Symbol 'Version7' is not part of the declared API. [D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\NuGet.Frameworks.csproj]
D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\FrameworkConstants.cs(190,51): error RS0016: Symbol 'Net70' is not part of the declared API. [D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\NuGet.Frameworks.csproj]
D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\PublicAPI.Unshipped.txt(2,1): error RS0017: Symbol 'NuGet.Frameworks.FrameworkConstants.Version7 -> Version' is part of the declared API, but is either not public or could not be found [D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\NuGet.Frameworks.csproj]
D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\FrameworkConstants.cs(15,40): error RS0016: Symbol 'Version7' is not part of the declared API. [D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\NuGet.Frameworks.csproj]
D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\FrameworkConstants.cs(190,51): error RS0016: Symbol 'Net70' is not part of the declared API. [D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\NuGet.Frameworks.csproj]
D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\PublicAPI.Unshipped.txt(2,1): error RS0017: Symbol 'NuGet.Frameworks.FrameworkConstants.Version7 -> Version' is part of the declared API, but is either not public or could not be found [D:\a\_work\1\s\src\NuGet.Core\NuGet.Frameworks\NuGet.Frameworks.csproj]

@nkolev92
Copy link
Member

🔔 @lbussell

@lbussell
Copy link
Contributor Author

I'm not sure if I've got the unshipped API format correct - can anyone give me some pointers?

@zivkan
Copy link
Member

zivkan commented Aug 27, 2022

The roslyn analyzer code fix can create the right syntax for you. You're almost certainly not using Windows and therefore Visual Studio is not an option for you, where roslyn analyzers and code fixes run by default. For reasons I don't understand, Omnisharp (or VSCode?) have them disabled by default, but in VSCode's settings I see an option "Omnisharp: Enably Roslyn Analyzers".

@zivkan
Copy link
Member

zivkan commented Aug 27, 2022

Also, if you build locally from the command line (cd to src\NuGet.Core\NuGet.Frameworks, then run dotnet build), the build should fail if the syntax isn't right.

@@ -186,6 +187,7 @@ public static class CommonFrameworks
// .NET 5.0 and later has NetCoreApp identifier
public static readonly NuGetFramework Net50 = new NuGetFramework(FrameworkIdentifiers.NetCoreApp, Version5);
public static readonly NuGetFramework Net60 = new NuGetFramework(FrameworkIdentifiers.NetCoreApp, Version6);
public static readonly NuGetFramework Net70 = new NuGetFramework(FrameworkIdentifiers.NetCoreApp, Version7);
Copy link
Member

Choose a reason for hiding this comment

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

You can always just not add this change as it's not really anything that requires your change. this is merely an optimization for restore.

@lbussell
Copy link
Contributor Author

I was able to get the project loaded in VS and I included the updates it suggested.

@lbussell
Copy link
Contributor Author

Not sure what the current PR validation failure is - it doesn't look related to my changes:

https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6620811&view=logs&j=68fd25e5-7968-5c7b-bc0b-13c16b2edd08&t=2fd8befc-7898-5b33-2bea-d02ab5970aca&l=10

@nkolev92
Copy link
Member

@lbussell If you rebase it should get fixed.

Ping us here so we can trigger the CI when you've rebased.

@lbussell
Copy link
Contributor Author

lbussell commented Sep 6, 2022

I updated the PR again, can we run CI again?

It also looks like the last failure was a timeout - unless you think that's something my change could have caused.

@lbussell
Copy link
Contributor Author

lbussell commented Sep 6, 2022

@nkolev92

@lbussell
Copy link
Contributor Author

lbussell commented Sep 6, 2022

I also re-ran failed legs from azure devops, looks like mac didn't timeout this time.

@lbussell
Copy link
Contributor Author

lbussell commented Sep 7, 2022

Current test failure looks the same as this PR: #4788

@nkolev92 nkolev92 merged commit a547d19 into NuGet:dev Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
5 participants