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

Support .NET 7.0 #3329

Merged
merged 16 commits into from
Nov 9, 2022
Merged

Support .NET 7.0 #3329

merged 16 commits into from
Nov 9, 2022

Conversation

zcsizmadia
Copy link
Contributor

@zcsizmadia zcsizmadia commented Sep 16, 2022

.NET 7.0 just went RC (rc.1) and expected to be officially released in mid November 2022. This PR should be merged once .NET 7.0 is released on November, however in the meantime the build process can be validated.

Note:
Before merging include-prerelease: true lines should be removed from the github workflow actions/setup-dotnet@v2 actions.

@github-actions github-actions bot added CI CI configuration issue or pull request performance Issue or pull request about performance problems test Pull request that adds new or changes existing tests labels Sep 16, 2022
@zcsizmadia
Copy link
Contributor Author

.NET Core 3.1 will go EOL mid December 2022 and .NET 5.0 is already EOL (https://dotnet.microsoft.com/en-us/download/dotnet)

Those targets could be removed as well when NET 7 is released. Supporting those EOL frameworks will be ok using the netstandard2.x targets.

@Shane32
Copy link
Member

Shane32 commented Sep 16, 2022

.NET Core 3.1 will go EOL mid December 2022 and .NET 5.0 is already EOL (https://dotnet.microsoft.com/en-us/download/dotnet)

Those targets could be removed as well when NET 7 is released. Supporting those EOL frameworks will be ok using the netstandard2.x targets.

GraphQL.NET only targets .NET Standard 2.0, .NET Standard 2.1 and .NET 6, as there are no ifdefs for other target frameworks. System.Text.Json includes a target for .NET Core 3.1 because System.Text.Json is included with .NET Core 3.0+. As such, the dependency can be eliminated.

At present I see no reason to change the targeted frameworks. I feel that .NET Core 3.1 is especially important because not only it is a LTS release, a number of the packages developed at that time are cross-platform compatible with .NET Framework -- for example, Entity Framework Core 3.1.x. While perhaps these applications can all run on .NET 6, when using the 3.1.x components of that time, it may be more likely that applications written for .NET Core 3.1 designed to be cross compatible with .NET Framework may not yet be upgraded to .NET 6. There is a similar situation that exists with .NET Core 2.1 for web applications -- hence why GraphQL.NET Server 7.x now supports .NET Framework and .NET Core 2.1 web applications. (Note that in that situation, there is official support from Microsoft for ASP.NET Core 2.1.x when run on supported editions of .NET Framework.)

I would have little argument against removing .NET 5 code -- it was never a LTS release and is more of a preview of .NET 6, just like .NET Core 3.0 was compared to .NET Core 3.1. But yet, there is no reason to at this time.

If and when it becomes troublesome to perform development/testing for older frameworks (e.g. trying to develop/test .NET Framework 4.5 applications today), we will likely drop those frameworks from the test suite and/or libraries at that time.

Thanks for the PR! As you suggest, we likely will not merge the PR until .NET 7.0 is released.

@sungam3r
Copy link
Member

sungam3r commented Nov 6, 2022

Those targets could be removed as well when NET 7 is released. Supporting those EOL frameworks will be ok using the netstandard2.x targets.

Well, I'm not so positive to quickly remove support for "old" TFMs especially if this support is given at low price.

@sungam3r
Copy link
Member

sungam3r commented Nov 6, 2022

I would have little argument against removing .NET 5 code -- it was never a LTS release and is more of a preview of .NET 6, just like .NET Core 3.0 was compared to .NET Core 3.1. But yet, there is no reason to at this time.

If and when it becomes troublesome to perform development/testing for older frameworks (e.g. trying to develop/test .NET Framework 4.5 applications today), we will likely drop those frameworks from the test suite and/or libraries at that time.

Thanks for the PR! As you suggest, we likely will not merge the PR until .NET 7.0 is released.

Agree. Let's merge ASAP after NET7 release.

@@ -39,10 +39,11 @@ jobs:
name: Documentation
path: docs2/public/**
if-no-files-found: error
- name: Setup .NET Core 6.0 SDK
- name: Setup .NET 7.0 SDK
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Setup .NET 7.0 SDK
- name: Setup .NET SDK

@@ -16,10 +16,11 @@ jobs:
- name: Checkout source
uses: actions/checkout@v3

- name: Setup .NET Core SDK
- name: Setup .NET 7.0 SDK
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Setup .NET 7.0 SDK
- name: Setup .NET SDK

@@ -46,10 +46,11 @@ jobs:
name: Documentation
path: docs2/public/**
if-no-files-found: error
- name: Setup .NET Core 6.0 SDK
- name: Setup .NET 7.0 SDK
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Setup .NET 7.0 SDK
- name: Setup .NET SDK

Copy link
Member

@sungam3r sungam3r left a comment

Choose a reason for hiding this comment

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

I'm fine to merge this one after resolving conflicts and .NET7 release.

@zcsizmadia
Copy link
Contributor Author

zcsizmadia commented Nov 6, 2022

@sungam3r Merged master into the PR:

  1. Using EOL SDKs, the build will generate several EOL warnings. As of now it is only 5.0, however 3.1 goes EOL soon, so that will do the same. Added <CheckEolTargetFramework>false</CheckEolTargetFramework> to Directory.build.props to disable that warning. (EOL SDKs are used only for tests and benchmarks)
  2. dotnet-quality: preview should be removed from the github workflow yml files before merging this PR. NET 7 is expected to be released later next week. As soon as it is out, I will remove it and do a final build/test without allowing preview SDK.

@sungam3r
Copy link
Member

sungam3r commented Nov 6, 2022

CheckEolTargetFramework is fine, we use it in the server project as well.

src/Directory.Build.props Outdated Show resolved Hide resolved
@zcsizmadia
Copy link
Contributor Author

@Shane32

  1. Removed the comments for CheckEolTargetFramework
  2. Removed the ApolloTrace AddTicks fix. The tests in this PR will fail until Fix accuracy of Apollo trace endTime property #3384 is not merged.

@sungam3r
Copy link
Member

sungam3r commented Nov 7, 2022

I see growing mails from dependabot updating different stuff to 7.0.0 😮 but I don't see 7.0.0 release on MS site yet.

@codecov-commenter
Copy link

Codecov Report

Merging #3329 (01c4bf0) into master (06d7bd7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3329   +/-   ##
=======================================
  Coverage   83.84%   83.84%           
=======================================
  Files         376      376           
  Lines       16274    16274           
  Branches     2610     2613    +3     
=======================================
  Hits        13645    13645           
  Misses       2011     2011           
  Partials      618      618           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zcsizmadia
Copy link
Contributor Author

zcsizmadia commented Nov 8, 2022

NET 7 was released today. Merged master into the PR branch and removed the dotnet-quality=preview lines from the github workflows.

@Shane32
Copy link
Member

Shane32 commented Nov 8, 2022

Visual Studio 17.4 hasn't been released yet. Probably it will be within a day or so and then we can merge this.

@sungam3r
Copy link
Member

sungam3r commented Nov 9, 2022

I have run VS installer just now - 2.4 Gb update for 17.4 :)

@sungam3r sungam3r merged commit 0ded053 into graphql-dotnet:master Nov 9, 2022
@sungam3r
Copy link
Member

sungam3r commented Nov 9, 2022

Successful build on my machine except broken localization in console
изображение

@zcsizmadia zcsizmadia deleted the upgrade-to-net7 branch November 9, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI configuration issue or pull request performance Issue or pull request about performance problems test Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants