> On March 20, 2018, 4:55 p.m., Benjamin Bannier wrote: > > include/csi/spec.hpp > > Lines 23 (patched) > > <https://reviews.apache.org/r/66156/diff/1/?file=1982683#file1982683line23> > > > > Not really related to this particular patch, but referencing this > > somewhat internal define in a public header is not great since it requires > > users of this header to know about our build setup. This is different from > > e.g., branching on say `__WINDOWS__` which is compiler-provided. > > > > Does it make sense for us to e.g., generate an empty > > `csi/csi.grpc.pb.h` when gRPC is not enabled and include it unconditionally > > here? > > > > Anyway, probably work for some other patch. > > Chun-Hung Hsiao wrote: > I'm not sure about this and feel a bit uncomfortable generating blank > headers. Also we already have things like this in the code base: > https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/profiler.cpp#L33
The example you gave is in not in a public header and even in a compiled file. Users of this file do not need to know about the define. To not have empty headers we could think about removing the include of the generated header out of the public header. There are probably other solutions as well if we think hard enough. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66156/#review199546 ----------------------------------------------------------- On March 21, 2018, 12:44 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66156/ > ----------------------------------------------------------- > > (Updated March 21, 2018, 12:44 a.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-8492 > https://issues.apache.org/jira/browse/MESOS-8492 > > > Repository: mesos > > > Description > ------- > > By generating `csi.pb.h` even if gRPC is disabled, other Mesos protocol > buffers can embed the CSI protocol buffers. This is required if we want > to checkpoint disk profiles in `ResourceProviderState`. > > Moreover, we relaxed the precondition of compiling most files, as the > gRPC is only required by SLRP, CSI clients and the tests. > > > Diffs > ----- > > 3rdparty/Makefile.am f4cb731198e271b967b920235ed2785e9296f9e1 > include/csi/spec.hpp df01f44e6f89a03eff9aef77c29ca71495d767f2 > src/CMakeLists.txt fb9e9d4cfc4a62830fe3065a139ae14401c0e52e > src/Makefile.am 84753794eca822ab251200cccb907b328c849fb7 > src/slave/flags.hpp 0c67bf214ceb93ae7ff088bec2648fa26ddac59e > src/slave/flags.cpp 962b07c1d701f4ab819b14730fbc116b981433bb > src/slave/slave.cpp 915d4f1bbd642ca5d2587cfdd846262cb7e08788 > > > Diff: https://reviews.apache.org/r/66156/diff/2/ > > > Testing > ------- > > `sudo make check` > > `make distcheck` with and without `--enable-grpc` > > > Thanks, > > Chun-Hung Hsiao > >
