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 to .NET 8 + C# 12 #26957

Merged
merged 15 commits into from
Feb 5, 2024
Merged

Update to .NET 8 + C# 12 #26957

merged 15 commits into from
Feb 5, 2024

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Feb 2, 2024

@pull-request-size pull-request-size bot added size/L and removed size/XL labels Feb 2, 2024
Templates/Rulesets/ruleset-empty/app.manifest Outdated Show resolved Hide resolved
@@ -153,12 +153,12 @@ private struct ByHandleFileInformation
public static extern int link(string oldpath, string newpath);

[DllImport("libc", SetLastError = true)]
private static extern int stat(string pathname, out struct_stat statbuf);
private static extern int stat(string pathname, out Stat statbuf);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For anyone else, explanation is that this triggers https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/warning-waves#cs8981---the-type-name-only-contains-lower-cased-ascii-characters.

Not sure how to feel about this change but don't think it's worth losing sleep over either.

@Joehuu
Copy link
Member

Joehuu commented Feb 2, 2024

.NET 8 ends support for Windows 8.1. Are we ending support for that? Maybe have some sort of tooling that can build .NET 6 for Windows 8.1 only and .NET 8 for the rest, but requires upkeep I would think and is probably not worth it as only 0.13% of steam users are still using it.

Prereqs aren't updated in the readme.

@smoogipoo
Copy link
Contributor Author

It looks like 8.1 is EOL so I would go ahead with dropping it.

image

@smoogipoo
Copy link
Contributor Author

smoogipoo commented Feb 2, 2024

I've updated the system requirements in the readme. More concerning to me is that macOS 10.15 isn't supported anymore, which was the last x86-64 build of macOS.

Curious what this means for that system and if it'll run at all anymore. If not... we may have to reconsider.

@Joehuu
Copy link
Member

Joehuu commented Feb 2, 2024

Actually no. Their definition of supported means not-EOL operating systems, see dotnet/core#9038 (comment). Windows 8.1 and others could still work but may break whenever.

Edit: still seems to run on Windows 8.1

@Susko3
Copy link
Member

Susko3 commented Feb 2, 2024

Android needs attention (see action logs):

The TargetFrameworkVersion (Android API level 34) is higher than the targetSdkVersion (33). Please increase the android:targetSdkVersion in the AndroidManifest.xml so that the API levels match.

@peppy peppy self-requested a review February 3, 2024 14:23
"version": "6.0.100",
"rollForward": "latestFeature"
"version": "8.0.0",
"rollForward": "latestFeature",
Copy link
Contributor

@huoyaoyuan huoyaoyuan Feb 4, 2024

Choose a reason for hiding this comment

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

I did have something to discuss around this.
If rollForward is set to major, it would allow higher SDK to be loaded, but lower version will still be preferred if present.

I know that Android/iOS workloads are more sensitive to SDK upgrades. I've faced similar problems with CsWinRT that patch updates of SDK broke things.
We may choose to pin Android/iOS workload verions instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't intending to change anything here, just to update the version.

@huoyaoyuan
Copy link
Contributor

Actually no. Their definition of supported means not-EOL operating systems, see dotnet/core#9038 (comment). Windows 8.1 and others could still work but may break whenever.

Edit: still seems to run on Windows 8.1

As far as I can tell, coreclr hasn't been really removing code for Windows 7. Simple programs will probably just work, but issues won't be fixed.

The area needs most attention should be cryptographic. Old algorithms/APIs are going to be deprecated these years.

I've updated the system requirements in the readme. More concerning to me is that macOS 10.15 isn't supported anymore, which was the last x86-64 build of macOS.

According to wikipedia and Apple support, macOS Monterey still supports x86-64 Mac, with some features unavailable.

global.json Outdated
@@ -1,7 +1,7 @@
{
"sdk": {
"version": "6.0.100",
"rollForward": "latestFeature"
"version": "8.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

The SDK version should be 8.0.100.

Copy link
Contributor Author

@smoogipoo smoogipoo Feb 5, 2024

Choose a reason for hiding this comment

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

Does this change anything given it's latestFeature? This was changed automatically via Rider's SDK selection window.

Copy link
Contributor

Choose a reason for hiding this comment

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

latestFeature should mean that all 8.0.xxx versions are valid.
The concern is that 8.0.0 is not valid SDK version number format. I've not found the global.json parsing code, but maybe something is depending on the format.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Seems good to go from my end (android tested to work)

@peppy peppy merged commit cc5e1e0 into ppy:master Feb 5, 2024
15 of 17 checks passed
@peppy peppy self-requested a review February 5, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants