Skip to content

Redo C# examples to use new Grpc.Tools (with codegen build integration)#14684

Merged
jtattermusch merged 2 commits intogrpc:v1.17.xfrom
kkm000:package-grpc-tools-examples
Dec 6, 2018
Merged

Redo C# examples to use new Grpc.Tools (with codegen build integration)#14684
jtattermusch merged 2 commits intogrpc:v1.17.xfrom
kkm000:package-grpc-tools-examples

Conversation

@kkm000
Copy link
Copy Markdown
Contributor

@kkm000 kkm000 commented Mar 13, 2018 •
edited
Loading

  • No pre-compilation of proto files required;
  • Grpc and protobuf versions updated to current;
  • Tested under Windows and Linux dotnet and mono;
  • But not tested on Mac/mono;
  • README updated.

Depends on #13207 and #14683.

Version must be corrected where 1.10 is currently mentioned!
Changed to 1.14.
Changed to 1.17.

@kkm000 kkm000 mentioned this pull request Mar 13, 2018
Merged
@jtattermusch jtattermusch self-assigned this Mar 13, 2018
@jtattermusch jtattermusch self-requested a review March 13, 2018 08:15
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Mar 26, 2018 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Mar 26, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Mar 26, 2018 

[microbenchmarks] No significant performance differences

@kkm000 kkm000 mentioned this pull request Jun 14, 2018
Merged
@kkm000 kkm000 force-pushed the package-grpc-tools-examples branch 2 times, most recently from aa86c1e to affd149 Compare June 14, 2018 06:35
Redo C# examples to use new Grpc.Tools
6fcb5b2 
* No pre-compilation of proto files required;
* Tested under Windows and Linux dotnet and mono;
* But not tested on Mac/mono;
* README updated.
@kkm000 kkm000 force-pushed the package-grpc-tools-examples branch from affd149 to 6fcb5b2 Compare October 14, 2018 09:25
@jtattermusch jtattermusch changed the base branch from master to v1.17.x November 21, 2018 11:07
@jtattermusch jtattermusch added release notes: no Indicates if PR should not be in release notes kokoro:run labels Nov 21, 2018
@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Nov 21, 2018

@kkm000 I changed the PR base to v1.17.x release branch (which got created yesterday).
The process: After the release, we can merge the examples in the release branch. After then we will upmerge to the master branch.

jtattermusch
jtattermusch reviewed Nov 21, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

Mostly happy with the change, a few nits.

examples/csharp/Helloworld/GreeterClient/GreeterClient.csproj
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<AssemblyTitle>GreeterClient</AssemblyTitle>
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch Nov 21, 2018

Choose a reason for hiding this comment

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

why remove these?

Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch Nov 21, 2018

Choose a reason for hiding this comment

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

It's fine to remove, but I'd like to see what the reason is.

Copy link
Copy Markdown
Contributor Author

@kkm000 kkm000 Nov 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The thing is, there is a dozen or more parameters set by the build process, based one on the other, with sensible defaults. This randomly changes one of them--to its default (all of these base off the directory name in the end). So it just looked kind of random to me.

EDIT: This is in line with the SDK philosophy: the "classic" VS-maintained .csproj files became a monstrosity, so they decided to make these files for the SDK as simple as possible for human manipulation (as much as XML allows for that); basically, add sources and it "just works". And even the "add sources" is still automatic by default, although rather un-hermetizing, to say the least.

Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch Dec 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

sg, thanks for explaining.

examples/csharp/Helloworld/Greeter/Greeter.csproj
<PackageReference Include="Grpc" Version="1.17.0" />
<PackageReference Include="Grpc.Tools" Version="1.17.0" PrivateAssets="All" />

<Protobuf Include="../../../protos/helloworld.proto" Link="helloworld.proto" />
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch Nov 21, 2018

Choose a reason for hiding this comment

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

nit: i'm not a fan of <Protobuf> being under the same <ItemGroup> as the package references.

Copy link
Copy Markdown
Contributor Author

@kkm000 kkm000 Nov 29, 2018

Choose a reason for hiding this comment

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

Done (+1 other file).

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 21, 2018 

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,496      Total (=)      2,020,496

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,175,626      Total (>)     11,175,624

 No significant differences in binary sizes

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 3, 2018 

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,496      Total (=)      2,020,496

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,175,623      Total (>)     11,175,620

 No significant differences in binary sizes

jtattermusch
jtattermusch approved these changes Dec 6, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM.

@jtattermusch jtattermusch merged commit 8955557 into grpc:v1.17.x Dec 6, 2018
@srini100 srini100 added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels Dec 6, 2018
@jtattermusch jtattermusch mentioned this pull request Jan 3, 2019
Merged
@jtattermusch jtattermusch changed the title Redo C# examples to use new Grpc.Tools Redo C# examples to use new Grpc.Tools (with codegen build integration) Jan 8, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jtattermusch jtattermusch jtattermusch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jtattermusch jtattermusch

Labels

lang/C# release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kkm000 @grpc-testing @jtattermusch @grpc-kokoro @kokoro-team @srini100