Generate Protobuf C# objects automatically in MSBuild #7063
Conversation
|
Thanks for this @yanpitangui - I'll take a look at this |
There was a problem hiding this comment.
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?
| @@ -25,6 +25,7 @@ | |||
| <NewtonsoftJsonVersion>[13.0.1,)</NewtonsoftJsonVersion> | |||
| <NBenchVersion>2.0.1</NBenchVersion> | |||
| <ProtobufVersion>3.25.2</ProtobufVersion> | |||
| <GrpcToolsVersion>2.60.0</GrpcToolsVersion> | |||
| @@ -426,51 +426,6 @@ Target "PublishNuget" (fun _ -> | |||
| printfn "%s" exn.Message | |||
| ) | |||
|
|
|||
| //-------------------------------------------------------------------------------- | |||
I'm fine leaving them as |
I think what you have going on here is actually a namespace clash @yanpitangui - these files should normally be copied into the 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 |
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 |
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 |
|
Ah, you're right those files were edited. TIL. Ok. give that a shot with the |
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 |
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. |
|
So I've found the issue here: 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. |
|
We fixed the issue with Akka.Cluster.Mertrics - you should be good to move forward with this now @yanpitangui |
|
Thank you @Aaronontheweb and @Arkatufus, I will resume from here! |
|
@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 |
And if that "breaks" a public API - the protobuf data types were never intended to be |
|
Hmm... looks like this pr broke the tests? I will have to take a deeper look in what happened |
|
@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 |
|
I'm gonna get this merged in soon, fyi @yanpitangui |
Thank you. Should I resolve the conflicts or are you doing it? |
|
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: ***@***.***>
|
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):