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

Change to use grpc-dotnet instead of Grpc.Core in C# SDK #3397

Merged
merged 15 commits into from
Sep 28, 2023
Merged

Change to use grpc-dotnet instead of Grpc.Core in C# SDK #3397

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7680635
Add conformance test implementation for C# SDK
yoshd Sep 20, 2023
a074e0e
add successful log
yoshd Sep 21, 2023
ff7e37e
Fix SetLabel/SetAnnotation tests
yoshd Sep 21, 2023
685a5df
Remove garbage
yoshd Sep 21, 2023
513cf29
Rename sdk test file
yoshd Sep 21, 2023
efb936f
Fix SetLabel test
yoshd Sep 21, 2023
870976b
Merge branch 'main' into add_csharp_conformance_test
yoshd Sep 21, 2023
e484d1c
Change to use grpc-dotnet instead of Grpc.Core
yoshd Sep 21, 2023
564d1ef
Remove Grpc.Core.Testing
yoshd Sep 21, 2023
f2d74f8
Merge branch 'main' into change_csharp_grpc_lib
yoshd Sep 22, 2023
d1660e0
Move proto file output
yoshd Sep 22, 2023
c287767
Fix proto file output destination for C#
yoshd Sep 22, 2023
3e57dc9
Merge branch 'main' into change_csharp_grpc_lib
yoshd Sep 26, 2023
2f2204b
Merge branch 'main' into change_csharp_grpc_lib
yoshd Sep 28, 2023
6ab40e6
Merge branch 'main' into change_csharp_grpc_lib
markmandel Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build/build-sdk-images/csharp/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ RUN wget -q https://packages.microsoft.com/config/ubuntu/19.04/packages-microsof


RUN apt-get update \
&& apt-get install -y dotnet-sdk-7.0 dotnet-sdk-3.1
&& apt-get install -y dotnet-sdk-7.0

# code generation scripts
COPY *.sh /root/
Expand Down
77 changes: 31 additions & 46 deletions build/build-sdk-images/csharp/gen.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,52 +16,37 @@

set -ex

header() {
cat /go/src/agones.dev/agones/build/boilerplate.go.txt "$1" | sponge "$1"
}
proto=/go/src/agones.dev/agones/proto
sdk=${proto}/sdk
googleapis=${proto}/googleapis
csharp_proto_file_output_dir=/go/src/agones.dev/agones/sdks/csharp/sdk/proto
Copy link
Member

@markmandel markmandel Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Curious - does it have to be in sdk/proto? Also wondering if we should update the .gitignore so it doesn't ignore ./proto folder anymore, since the csharp code is now generated at build time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't necessarily need to be in sdk/proto, but since only the C# SDK uses this specially rewritten .proto files, I decided to output it there.
Currently it doesn't seem to be ignored by .gitignore, but is there likely to be a problem?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I wasn't clear, lemme explain this another way:

Shall we put the proto files in /go/src/agones.dev/agones/sdks/csharp/proto (the sdk seems redundant), which is the same as how we do the same thing with rust https://github.com/googleforgames/agones/tree/main/sdks/rust/proto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I understand!
I did it that way at first, but the actual C# SDK implementation was in sdks/csharp/sdk.
For this reason, although it is redundant, I thought it would be better to put them in the same hierarchy.
WDYT?
https://github.com/googleforgames/agones/tree/main/sdks/csharp/sdk

Copy link
Member

Choose a reason for hiding this comment

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

oooooh! So it is!

I guess what's the usual convention for csharp? Are the protos usually kept with the source code (i.e. sdk) or kept separately (i.e. one level up)?

We should probably just do whatever the regular convention is - I don't actually mind either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally, the proto file is often shared between the server and client, so I think it should be placed in a separate folder from the source code.
(However, this time it seems to be different in that a proto file exclusively for the C# SDK is generated.)
That said, I agree that it would be better to have a structure similar to most projects.
Now let's modify it to output to sdks/csharp .

Copy link
Member

Choose a reason for hiding this comment

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

Now let's modify it to output to sdks/csharp .

Sorry, wasn't sure I 100% understood.

I think we're saying to leave it at /go/src/agones.dev/agones/sdks/csharp/sdk/proto yes? or no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that's no.
I modify it to csharp/sdk.
And I just committed it.

Copy link
Member

Choose a reason for hiding this comment

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

Got it! That works for me!


sdk=/go/src/agones.dev/agones/proto/sdk
googleapis=/go/src/agones.dev/agones/proto/googleapis
protoc_intermediate=/go/src/agones.dev/agones/sdks/csharp/proto
protoc_destination=/go/src/agones.dev/agones/sdks/csharp/sdk/generated

# Create temporary proto files
mkdir -p ${protoc_intermediate}
cp -r ${sdk} ${protoc_intermediate}
echo "Copying protobuffers to csharp sdk"
mkdir -p ${csharp_proto_file_output_dir}
cp -r ${sdk} ${csharp_proto_file_output_dir}
cp -r ${googleapis} ${csharp_proto_file_output_dir}

# Remove protoc-gen-openapiv2 definitions because C# package doesn't support grpc-gateway
sed -i -e 's/import "protoc-gen-openapiv2\/options\/annotations.proto";//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/info: {//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/title: "sdk.proto";//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -z 's/version: "version not set";\n };//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/schemes: HTTP;//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/consumes: "application\/json";//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -z 's/produces: "application\/json";\n};//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/bool disabled = 1.*/bool disabled = 1;/' ${protoc_intermediate}/sdk/sdk.proto

sed -i -e 's/import "protoc-gen-openapiv2\/options\/annotations.proto";//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/info: {//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/title: "alpha.proto";//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -z 's/version: "version not set";\n };//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/schemes: HTTP;//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/consumes: "application\/json";//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -z 's/produces: "application\/json";\n};//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/bool bool = 1.*/bool bool = 1;/' ${protoc_intermediate}/sdk/alpha/alpha.proto

# Generate C# proto file like `Sdk.cs`
protoc -I ${googleapis} -I ${protoc_intermediate}/sdk --plugin=protoc-gen-grpc=`which grpc_csharp_plugin` --csharp_out=${protoc_destination} ${protoc_intermediate}/sdk/sdk.proto
protoc -I ${googleapis} -I ${protoc_intermediate}/sdk/alpha --plugin=protoc-gen-grpc=`which grpc_csharp_plugin` --csharp_out=${protoc_destination} ${protoc_intermediate}/sdk/alpha/alpha.proto

# Generate grpc file like `SdkGrpc.cs`
protoc -I ${googleapis} -I ${protoc_intermediate}/sdk --plugin=protoc-gen-grpc=`which grpc_csharp_plugin` --grpc_out=${protoc_destination} ${protoc_intermediate}/sdk/sdk.proto
protoc -I ${googleapis} -I ${protoc_intermediate}/sdk/alpha --plugin=protoc-gen-grpc=`which grpc_csharp_plugin` --grpc_out=${protoc_destination} ${protoc_intermediate}/sdk/alpha/alpha.proto

cd ${protoc_destination}
header Alpha.cs
header AlphaGrpc.cs
header Sdk.cs
header SdkGrpc.cs

rm -rf ${protoc_intermediate}
sed -i -e 's/import "protoc-gen-openapiv2\/options\/annotations.proto";//' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -e 's/option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {//' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -e 's/info: {//' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -e 's/title: "sdk.proto";//' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -z 's/version: "version not set";\n };//' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -e 's/schemes: HTTP;//' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -e 's/consumes: "application\/json";//' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -z 's/produces: "application\/json";\n};//' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -e 's/bool disabled = 1.*/bool disabled = 1;/' ${csharp_proto_file_output_dir}/sdk/sdk.proto
sed -i -e 's/^ *$//' ${csharp_proto_file_output_dir}/sdk/sdk.proto

sed -i -e 's/import "protoc-gen-openapiv2\/options\/annotations.proto";//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -e 's/option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -e 's/info: {//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -e 's/title: "alpha.proto";//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -z 's/version: "version not set";\n };//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -e 's/schemes: HTTP;//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -e 's/consumes: "application\/json";//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -z 's/produces: "application\/json";\n};//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -e 's/bool bool = 1.*/bool bool = 1;/' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto
sed -i -e 's/^ *$//' ${csharp_proto_file_output_dir}/sdk/alpha/alpha.proto

echo "csharp code is generated at build time"
25 changes: 13 additions & 12 deletions sdks/csharp/sdk/AgonesSDK.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@
// See the License for the specific language governing permissions and
// limitations under the License.
using Agones.Dev.Sdk;
using Grpc.Core;
using Microsoft.Extensions.Logging;
using System;
using System.Threading;
using System.Threading.Tasks;
using Grpc.Core;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The namespace Grpc.Core remains, but it is included in grpc-dotnet.

using Grpc.Net.Client;

[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("Agones.Test")]
namespace Agones
Expand All @@ -33,8 +34,8 @@ public sealed class AgonesSDK : IAgonesSDK

internal SDK.SDKClient client;
internal readonly Alpha alpha;
internal readonly Channel channel;
internal AsyncClientStreamingCall<Empty, Empty> healthStream;
internal readonly GrpcChannel channel;
internal AsyncClientStreamingCall<Empty,Empty> healthStream;
internal readonly CancellationTokenSource cts;
internal readonly bool ownsCts;
internal CancellationToken ctoken;
Expand Down Expand Up @@ -74,7 +75,10 @@ public AgonesSDK(
}

ctoken = cts.Token;
channel = new Channel(Host, Port, ChannelCredentials.Insecure);
channel = GrpcChannel.ForAddress(
$"http://{Host}:{Port}"
);

client = sdkClient ?? new SDK.SDKClient(channel);
alpha = new Alpha(channel, requestTimeoutSec, cancellationTokenSource, logger);
}
Expand All @@ -91,16 +95,11 @@ public IAgonesAlphaSDK Alpha()
/// <summary>
/// Connect the underlying gRPC channel.
/// </summary>
/// <returns>True if successful</returns>
/// <returns>Always return true</returns>
[Obsolete("No need to call ConnectAsync anymore")]
public async Task<bool> ConnectAsync()
{
await channel.ConnectAsync(DateTime.UtcNow.AddSeconds(RequestTimeoutSec));
if (channel.State == ChannelState.Ready)
{
return true;
}
LogError($"Could not connect to the sidecar at {Host}:{Port}. Exited with connection state: {channel.State}.");
return false;
return true;
}

/// <summary>
Expand Down Expand Up @@ -387,6 +386,8 @@ public void Dispose()
cts.Dispose();
}

channel.Dispose();

// Since we don't provide any facility to unregister a WatchGameServer callback, set the event to null to
// clear its underlying invocation list, so we don't keep holding references to objects that would prevent
// them to be GC'd in case we don't go out of scope.
Expand Down
5 changes: 2 additions & 3 deletions sdks/csharp/sdk/Alpha.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Grpc.Net.Client;

[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("Agones.Test")]
namespace Agones
Expand All @@ -32,7 +33,6 @@ public sealed class Alpha : IAgonesAlphaSDK
public double RequestTimeoutSec { get; set; }

internal SDK.SDKClient client;
internal readonly Channel channel;
internal readonly IClientStreamWriter<Empty> healthStream;
internal readonly CancellationTokenSource cts;
internal readonly bool ownsCts;
Expand All @@ -42,7 +42,7 @@ public sealed class Alpha : IAgonesAlphaSDK
private bool _disposed;

public Alpha(
Channel channel,
GrpcChannel channel,
double requestTimeoutSec = 15,
CancellationTokenSource cancellationTokenSource = null,
ILogger logger = null)
Expand All @@ -62,7 +62,6 @@ public Alpha(
}

ctoken = cts.Token;
this.channel = channel;
client = new SDK.SDKClient(channel);
}

Expand Down
13 changes: 6 additions & 7 deletions sdks/csharp/sdk/csharp-sdk.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,17 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Google.Api.CommonProtos" Version="2.0.0" />
<PackageReference Include="Google.Protobuf" Version="3.21.9" />
<PackageReference Include="Grpc" Version="2.46.5" />
<PackageReference Include="Grpc.Core" Version="2.46.5" />
<PackageReference Include="Grpc.Tools" Version="2.50.0">
<PackageReference Include="Google.Api.CommonProtos" Version="2.10.0" />
<PackageReference Include="Google.Protobuf" Version="3.24.3" />
<PackageReference Include="Grpc.Net.Client" Version="2.57.0" />
<PackageReference Include="Grpc.Tools" Version="2.58.0">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>
<PackageReference Include="Microsoft.Extensions.Logging" Version="3.1.4" />
</ItemGroup>

<ItemGroup Condition="'$(Configuration)' == 'DebugProtoGen'">
<Protobuf Include="sdk.proto" ProtoRoot="../../../proto/sdk/;../../../proto/googleapis;../../../proto/grpc-gateway" OutputDir="generated/" CompileOutputs="false" />
<ItemGroup>
<Protobuf Include="./proto/sdk/sdk.proto;./proto/sdk/alpha/alpha.proto" AdditionalImportDirs="./proto/googleapis" GrpcServices="Client" />
</ItemGroup>
</Project>
Loading