Generate Protobuf C# objects automatically in MSBuild

[yanpitangui]

Fixes #5362

Changes

• Removed manually generated files with the msbuild alternative (https://chromium.googlesource.com/external/github.com/grpc/grpc/+/HEAD/src/csharp/BUILD-INTEGRATION.md)

• For this to work, we need to figure out a way to override the Equals and GetHashCode from NodeMetrics and Metrics in Akka.Cluster.Metrics, AFAIK that is not possible right now, so I kept the build not functioning yet by not removing the manually created Equals and GetHashCode

• One thing that we should look at is the accessibility of the generated classes, right now everything is public. Changing everything to internal also break things, so I would like some guidance.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

• [x] This change follows the Akka.NET API Compatibility Guidelines.
• [x] This change follows the Akka.NET Wire Compatibility Guidelines.
• [x] I have reviewed my own pull request.
• [x] Changes in public API reviewed, if any.

[Aaronontheweb]

Thanks for this @yanpitangui - I'll take a look at this

[Aaronontheweb]

One thing that we should look at is the accessibility of the generated classes, right now everything is public. Changing everything to internal also break things, so I would like some guidance.

I'm fine leaving them as public - these classes are subjected to Extend-only Design inherently through the design of the Protobuf versioning guidelines. And besides, these types map directly to wire formats so they have to be treated like they're public anyway due to our wire versioning guidelines too: https://getakka.net/community/contributing/wire-compatibility.html

[Aaronontheweb]

D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(685,17): error CS0111: Type 'NodeMetrics' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(700,25): error CS0111: Type 'NodeMetrics' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(1478,21): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(1493,29): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(685,17): error CS0111: Type 'NodeMetrics' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(700,25): error CS0111: Type 'NodeMetrics' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(1478,21): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(1493,29): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
    0 Warning(s)
    8 Error(s)

I think what you have going on here is actually a namespace clash @yanpitangui - these files should normally be copied into the .Proto sub-namespace for each of their respective projects. That's what the FAKE script was doing previously:

let protoFiles = [
        ("WireFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("ContainerFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("SystemMessageFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("ClusterMessages.proto", "/src/core/Akka.Cluster/Serialization/Proto/");
        ("ClusterClientMessages.proto", "/src/contrib/cluster/Akka.Cluster.Tools/Client/Serialization/Proto/");
        ("DistributedPubSubMessages.proto", "/src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Serialization/Proto/");
        ("ClusterShardingMessages.proto", "/src/contrib/cluster/Akka.Cluster.Sharding/Serialization/Proto/");
        ("ReliableDelivery.proto", "/src/core/Akka.Cluster/Serialization/Proto/");
        ("TestConductorProtocol.proto", "/src/core/Akka.Remote.TestKit/Proto/");
        ("Persistence.proto", "/src/core/Akka.Persistence/Serialization/Proto/");
        ("StreamRefMessages.proto", "/src/core/Akka.Streams/Serialization/Proto/");
        ("ReplicatorMessages.proto", "/src/contrib/cluster/Akka.DistributedData/Serialization/Proto/");
        ("ReplicatedDataMessages.proto", "/src/contrib/cluster/Akka.DistributedData/Serialization/Proto/"); ]

So in your case, I think you just need to change the output directory these files are generated into - ensure that they get generated into Serialization/Proto for each of these respective projects. Does that make sense?

[yanpitangui]

The structure of the project and the build changes look fine - I need to take a look at the build errors next. I am not sure why this is the case (per @yanpitangui 's original PR comment):

For this to work, we need to figure out a way to override the Equals and GetHashCode from NodeMetrics and Metrics in Akka.Cluster.Metrics, AFAIK that is not possible right now, so I kept the build not functioning yet by not removing the manually created Equals and GetHashCode

That's odd, because we never manually edited any of the generated files before. Is this a new change caused by the way Grpc.Tools is calling protoc?

I'm pretty sure that the generated files were being versioned in git. If you take a look into the deleted files, you will see them Also, take a look at this comment: https://github.com/yanpitangui/akka.net/blob/e4c1ee2637a222d1a64a9cf2a34ac1de1d5d105c/src/contrib/cluster/Akka.Cluster.Metrics/Serialization/Metric.cs#L167

[yanpitangui]

D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(685,17): error CS0111: Type 'NodeMetrics' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(700,25): error CS0111: Type 'NodeMetrics' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(1478,21): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(1493,29): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(685,17): error CS0111: Type 'NodeMetrics' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(700,25): error CS0111: Type 'NodeMetrics' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(1478,21): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(1493,29): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
    0 Warning(s)
    8 Error(s)

I think what you have going on here is actually a namespace clash @yanpitangui - these files should normally be copied into the .Proto sub-namespace for each of their respective projects. That's what the FAKE script was doing previously:

let protoFiles = [
        ("WireFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("ContainerFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("SystemMessageFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("ClusterMessages.proto", "/src/core/Akka.Cluster/Serialization/Proto/");
        ("ClusterClientMessages.proto", "/src/contrib/cluster/Akka.Cluster.Tools/Client/Serialization/Proto/");
        ("DistributedPubSubMessages.proto", "/src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Serialization/Proto/");
        ("ClusterShardingMessages.proto", "/src/contrib/cluster/Akka.Cluster.Sharding/Serialization/Proto/");
        ("ReliableDelivery.proto", "/src/core/Akka.Cluster/Serialization/Proto/");
        ("TestConductorProtocol.proto", "/src/core/Akka.Remote.TestKit/Proto/");
        ("Persistence.proto", "/src/core/Akka.Persistence/Serialization/Proto/");
        ("StreamRefMessages.proto", "/src/core/Akka.Streams/Serialization/Proto/");
        ("ReplicatorMessages.proto", "/src/contrib/cluster/Akka.DistributedData/Serialization/Proto/");
        ("ReplicatedDataMessages.proto", "/src/contrib/cluster/Akka.DistributedData/Serialization/Proto/"); ]

So in your case, I think you just need to change the output directory these files are generated into - ensure that they get generated into Serialization/Proto for each of these respective projects. Does that make sense?

I think my previous comment touches the point here: the generated files were being stripped of their GetHashCode and Equals and versioned. Since the classes are partial, they work fine and can be extended

[Aaronontheweb]

Ah, you're right those files were edited. TIL. Ok. give that a shot with the partial class approach.

[yanpitangui]

D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(685,17): error CS0111: Type 'NodeMetrics' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(700,25): error CS0111: Type 'NodeMetrics' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(1478,21): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\netstandard2.0\Serialization\Proto\ClusterMetricsMessages.cs(1493,29): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=netstandard2.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(685,17): error CS0111: Type 'NodeMetrics' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(700,25): error CS0111: Type 'NodeMetrics' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(1478,21): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'Equals' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\obj\Release\net6.0\Serialization\Proto\ClusterMetricsMessages.cs(1493,29): error CS0111: Type 'NodeMetrics.Types.Metric' already defines a member called 'GetHashCode' with the same parameter types [D:\a\1\s\src\contrib\cluster\Akka.Cluster.Metrics\Akka.Cluster.Metrics.csproj::TargetFramework=net6.0]
    0 Warning(s)
    8 Error(s)

I think what you have going on here is actually a namespace clash @yanpitangui - these files should normally be copied into the .Proto sub-namespace for each of their respective projects. That's what the FAKE script was doing previously:

let protoFiles = [
        ("WireFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("ContainerFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("SystemMessageFormats.proto", "/src/core/Akka.Remote/Serialization/Proto/");
        ("ClusterMessages.proto", "/src/core/Akka.Cluster/Serialization/Proto/");
        ("ClusterClientMessages.proto", "/src/contrib/cluster/Akka.Cluster.Tools/Client/Serialization/Proto/");
        ("DistributedPubSubMessages.proto", "/src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/Serialization/Proto/");
        ("ClusterShardingMessages.proto", "/src/contrib/cluster/Akka.Cluster.Sharding/Serialization/Proto/");
        ("ReliableDelivery.proto", "/src/core/Akka.Cluster/Serialization/Proto/");
        ("TestConductorProtocol.proto", "/src/core/Akka.Remote.TestKit/Proto/");
        ("Persistence.proto", "/src/core/Akka.Persistence/Serialization/Proto/");
        ("StreamRefMessages.proto", "/src/core/Akka.Streams/Serialization/Proto/");
        ("ReplicatorMessages.proto", "/src/contrib/cluster/Akka.DistributedData/Serialization/Proto/");
        ("ReplicatedDataMessages.proto", "/src/contrib/cluster/Akka.DistributedData/Serialization/Proto/"); ]

So in your case, I think you just need to change the output directory these files are generated into - ensure that they get generated into Serialization/Proto for each of these respective projects. Does that make sense?

About the FAKE script, if I'm not mistaken, the "Protobuf" step wasn't part of the build process. It was executed manually whenever a protobuf file changed or was created and then the correspondent file would be updated

[yanpitangui]

Ah, you're right those files were edited. TIL. Ok. give that a shot with the partial class approach.

Even though the classes are partial, as they already defined GetHashCode and Equals, I don't see a way to solve this

[Aaronontheweb]

Ah, you're right those files were edited. TIL. Ok. give that a shot with the partial class approach.

Even though the classes are partial, as they already defined GetHashCode and Equals, I don't see a way to solve this

Let me think on it - maybe we're doing something bad that we should not be, with those types.

[Aaronontheweb]

So I've found the issue here:

https://github.com/akkadotnet/akka.net/blob/0782c7dc069618e20a3791aaab667076f487ad96/src/contrib/cluster/Akka.Cluster.Metrics/Serialization/NodeMetrics.cs#L23-L110

Indeed, we are abusing the Protobuf objects inside our internal usages within Akka.Cluster.Metrics. That's a separate issue that I think will need to be solved in a different PR.

[Aaronontheweb]

We fixed the issue with Akka.Cluster.Mertrics - you should be good to move forward with this now @yanpitangui

[yanpitangui]

Thank you @Aaronontheweb and @Arkatufus, I will resume from here!

[yanpitangui]

@Aaronontheweb, now, adding the internal accessor to the protobuf definition in the csproj, everything works fine. If I remove it, i will have to add the new types to the core api specs. Is that desirable?

[Aaronontheweb]

@Aaronontheweb, now, adding the internal accessor to the protobuf definition in the csproj, everything works fine. If I remove it, i will have to add the new types to the core api specs. Is that desirable?

We should make the internal protobuf types all internal

[Aaronontheweb]

@Aaronontheweb, now, adding the internal accessor to the protobuf definition in the csproj, everything works fine. If I remove it, i will have to add the new types to the core api specs. Is that desirable?

We should make the internal protobuf types all internal

And if that "breaks" a public API - the protobuf data types were never intended to be public in the first place, so this falls under our definition of "Practical SemVer"

[yanpitangui]

Hmm... looks like this pr broke the tests? I will have to take a deeper look in what happened

[Aaronontheweb]

@yanpitangui nah these are some racy tests - your code did not trigger those. We're even working on fixing some of them right now over on https://github.com/akkadotnet/akka.net/pull/7068

[yanpitangui]

@yanpitangui nah these are some racy tests - your code did not trigger those. We're even working on fixing some of them right now over on #7068

Oh ok, I will mark it as ready to review, then

[Aaronontheweb]

I'm gonna get this merged in soon, fyi @yanpitangui

[yanpitangui]

I'm gonna get this merged in soon, fyi @yanpitangui

Thank you. Should I resolve the conflicts or are you doing it?

[Aaronontheweb]

If you can do it, please do 🫡Sent from my iPhoneOn Apr 19, 2024, at 1:03 PM, Yan Pitangui @.***> wrote:

I'm gonna get this merged in soon, fyi @yanpitangui

Thank you. Should I resolve the conflicts or are you doing it?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were assigned.Message ID: @.***>