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

gRPC C++ Public Header Directory Change #14210

Merged
merged 1 commit into from Feb 16, 2018
Merged

gRPC C++ Public Header Directory Change #14210

merged 1 commit into from Feb 16, 2018

Conversation

muxi
Copy link
Member

@muxi muxi commented Jan 27, 2018 •
edited

PR for gRFC proposal: grpc/proposal#57.

  • Move headers from include/grpc++ into include/grpcpp.
  • Change original headers into wrapper headers.
  • Update build system correspondingly.

@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]    +456  +0.0%

  [ = ]       0 TOTAL     +456  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]    +456  +0.0%

  [ = ]       0 TOTAL     +456  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]    +456  +0.0%

  [ = ]       0 TOTAL     +456  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@nicolasnoble
Copy link
Member

Could you add a line in https://github.com/grpc/grpc/blob/master/doc/cpp/pending_api_cleanups.md to remove the old grpc++ files ?

@muxi muxi force-pushed the move-cpp-headers branch 2 times, most recently from 6f290ea to 445c7d2 Compare February 9, 2018 00:33
@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None] +1.09Ki  +0.0%

  [ = ]       0 TOTAL  +1.09Ki  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc grpc deleted a comment from grpc-testing Feb 9, 2018
@grpc grpc deleted a comment from grpc-testing Feb 9, 2018
@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None] +1.09Ki  +0.0%

  [ = ]       0 TOTAL  +1.09Ki  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@muxi
Copy link
Member Author

muxi commented Feb 14, 2018

@nicolasnoble Could you please provide a review? Need to put this into v1.10.x cutting 2 Friday.

@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None] +1.09Ki  +0.0%

  [ = ]       0 TOTAL  +1.09Ki  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@vjpai vjpai requested review from vjpai and removed request for nicolasnoble February 15, 2018 06:39
@vjpai vjpai assigned vjpai and unassigned nicolasnoble Feb 15, 2018
vjpai
vjpai suggested changes Feb 15, 2018
View reviewed changes
Copy link
Member

@vjpai vjpai left a comment

Choose a reason for hiding this comment

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

Thanks for leaving test alone for now; this provides confidence that there is no API breaker in here. I have no problem with the change, except....

This PR should not go in without a sanity check to make sure that include/grpc++ stays dead. The sanity check should first prevent the addition of any new files into the include/grpc++ hierarchy. Second, it should make sure that every file in that hierarchy is marked with a DEPRECATED comment and has no content other than the license, the deprecated comment, the header guards, and a single include of the proper file. It is more important to do this right than to push it into this release cut.

@grpc grpc deleted a comment from grpc-testing Feb 15, 2018
@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None] +1.09Ki  +0.0%

  [ = ]       0 TOTAL  +1.09Ki  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

1 similar comment
@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

1 similar comment
@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None] +1.09Ki  +0.0%

  [ = ]       0 TOTAL  +1.09Ki  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None] +1.09Ki  +0.0%

  [ = ]       0 TOTAL  +1.09Ki  +0.0%

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

1 similar comment
@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@muxi
Copy link
Member Author

muxi commented Feb 16, 2018

Test failure is #14445.

@muxi muxi merged commit 34e8e0a into grpc:master Feb 16, 2018
@muxi muxi deleted the move-cpp-headers branch February 16, 2018 00:46
@muxi muxi mentioned this pull request Feb 21, 2018
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vjpai vjpai vjpai approved these changes

Assignees

@vjpai vjpai

Labels
lang/c++ priority/P0/RELEASE BLOCKER
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@muxi @grpc-testing @nicolasnoble @vjpai