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

Add .NET 7, 8, trimming and AOT support #273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

giggio
Copy link

@giggio giggio commented Nov 11, 2023

Also fixed several nullability warnings.
Closes #272.

@giggio giggio force-pushed the gb/aot branch 2 times, most recently from aa64b33 to 77752ab Compare November 11, 2023 02:37
@jskeet
Copy link
Contributor

jskeet commented Nov 11, 2023

I'll have a look when I can, but I don't know when that will be - quite possibly not for a few weeks due to other current work.

@jskeet
Copy link
Contributor

jskeet commented Nov 13, 2023

One initial comment before I get chance to review properly - I would strongly prefer not to target .NET 7.0, given that it will go out of support in H1 2024.

It's also not clear to me why we need to explicitly target .NET 8.0, given that a .NET 6.0 target will work for .NET 8.0 - could you comment on that?

Finally, it looks like quite a lot of changes are just whitespace - please revert these. (I may add an editorconfig at some point, but until then I really don't want changes that are just removing a space in a cast expression, for example.)

@giggio
Copy link
Author

giggio commented Nov 13, 2023

@jskeet answers bellow:

One initial comment before I get chance to review properly - I would strongly prefer not to target .NET 7.0, given that it will go out of support in H1 2024.

That's fine and easy to change, but don't you think it could be a path forward for people who are migrating from .NET 6? We could easily remove it when it goes out of support.

It's also not clear to me why we need to explicitly target .NET 8.0, given that a .NET 6.0 target will work for .NET 8.0 - could you comment on that?

AOT only became available at .NET 7+, and with very limited support.

Also, diagnostics. The IsAotCompatible msbuild property only works for target framework .NET 7+. See:

https://github.com/giggio/cloudevents-sdk-csharp/blob/77752ab070f31059a49ad1b78ec8b489883803c2/src/Directory.Build.props#L33

We need the .NET 7 or .NET 8 sdk to be able to use the -p:PublishAot flag. See docs.

But I could add .NET 6. Do you think that would be useful? It wouldn't work with AOT, though.

Finally, it looks like quite a lot of changes are just whitespace - please revert these. (I may add an editorconfig at some point, but until then I really don't want changes that are just removing a space in a cast expression, for example.)

Sure, I'll update the PR and remove those.

@jskeet
Copy link
Contributor

jskeet commented Nov 13, 2023

(Removing .NET 7.)

That's fine and easy to change, but don't you think it could be a path forward for people who are migrating from .NET 6? We could easily remove it when it goes out of support.

Except that's a breaking change, less than 6 months after adding it... it doesn't fell like it's worth it to me. People migrating from .NET 6 should go straight to .NET 8, which is due to be released tomorrow. I see no reason anyone would adopt .NET 7 now.

Happy to accept the benefit of adding .NET 8 for AOT publishing, if we definitely need that... but is a .NET 8 target definitely needed for libraries that the app depends on? I'll need to read the docs more carefully. (That may well be the case, and would be a reason to add the target. But I only want to do that if there really is a benefit.)

Will look in more detail when I get more time.

Also fixed several nullability warnings.

Signed-off-by: Giovanni Bassi <[email protected]>
@giggio
Copy link
Author

giggio commented Nov 13, 2023

Ok, if you want me to remove .NET 7 it's a simple change, just let me know.

I believe I have removed all the whitespace, it was a lot of work, it would be a good idea to add an .editorconfig file and do I big commit that fixes all the code that is not conformant.

Regarding the importance of AOT, I see it is a big thing for a project like this. CloudEvents are often used in function apps, which tend to start, do something and stop (if not immediately, some not long time after). They start and stop often, so startup time is important and AOT brings that.

@jskeet
Copy link
Contributor

jskeet commented Nov 13, 2023

Regarding the importance of AOT, I see it is a big thing for a project like this. CloudEvents are often used in function apps, which tend to start, do something and stop (if not immediately, some not long time after). They start and stop often, so startup time is important and AOT brings that.

I didn't question the importance of AOT. I was questioning whether .NET 8 was a pre-requisite as a target framework for dependencies rather than just using the .NET 8 SDK and/or the application targeting .NET 8.

@giggio
Copy link
Author

giggio commented Nov 13, 2023

I didn't question the importance of AOT. I was questioning whether .NET 8 was a pre-requisite as a target framework for dependencies rather than just using the .NET 8 SDK and/or the application targeting .NET 8.

Ok, got it.
We won't get the diagnostics unless we target .NET 8, see:

<IsAotCompatible Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0'))">true</IsAotCompatible>

This was added by the tooling. See docs.
It is interesting, because the tooling added it requiring .NET 7+, but the docs mention that the analyzers are not available for .NET 7, but only for .NET 8. But I'm not getting any warnings or errors this way. Maybe they backported the analyzers and didn't update the docs.

@jskeet
Copy link
Contributor

jskeet commented Nov 14, 2023

Looking at the docs, it's still not clear to me whether we really need to target .NET 8 to use AOT. I suspect the "this needs .NET 8" means "you need to be using the .NET 8 SDK" rather than "every library you depend on needs to target .NET 8". I'm going to get in touch with a friend at MS who should be able to help more on this.

@jskeet
Copy link
Contributor

jskeet commented Nov 14, 2023

Having looked at this briefly, there's definitely more here than I'd want in one commit anyway - and I won't be able to review it in one big chunk.

What I propose is that we leave this PR here, but I basically recreate it (or most of it) one step at a time. I'll start with an editorconfig and get everything to conform to that, then fix NRT issues, then move on to AOT. We can think about the minimal API sample separately.

(To give a little context, I'm on a project which doesn't give me much headspace for anything big at the moment, but does let me work on bite-sized chunks. Doing this in multiple PRs is going to end up being way quicker than waiting for me to have the time to review everything in one go.)

@giggio
Copy link
Author

giggio commented Nov 14, 2023

Sure. I can help. I'll wait for the editorconfig, when that is done let me know and we can collaborate on that.

@jskeet
Copy link
Contributor

jskeet commented Nov 14, 2023

Editorconfig PRs are merged. When the .NET 8.0 SDK (non-RC) is out - hopefully later today - I'll install that and get on the non-nullable side. My approaches may not always match yours, mind you - but I'll try to make sure I cover everything in your PR.

@giggio
Copy link
Author

giggio commented Dec 12, 2023

@jskeet I see you have progressed on it, do you want some help to move this forward?

@jskeet
Copy link
Contributor

jskeet commented Dec 12, 2023

I don't have the time to do much else before January now, but if you can revise your PR to reduce it to "just the AOT stuff" that would help me check my next step

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.

Make library AOT compatible
2 participants