You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/05/08 03:23:29 UTC
Review Request 66996: Removed the OpenSSL dependency for building
gRPC in Mesos.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66996/
-----------------------------------------------------------
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
Re: Review Request 66996: Removed the OpenSSL dependency for building
gRPC in Mesos.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> 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?
>
> Chun-Hung Hsiao wrote:
> 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.
>
> Chun-Hung Hsiao wrote:
> I'll just use the standard libs here as well.
Hmm. Actually, let me check the "unsecure" variant in `configure.ac` instead.
- 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
>
>
Re: Review Request 66996: Removed the OpenSSL dependency for building
gRPC in Mesos.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> 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
>
>
Re: Review Request 66996: Removed the OpenSSL dependency for building
gRPC in Mesos.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66996/#review202621
-----------------------------------------------------------
3rdparty/Makefile.am
Lines 326 (patched)
<https://reviews.apache.org/r/66996/#comment284529>
This variable name makes sense only 50% of the time, how about e.g., `GRPC_LIB_SUFFIX`?
3rdparty/Makefile.am
Lines 341-350 (original), 339-348 (patched)
<https://reviews.apache.org/r/66996/#comment284530>
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)"
3rdparty/Makefile.am
Lines 341-342 (original), 339-340 (patched)
<https://reviews.apache.org/r/66996/#comment284531>
Why are we removing these extra flags? They don't seem to come from the normal flags?
src/Makefile.am
Lines 175 (patched)
<https://reviews.apache.org/r/66996/#comment284532>
Why are we removing these extra flags? They don't seem to come from the normal flags?
src/Makefile.am
Line 182 (original), 185 (patched)
<https://reviews.apache.org/r/66996/#comment284533>
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?
src/python/native_common/ext_modules.py.in
Line 132 (original), 132 (patched)
<https://reviews.apache.org/r/66996/#comment284534>
This variable name makes sense only 50% of the time, how about e.g., `grpc_lib_suffix`?
src/python/native_common/ext_modules.py.in
Lines 151-152 (original)
<https://reviews.apache.org/r/66996/#comment284535>
How do we manage to link an object file (which has no dependency information) when SSL is enabled?
- Benjamin Bannier
On May 8, 2018, 5: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, 5: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
>
>
Re: Review Request 66996: Removed the OpenSSL dependency for building
gRPC in Mesos.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66996/#review202739
-----------------------------------------------------------
Fix it, then Ship it!
configure.ac
Lines 2071 (patched)
<https://reviews.apache.org/r/66996/#comment284721>
We should be able to use `GRPC_VARIANT` here as well as the name should not collide with what we do on the automake side.
- Benjamin Bannier
On May 9, 2018, 2:50 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66996/
> -----------------------------------------------------------
>
> (Updated May 9, 2018, 2:50 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.
>
>
> 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/2/
>
>
> Testing
> -------
>
> This patch does not work standalone. Tests are done in the next patch.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 66996: Removed the OpenSSL dependency for building
gRPC in Mesos.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66996/
-----------------------------------------------------------
(Updated May 9, 2018, 7:02 p.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
Changes
-------
Addressed Benjamin's comment.
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.
Diffs (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/66996/diff/2-3/
Testing
-------
This patch does not work standalone. Tests are done in the next patch.
Thanks,
Chun-Hung Hsiao
Re: Review Request 66996: Removed the OpenSSL dependency for building
gRPC in Mesos.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66996/
-----------------------------------------------------------
(Updated May 9, 2018, 12:50 a.m.)
Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-8798
https://issues.apache.org/jira/browse/MESOS-8798
Repository: mesos
Description (updated)
-------
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.
Diffs (updated)
-----
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/2/
Changes: https://reviews.apache.org/r/66996/diff/1-2/
Testing
-------
This patch does not work standalone. Tests are done in the next patch.
Thanks,
Chun-Hung Hsiao