> On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote: > > 3rdparty/Makefile.am > > Lines 326 (patched) > > <https://reviews.apache.org/r/66996/diff/1/?file=2017785#file2017785line326> > > > > This variable name makes sense only 50% of the time, how about e.g., > > `GRPC_LIB_SUFFIX`?
I'll use `GRPC_VARIANT`. > On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote: > > 3rdparty/Makefile.am > > Lines 341-350 (original), 339-348 (patched) > > <https://reviews.apache.org/r/66996/diff/1/?file=2017785#file2017785line345> > > > > It is very hard to see what is actually being execute here in what > > environment. Could you reflow this to e.g., always pass the env after > > `make`, reorder the variables and reflow the code? > > > > $(MAKE) $(AM_MAKEFLAGS) \ > > $(LIB_GRPC:%=$(abs_builddir)/%) \ > > CC="$(CC)" \ > > CXX="$(CXX)" \ > > LD="$(CC)" \ > > LDXX="$(CXX)" \ > > CPPFLAGS="$(PROTOBUF_INCLUDE_FLAGS) \ > > $(SSL_INCLUDE_FLAGS) \ > > $(ZLIB_INCLUDE_FLAGS)" \ > > LDFLAGS="$(PROTOBUF_LINKER_FLAGS) \ > > $(SSL_LINKER_FLAGS) \ > > $(ZLIB_LINKER_FLAGS)" \ > > HAS_PKG_CONFIG=false \ > > NO_PROTOC=false \ > > PROTOC="$(PROTOC)" gRPC's makefile uses target-specific variable assignments (https://www.gnu.org/software/make/manual/html_node/Target_002dspecific.html) for appending `CPPFLAGS`. It seems that values taken from the command line will take precedence and the append will be ignored, and this is why I assign `CPPFLAGS` in the environment. Will add a comment about this. > On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote: > > 3rdparty/Makefile.am > > Lines 341-342 (original), 339-340 (patched) > > <https://reviews.apache.org/r/66996/diff/1/?file=2017785#file2017785line345> > > > > Why are we removing these extra flags? They don't seem to come from the > > normal flags? Those flags are no longer required in grpc v1.10. Will use another patch to remove them first. > On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote: > > src/Makefile.am > > Lines 175 (patched) > > <https://reviews.apache.org/r/66996/diff/1/?file=2017787#file2017787line175> > > > > Why are we removing these extra flags? They don't seem to come from the > > normal flags? The SSL flags should have already been set up in `LIBS` by `configure.ac` when SSL is enabled: https://github.com/apache/mesos/blob/master/configure.ac#L1864 > On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote: > > src/Makefile.am > > Line 182 (original), 185 (patched) > > <https://reviews.apache.org/r/66996/diff/1/?file=2017787#file2017787line186> > > > > It is not clear to me that this would always find the correct > > libraries. I see e.g., that the cmake build does not seem to use the > > `_unsecure` suffix even when the libraries where built without SSL. > > > > Do we need to explicitly discover these libraries during configure time > > to make sure we don't risk failing to link later? We already did: https://github.com/apache/mesos/blob/master/configure.ac#L2072. The assumption here is that for custom grpc, we assume the whole grpc package is installed so the standard grpc library must be there. > On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote: > > src/python/native_common/ext_modules.py.in > > Line 132 (original), 132 (patched) > > <https://reviews.apache.org/r/66996/diff/1/?file=2017788#file2017788line132> > > > > This variable name makes sense only 50% of the time, how about e.g., > > `grpc_lib_suffix`? I'll use `grpc_variant`. > On May 8, 2018, 9:33 a.m., Benjamin Bannier wrote: > > src/python/native_common/ext_modules.py.in > > Lines 151-152 (original) > > <https://reviews.apache.org/r/66996/diff/1/?file=2017788#file2017788line152> > > > > How do we manage to link an object file (which has no dependency > > information) when SSL is enabled? These SSL flags should have already been set up in `LIBS` by `configure.ac` when SSL is enabled: https://github.com/apache/mesos/blob/master/configure.ac#L1864 https://github.com/apache/mesos/blob/master/src/Makefile.am#L2196 https://github.com/apache/mesos/blob/master/src/python/native_common/ext_modules.py.in#L168 - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66996/#review202621 ----------------------------------------------------------- On May 8, 2018, 3:23 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66996/ > ----------------------------------------------------------- > > (Updated May 8, 2018, 3:23 a.m.) > > > Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu. > > > Bugs: MESOS-8798 > https://issues.apache.org/jira/browse/MESOS-8798 > > > Repository: mesos > > > Description > ------- > > When the SSL build feature is disabled, Mesos now builds > `libgrpc_unsecure` and `libgrpc++_unsecure` instead of `libgrpc` and > `libgrpc++`, so the SSL headers and libraries are no longer required. > > NOTE: gRPC v1.10 no longer needs `-Wno-deprecated-declarations` and > `-Wno-unused-function` when building with OpenSSL v1.1. > > > Diffs > ----- > > 3rdparty/Makefile.am 8d9fa85dd65a94d91670d54dab36deea80d14996 > configure.ac 429797c35b93a6df69ba0cb0fc28cb66a3171074 > src/Makefile.am 7e91681e3b8b217f8b23fa5347e059640c62fc65 > src/python/native_common/ext_modules.py.in > 87387fd580c40b252fc82f98b5238b9b9120903a > > > Diff: https://reviews.apache.org/r/66996/diff/1/ > > > Testing > ------- > > This patch does not work standalone. Tests are done in the next patch. > > > Thanks, > > Chun-Hung Hsiao > >
