Add support for .NET Native AOT #10978

JamesNK · 2022-11-14T14:24:20Z

Support reflection accessors
Support extensions
Integrate Native AOT app into unit tests EDIT: Should these run automatically? How? From CI script or as part of unit test publishing and launching app?

Previously added trim attributes enables most of Google.Protobuf to work with Native AOT. There are some problematic MakeGenericType usages around reflection and extensions that need work.

jskeet

Code seems fine - my main concern is around requiring .NET 7 and C# 11.

csharp/src/Google.Protobuf.Test.NativeAot/Assert.cs

csharp/src/Directory.Build.props

csharp/src/Google.Protobuf.Test.NativeAot/Reflection/Descriptors.cs

csharp/src/Google.Protobuf.Test.NativeAot/Program.cs

jskeet · 2022-11-16T08:34:26Z

csharp/src/Google.Protobuf.sln

-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google.Protobuf.Test.TestProtos", "Google.Protobuf.Test.TestProtos\Google.Protobuf.Test.TestProtos.csproj", "{ADF24BEB-A318-4530-8448-356B72B820EA}"
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Google.Protobuf.Test.TestProtos", "Google.Protobuf.Test.TestProtos\Google.Protobuf.Test.TestProtos.csproj", "{ADF24BEB-A318-4530-8448-356B72B820EA}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Google.Protobuf.Test.NativeAot", "Google.Protobuf.Test.NativeAot\Google.Protobuf.Test.NativeAot.csproj", "{642232EA-FA89-4E84-9CA0-423EA98F37ED}"


Depending on the outcome of discussions around .NET 7, we may not want to include this in the solution.

csharp/src/Google.Protobuf/Google.Protobuf.csproj

csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs

jskeet · 2022-11-16T08:39:51Z

global.json

@@ -1,6 +1,6 @@
 {
  "sdk": {
-    "version": "6.0.100",
+    "version": "7.0.100",


If we go ahead with my suggestion of "you only need .NET 7 to run the AOT test project", I assume we'd revert this but add another global.json in the AOT test project directory. Does that sound feasible?

JamesNK · 2022-11-17T01:29:56Z

PR feedback is done.

Is there a problem with updating to the .NET 7 SDK? A few years ago, the SDK was .NET 5 to support some new features. There wasn't any issue.

jskeet · 2022-11-17T07:34:52Z

A few years ago, the SDK was .NET 5 to support some new features. There wasn't any issue.

I'm pretty sure I didn't review that PR, or I might have been concerned :)

The problem is that it forces anyone working on the runtime to install a non-LTS version of the SDK. My gut feeling is that a lot of companies and individuals will only want to install LTS versions of the SDK (and depend on LTS versions of .NET). Not all, certainly - but enough to make this at least something to consider.

Within Google, it definitely makes things more difficult if we need C# 11 support. I'm not going to reveal internal details, but keeping to just LTS SDK versions would reduce maintenance.

This isn't a complete blocker - I just want us to think about the feasibility of sticking to LTS SDK versions.

(I'll do a more complete review of the PR later; I haven't really started my working day yet.)

jskeet

One nit, but other than that I think we're good to go once we've decided on the "make everyone install .NET 7" vs "only need to install .NET 7 to run the AOT tests" part.

Although thinking about it more, presumably we have to be using the .NET 7 SDK in order to the get benefits of the EnableAOTAnalyzer property, which is in Google.Protobuf. Hmm. I'll need to think about that more - and check with the Protobuf team about their release system.

I'll ping the team an email today, but I'm on vacation tomorrow, so it'll probably be Monday before we can get to a decision. If this is blocking and urgent, let me know and I'll see what I can do.

jskeet · 2022-11-17T10:49:08Z

csharp/src/Google.Protobuf.Test.NativeAot/Program.cs

+{
+    // This project tests specific Google.Protobuf features with .NET Native AOT.
+    // It is designed to be published as a native app, and then run.
+    // Return value of 100 is success, 101 is failure.


Any reason for returning 100 vs 101, instead of 0 vs 1? The latter would be simpler to handle in shell scripts etc, where 0 is normally "success" and anything else is failure.

(We could potentially have an Action[] and test each in turn, catching exceptions, and returning the number of failures.)

Sorry for not spotting this before.

I followed some other tests which used 100 and 101. I've changed it to 0 and 1.

Using Action[] would be nice, but something to consider with AOT and trimming is running things in isolation. I think the ideal approach is adding #ifdef and then building and running the app for each test, passing in the test name as a compiler constant. I added that to the comment.

jskeet · 2022-11-21T15:28:40Z

@JamesNK: I've spoken to the team, and we think it's okay to require the .NET 7 SDK for building. That resolves a bunch of my comments, basically :) We need to make sure we stick with C# 10 as the language version though - so thanks for amending that already.

I'm just checking whether we need to make our release pipeline changes before merging this PR, or whether we can merge first. Are you basically happy for this to be merged when everything else is good to go, or are there any other changes you want to make?

jskeet · 2022-11-22T08:11:01Z

Status update for this: @deannagarcia has been investigating using the .NET 7 SDK, and we're currently blocked on NuGet/Home#12227 - while workarounds are possible, I think it's better to wait for that to be resolved. Once we've got a working .NET 7 build, we'll be able to merge this. (And indeed this PR can become slightly smaller :)

jskeet · 2023-01-17T15:22:00Z

@JamesNK: Could you rebase this onto the current head - or are you happy for me to do so? I believe everything should be in place now for this to be merged in the new flow, but I suspect it's more likely to go through if it's synced against a recent commit.

JamesNK · 2023-01-17T22:58:59Z

This one is still blocked I think. I rebased #11128

jskeet · 2023-01-17T23:06:35Z

It'll need internal review - it's a matter of making sure it gets that far. We'll get there - thanks for the rebase.

@jskeet

The fixes from #10978. This PR can be merged without updating the SDK. Will allow people to use Protobuf + AOT sooner rather than later. When the repo is updated to .NET 7 or .NET 8, the original PR can be rebased on latest to add AOT analysis and provide some AOT smoke tests. cc @jskeet Closes #11128 COPYBARA_INTEGRATE_REVIEW=#11128 from JamesNK:jamesnk/enable-aot-analysis-2 51ed1bd PiperOrigin-RevId: 503077675

JamesNK · 2023-01-31T09:52:28Z

Rebased on main. This PR now just updates the solution to .NET 7 and adds tests.

No urgency to merge this.

JamesNK · 2023-05-24T01:08:42Z

Blocked on repo using .NET 7 SDK. Will reopen when SDK is successfully updated.

JamesNK marked this pull request as ready for review November 16, 2022 01:30

JamesNK requested a review from a team as a code owner November 16, 2022 01:30

JamesNK requested review from jskeet and removed request for a team November 16, 2022 01:30

jskeet reviewed Nov 16, 2022

View reviewed changes

jskeet mentioned this pull request Nov 16, 2022

Google.Protobuf has errors when compiled with C# 11 #11004

Closed

jskeet reviewed Nov 17, 2022

View reviewed changes

deannagarcia mentioned this pull request Nov 21, 2022

Update to .NET 7 SDK #11035

Closed

acizmarik mentioned this pull request Nov 21, 2022

Added managed implementation for profiler acizmarik/sharpdetect#27

Merged

18 tasks

googleberg added c# kokoro:run labels Nov 24, 2022

protobuf-kokoro removed the kokoro:run label Nov 24, 2022

JamesNK mentioned this pull request Dec 2, 2022

Fix .NET Native AOT warnings in Protobuf reflection #11128

Closed

JamesNK force-pushed the jamesnk/enable-aot-analysis branch from 4711ae1 to d5559e6 Compare January 17, 2023 23:00

Add support for .NET Native AOT

6ba4e84

JamesNK force-pushed the jamesnk/enable-aot-analysis branch from d5559e6 to 6ba4e84 Compare January 31, 2023 09:50

zhangskz removed the release notes: yes label Feb 23, 2023

JamesNK closed this May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for .NET Native AOT #10978

Add support for .NET Native AOT #10978

JamesNK commented Nov 14, 2022 •

edited

Loading

jskeet left a comment

jskeet Nov 16, 2022

jskeet Nov 16, 2022

JamesNK commented Nov 17, 2022

jskeet commented Nov 17, 2022

jskeet left a comment

jskeet Nov 17, 2022

JamesNK Nov 18, 2022

jskeet commented Nov 21, 2022

jskeet commented Nov 22, 2022

jskeet commented Jan 17, 2023

JamesNK commented Jan 17, 2023 •

edited

Loading

jskeet commented Jan 17, 2023

JamesNK commented Jan 31, 2023 •

edited

Loading

JamesNK commented May 24, 2023

Add support for .NET Native AOT #10978

Add support for .NET Native AOT #10978

Conversation

JamesNK commented Nov 14, 2022 • edited Loading

jskeet left a comment

Choose a reason for hiding this comment

jskeet Nov 16, 2022

Choose a reason for hiding this comment

jskeet Nov 16, 2022

Choose a reason for hiding this comment

JamesNK commented Nov 17, 2022

jskeet commented Nov 17, 2022

jskeet left a comment

Choose a reason for hiding this comment

jskeet Nov 17, 2022

Choose a reason for hiding this comment

JamesNK Nov 18, 2022

Choose a reason for hiding this comment

jskeet commented Nov 21, 2022

jskeet commented Nov 22, 2022

jskeet commented Jan 17, 2023

JamesNK commented Jan 17, 2023 • edited Loading

jskeet commented Jan 17, 2023

JamesNK commented Jan 31, 2023 • edited Loading

JamesNK commented May 24, 2023

JamesNK commented Nov 14, 2022 •

edited

Loading

JamesNK commented Jan 17, 2023 •

edited

Loading

JamesNK commented Jan 31, 2023 •

edited

Loading