mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 66996: Removed the OpenSSL dependency for building gRPC in Mesos.
Date Tue, 08 May 2018 22:14:44 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message