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.

Sign up for GitHub

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

Jump to bottom

Add net8.0 target framework #4502

Merged
merged 4 commits into from
Oct 2, 2023
Merged

Add net8.0 target framework #4502

merged 4 commits into from
Oct 2, 2023

Conversation

cristipufu
Copy link
Contributor

@cristipufu cristipufu commented Aug 30, 2023 •
edited
Loading

// Will update as soon as .net 8 gets a stable release

// Need to update Nuke.Common to support net8 nuke-build/nuke#1247

@cristipufu cristipufu mentioned this pull request Aug 30, 2023
Closed
@RicoSuter
Copy link
Owner

Would be great if you could update it against latest master

@RicoSuter
Copy link
Owner

I'd say the build project can stay on .net 7 for now, just the needed libraries etc need to be updated for now.

@cristipufu
Copy link
Contributor Author

@RicoSuter @lahma done

@lahma
Copy link
Collaborator

lahma commented Sep 29, 2023

nit: if you look at the GitHub diff, you can see that there are multiple odd indents present, maybe tabs vs spaces or something?

@cristipufu
Copy link
Contributor Author

@RicoSuter @lahma can you please approve the latest workflow?

@cristipufu
Copy link
Contributor Author

@lahma
I don't think that the net8-rc1 sdk is installed in the base image, do I need to add these back or we just wait for the final release?

build.yaml/pr.yaml
- uses: actions/setup-dotnet@v3
        with:
          dotnet-version: 8.0.100-rc.1.23463.5

Build.CI.GitHubActions.cs
// only need to list the ones that are missing from default image
newSteps.Insert(0, new GitHubActionsSetupDotNetStep(new[] 
{
    "8.0.100-rc.1.23463.5"
}));

Any other suggestions?

@lahma
Copy link
Collaborator

lahma commented Sep 30, 2023

IMO it's a good idea to add them, NUKE setup was cleaned earlier to not include obsolete ones bur for NET 8 is a different story.

Getting builds out against RC would help to validate and find possible problems.

@cristipufu
Copy link
Contributor Author

@lahma done, we can give it another try

lahma
lahma approved these changes Sep 30, 2023
View reviewed changes
Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

LGTM

@lahma
Copy link
Collaborator

lahma commented Oct 1, 2023 •
edited
Loading

I found one place that might still need alteration:

NSwag/src/NSwag.Commands/Commands/Generation/AspNetCore/AspNetCoreToOpenApiCommand.cs

Lines 146 to 148 in c24ed12

if (projectMetadata.TargetFrameworkIdentifier == ".NETCoreApp" ||
projectMetadata.TargetFrameworkIdentifier == "net6.0" ||
projectMetadata.TargetFrameworkIdentifier == "net7.0")

Because it's under pre-processor directive which should only hit modern .NET I wonder whether the if is needed at all.

@RicoSuter
Copy link
Owner

I think we can remove this if completely

RicoSuter
RicoSuter reviewed Oct 2, 2023
View reviewed changes
src/NSwag.AspNetCore/Middlewares/RedirectToIndexMiddleware.cs
@@ -40,7 +40,7 @@ public async Task Invoke(HttpContext context)

var suffix = !string.IsNullOrWhiteSpace(_swaggerRoute) ? "?url=" + _transformToExternal(_swaggerRoute, context.Request) : "";
var path = _transformToExternal(_swaggerUiRoute, context.Request);
context.Response.Headers.Add("Location", (path != "/" ? path : "") + "/index.html" + suffix);
context.Response.Headers.Append("Location", (path != "/" ? path : "") + "/index.html" + suffix);
Copy link
Owner

Choose a reason for hiding this comment

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

Did you have a problem with the Add()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VS preview complains with https://learn.microsoft.com/en-us/aspnet/core/diagnostics/asp0019?view=aspnetcore-8.0

@RicoSuter
Copy link
Owner

I think we can remove this if completely

I think the whole reasoning behind the two ifs (.netframework, .netcore) is that you get a "nice" error when you try to load a .netfw project in .net core and vice versa, which you would not get anymore (probably it fails differently now :))

@RicoSuter
Copy link
Owner

RicoSuter commented Oct 2, 2023 •
edited
Loading

... updated with my proposal (keep ifs to be more safe but make it more generic in .net "core" case so we do not have to touch it again) - it's anway just a "small" safe guard.

@Numpsy Numpsy mentioned this pull request Oct 2, 2023
Open
@RicoSuter RicoSuter merged commit b32ccee into RicoSuter:master Oct 2, 2023
1 check passed
lahma pushed a commit to lahma/NSwag that referenced this pull request Jan 20, 2024
@cristipufu @RicoSuter @lahma
Add net8.0 target framework (RicoSuter#4502)
55e1acf 
* Add net8.0 target framework

* Install net8 sdk missing from base image

* Remove unnecessary TargetFramework conditions

* Update AspNetCoreToOpenApiCommand.cs

---------

Co-authored-by: Rico Suter <mail@rsuter.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RicoSuter RicoSuter RicoSuter left review comments

@lahma lahma lahma approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cristipufu @RicoSuter @lahma