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.

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

[C# grpc] Include grpc generated files automatically. #630

Closed
wants to merge 9 commits into from

Conversation

hcoona
Copy link
Contributor

@hcoona hcoona commented Oct 5, 2017

This change would include "%(filename)_grpc.cs" as well as "%(filename)_types.cs" only for the BondCodegen items with addition Options meta and the meta contains '--grpc'.

@msftclas
Copy link

msftclas commented Oct 5, 2017

CLA assistant check
All CLA requirements met.

.travis.yml Outdated
- if [ "$FLAVOR" == "cs" ]; then xbuild /p:Configuration=Fields cs/cs.sln; fi
# exclude the Compiler.vcxproj from cs/cs.sln on Linux (xbuild ignore it with a warning but msbuild fail on it)
- if [ "$FLAVOR" == "cs" ]; then sed '/21E175D5-BBDD-4B63-8FB7-38899BF2F9D1/d' cs/cs.sln > cs/cs_no_cpp.sln; fi
- if [ "$FLAVOR" == "cs" ]; then msbuild /p:Configuration=Debug cs/cs_no_cpp.sln; fi
Copy link
Member

@chwarr chwarr Oct 5, 2017

Choose a reason for hiding this comment

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

Switching to msbuild is understandable, but shouldn't be part of this PR. Can you revert the xbuild to msbuild changes from this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xbuild didn't get the property function work fine, so I have to switch to msbuild here. I'll make another PR for the msbuild switching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chwarr Please see PR #633

@@ -127,6 +127,12 @@
<DependentUpon>%(BondCodegen.Identity)</DependentUpon>
</_BondGeneratedFiles>

<_BondGeneratedFiles Include="@(BondCodegen -> '$(BondOutputDirectory)\%(FileName)_grpc.cs')"
Condition="$([System.String]::new('%(BondCodegen.Options)').Contains('--grpc'))">
Copy link
Member

Choose a reason for hiding this comment

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

This also needs to handle the case when the property $(BondOptions) contains "--grpc". Note that $(BondOptions) is only used for BondCodegen items that don't have overridden Options metadata. One of the examples can be changed to use $(BondOptions) as a test for this behavior.

@chwarr
Copy link
Member

chwarr commented Oct 5, 2017

Go ahead and add new commits (instead of rebasing). That will make follow ups to the PR easier. One of the maintainers can squash/rebase for the final merge.

@chwarr
Copy link
Member

chwarr commented Oct 12, 2017

Since this relies on MSBuild functionality, it would, when released, break anyone using xbuild. Let me see whether I can assess how large that population is. (I suspect that such a breaking change would be OK.)

@chwarr
Copy link
Member

chwarr commented Oct 20, 2017

Requiring MSBuild is fine, so we can rely on MSBuild-only functionality. After #633 is finished, this PR can be resumed atop those changes.

@chwarr
Copy link
Member

chwarr commented Oct 20, 2017

#633 has been merged (commit 85995fe), so this can be reworked atop that. You may want to keep an eye on #649, which is also touching .travis.yml, but doesn't materially affect the C# build.

chwarr pushed a commit to chwarr/bond that referenced this pull request Oct 21, 2017
Update the MSBuild codegen targets to automatically include the
generated _grpc.cs files if --grpc is passed to gbc.

Fixes microsoft#448
Closes microsoft#630
chwarr added a commit to chwarr/bond that referenced this pull request Oct 24, 2017
* The MSBuild codegen targets now detect --structs=false and skip
  automatic compilation of the non-existent `_types.cs` files.
* Added a gRPC example that show how to use a shared type assembly
  between a client and service.

Fixes microsoft#448
Closes microsoft#630
chwarr pushed a commit to chwarr/bond that referenced this pull request Oct 24, 2017
* Two cases need to be handeled: $BondOptions contains --grpc for
  no-overridden BondCodegen items, the %BondCodegen.Options contains
  --grpc. Also --grpc=false needs to be handled in both cases.
* Updated the examples ad tests to no longer include the
  generated *_grpc.cs files.

Fixes microsoft#448
Closes microsoft#630
@chwarr
Copy link
Member

chwarr commented Oct 24, 2017

I added support for --structs=false in PR #653, which includes the changes in this one as well. This PR will get closed automatically when #653 is merged: please leave it open until then.

@lalo lalo closed this in a120cd9 Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants