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 .NET Core 9.0 #718

Merged
merged 8 commits into from
Nov 22, 2024
Merged

Update .NET Core 9.0 #718

merged 8 commits into from
Nov 22, 2024

Conversation

hishamco
Copy link
Collaborator

No description provided.

@hishamco
Copy link
Collaborator Author

@sebastienros did I miss something here?

@@ -17,6 +17,9 @@ jobs:

steps:
- uses: actions/checkout@v4
- uses: actions/setup-dotnet@v4
with:
global-json-file: global.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

didn't know of this switch, cool

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Me too 😄

@@ -1,39 +1,43 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing spaces to tabs doesn't feel right

@@ -1,53 +1,68 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
Copy link
Collaborator

Choose a reason for hiding this comment

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

why change to tabs?

@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>net6.0;net8.0</TargetFrameworks>
Copy link
Collaborator

Choose a reason for hiding this comment

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

should probably drop net6.0 all around

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I format the file in VS it uses tabs over spaces, regular tab is 4 spaces

@sebastienros
Copy link
Owner

sebastienros commented Nov 22, 2024

My thoughts about this PR.

I wouldn't add the net9.0 tfm if there were not specific framework references to ASP.NET (for the view engines) (i.e. this is fine in this project), since there are net9.0 specific code paths otherwise.
Even if a dependency had a specific net9.0 target we would not need to (for instance if parlot has net9.0 tfm specifically)

I added the global.json such that users understand what minimum SDK version is required to build. I added action/setup-dotnet because the linux runners in GH don't have it yet (The windows ones have it already). I think we can keep it forever and drive the version from global.json.

I removed net7.0 because it's out of support for 6 months, and there is no reason to keep it otherwise (not an LTS even). I'd keep net6.0 for now since it just got unsupported a few days ago.

I am still trying to come up with a rationale about when to use a dotnet library version 9.0 if not required (example: extensions, abstractions, ...). MS is also working on this internally for the different types of libraries it ships.

@sebastienros
Copy link
Owner

@hishamco all these space changes should be reverted. Is there a way to enforce tooling follows some rules with the .editorsettings? This way VS (if this is what did the changes in this case) would follow it.

@lahma
Copy link
Collaborator

lahma commented Nov 22, 2024

I think NET 9 SDK is a good addition, but I also believe that NET 9 target is unnecessary. SDKs ship with new clever ways to transform your code to more performant structures (when you use collection expression and friends for example).

@sebastienros
Copy link
Owner

but I also believe that NET 9 target is unnecessary

Have you seen the framework references in the csproj files? But for Fluid.csproj I agree this is not necessary.

@lahma
Copy link
Collaborator

lahma commented Nov 22, 2024

but I also believe that NET 9 target is unnecessary

Have you seen the framework references in the csproj files? But for Fluid.csproj I agree this is not necessary.

Yes for the consuming projects net9.0 makes sense, just for publishing I see little value.

@sebastienros
Copy link
Owner

MvcViewEngines is publishing, and has a reference to the aspnet runtime. I am afraid it would pull net8.0 aspnetcore if you used a net9.0 app. I can test it.

@sebastienros
Copy link
Owner

I am still applying some changes, don't merge

@sebastienros
Copy link
Owner

I finally introduced a variable version for FileProvider.Abstractions as I am not clear yet if it pull ups some other dependencies, and also is referencing 9.0 only triggers warning for net6.0 builds AND it could trigger downgrade warnings for other projects pointing to a different version than they use.

@sebastienros sebastienros merged commit 0e8a243 into main Nov 22, 2024
1 check passed
@sebastienros sebastienros deleted the hishamco/net-9 branch November 22, 2024 21:31
@lahma
Copy link
Collaborator

lahma commented Nov 24, 2024

The latest release seems to cause some trouble.. https://www.nuget.org/packages/Fluid.Core lists

image

Which might the cause for some problems with NJsonSchema upgrade:

https://github.com/RicoSuter/NJsonSchema/actions/runs/11999376115/job/33447213638?pr=1758

20:34:37 [DBG] System.TypeInitializationException : The type initializer for 'LiquidTemplate' threw an exception.
20:34:37 [DBG] ---- System.IO.FileNotFoundException : Could not load file or assembly 'System.Text.Encodings.Web, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The system cannot find the file specified.

@sebastienros
Copy link
Owner

This was an intentional change which I thought would be fine ... going to rever this. The issue is with MEFA, right ?

@sebastienros
Copy link
Owner

There was a conflict between "System.Text.Encodings.Web, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" and "System.Text.Encodings.Web, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51".

That's the issue with bumping to 9.0.0, it's compatible with netstandard2.0, and there is no reason we shouldn't upgrade, but then other libraries might pull a different version directly and that could be an issue. Is NJsonSchema referencing System.Text.Encodings.Web, Version=8.0.0.0 directly?

@sebastienros
Copy link
Owner

      └─ NJsonSchema (v11.0.0)
         └─ System.Text.Json (v8.0.5)
            └─ System.Text.Encodings.Web (v8.0.0)

@sebastienros
Copy link
Owner

And it's only for net462, netstandard2.0, net472 since they have to reference STJ.

@sebastienros
Copy link
Owner

FYI I used dotnet nuget why .\NJsonSchema.sln System.Text.Encodings.Web in case you were not aware of the command

@lahma
Copy link
Collaborator

lahma commented Nov 24, 2024

I think with general netstandard it would be best to require lowest possible. A weird problem to see when latest SDK is being used and it should automatically resolve dependencies and version forwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants