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

feat(Solution): Add NET8 support (In an addition to existing NET9) #122

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RainDance74
Copy link

This pull request aims to support both currently supported by Microsoft .NET versions.

Tests on the main and current branches don't look stable to me, so I had about 7-11 tests failed locally (Win11).
Hope it will help to add support of .NET 8 for https://github.com/neuroglia-io/asyncapi.

If I did something wrong, please leave a comment, I will be happy to fix it if necessary.

In an addition to existing NET9
@RainDance74
Copy link
Author

Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

Looks awesome, many thanks for your awesome work!

@cdavernas
Copy link
Member

For the tests, some are indeed unstable, but should be skipped. The others should pass as expected. I'll test locally to see what's what.

On a side note, Blazor Dagre doesn't build anymore with those changes.

@RainDance74
Copy link
Author

On a side note, Blazor Dagre doesn't build anymore with those changes.

Alright, gonna check it out

@cdavernas
Copy link
Member

Another side note, it might be easier if I just edit the culprit project, meaning the Serialization.Xml one, port all its references to 8.0 and publish the resulting package to align with previous versions.

@cdavernas
Copy link
Member

Not sure if it's will work without any problems: https://github.com/RainDance74/framework/blob/feature-dotnet-8/src/Neuroglia.Plugins/Services/NugetPackagePluginSource.cs#L160

I dont see why not. AFAIK, it should pickup the currently running version of the framework, no?

@RainDance74
Copy link
Author

Yes, but in this case it will be necessary to have some specific version of AsyncAPI package required for NET8, or it will be updated somehow?

@RainDance74
Copy link
Author

I dont see why not. AFAIK, it should pickup the currently running version of the framework, no?

It look like, just clarifying

@cdavernas
Copy link
Member

Yes, but in this case it will be necessary to have some specific version of AsyncAPI package required for NET8, or it will be updated somehow?

Indeed, there would be the need to create a specific version for it. Im aware it might not be optimal

@RainDance74
Copy link
Author

On a side note, Blazor Dagre doesn't build anymore with those changes.

Fixed

@RainDance74
Copy link
Author

Im aware it might not be optimal

Well, as for other packages like Microsoft.Extensions.DependencyInjection, they use multiple target versions, just like here in the PR, also there might be a need to use #if statements for different versions of code.

@RainDance74
Copy link
Author

RainDance74 commented Jan 28, 2025

Seems there is an error in CI because no target framework installed, I guess I need some help on that because haven't really worked with github actions

UPD: Fixed, but got a test failed, is it because they aren't stable?
https://github.com/RainDance74/framework/actions/runs/13030743247/job/36349565480

Didn't found a way to do it with the matrix. Used this approach: actions/setup-dotnet#240
@cdavernas
Copy link
Member

@RainDance74 Thank you for your awesome work! I'll have a look asap, but I'm presenting at FOSDEM this weekend and have a lot on my plate, so it might take a little while 😉

@RainDance74
Copy link
Author

RainDance74 commented Jan 30, 2025

I'm presenting at FOSDEM this weekend and have a lot on my plate, so it might take a little while 😉

Alright, then you could merge it when you ready, closing WIP
Good luck with FOSDEM networking!

@RainDance74 RainDance74 marked this pull request as ready for review January 30, 2025 08:00
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.

2 participants