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/08/30 21:06:35 UTC
Review Request 68576: Mesos: Used standard macros for appending
compile flags.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and James Peach.
Bugs: MESOS-9193
https://issues.apache.org/jira/browse/MESOS-9193
Repository: mesos
Description
-------
This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
`AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
Diffs
-----
configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
m4/ax_append_compile_flags.m4 PRE-CREATION
m4/ax_append_flag.m4 PRE-CREATION
m4/ax_require_defined.m4 PRE-CREATION
Diff: https://reviews.apache.org/r/68576/diff/1/
Testing
-------
make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
Thanks,
Chun-Hung Hsiao
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Aug. 31, 2018, 11:31 a.m., Benjamin Bannier wrote:
> > configure.ac
> > Line 635 (original), 635 (patched)
> > <https://reviews.apache.org/r/68576/diff/3/?file=2079148#file2079148line635>
> >
> > This code is pretty robust and should work with any compiler. How about moving it out of this switch statement? We could then remove the inconsistent modification for `gnu` compilers below.
GCC uses `Wno-unused-local-typedefs` (with an additional `s`) so I prefer keeping this as is. Also it's easier to put compiler-specific comments. Dropping.
> On Aug. 31, 2018, 11:31 a.m., Benjamin Bannier wrote:
> > m4/ax_append_compile_flags.m4
> > Lines 1 (patched)
> > <https://reviews.apache.org/r/68576/diff/3/?file=2079149#file2079149line1>
> >
> > Let's use the file from upstream `HEAD`, e.g., from `4dfd2f87b4dda8e8a7e1fb7714a9dae66b8badb4`.
We couldn't use the upstream `HEAD` since it requires Autoconf 2.64, which is not the default on CentOS 6 (2.63) and Ubuntu 14.04 (2.59). Based on an offline discussion, I'll pick the latest commit that supports 2.59, namely `3e4a8395a33db74da6ed7b99a561bf7e4d2b06b3`, unless you think that we don't need to maintain version consistency between these three files.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/#review208165
-----------------------------------------------------------
On Aug. 31, 2018, 1:23 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68576/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2018, 1:23 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and James Peach.
>
>
> Bugs: MESOS-9193
> https://issues.apache.org/jira/browse/MESOS-9193
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
> `AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
> the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
>
> The macros are retrieved from the following repository with tag
> `v2014.10.15` to support Autoconf 2.59+:
> http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=tree;f=m4;hb=34f91518355f1f2cba082678c60008a1ed8d009a
>
>
> Diffs
> -----
>
> configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
> m4/ax_append_compile_flags.m4 PRE-CREATION
> m4/ax_append_flag.m4 PRE-CREATION
> m4/ax_require_defined.m4 PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68576/diff/3/
>
>
> Testing
> -------
>
> make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Aug. 31, 2018, 11:31 a.m., Benjamin Bannier wrote:
> > m4/ax_append_compile_flags.m4
> > Lines 1 (patched)
> > <https://reviews.apache.org/r/68576/diff/3/?file=2079149#file2079149line1>
> >
> > Let's use the file from upstream `HEAD`, e.g., from `4dfd2f87b4dda8e8a7e1fb7714a9dae66b8badb4`.
>
> Chun-Hung Hsiao wrote:
> We couldn't use the upstream `HEAD` since it requires Autoconf 2.64, which is not the default on CentOS 6 (2.63) and Ubuntu 14.04 (2.59). Based on an offline discussion, I'll pick the latest commit that supports 2.59, namely `3e4a8395a33db74da6ed7b99a561bf7e4d2b06b3`, unless you think that we don't need to maintain version consistency between these three files.
>
> Chun-Hung Hsiao wrote:
> Hmm there's actually no difference between `3e4a839` and `34f9151` so I'll just update the commit message.
After discussion, I'll take your suggestion to use the latest macros except for `ax_append_flag.m4`.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/#review208165
-----------------------------------------------------------
On Aug. 31, 2018, 5:27 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68576/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2018, 5:27 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and James Peach.
>
>
> Bugs: MESOS-9193
> https://issues.apache.org/jira/browse/MESOS-9193
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
> `AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
> the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
>
> The macros are retrieved from the following repository with commit
> `3e4a839` to support Autoconf 2.59+:
> http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=tree;f=m4;hb=3e4a839
>
>
> Diffs
> -----
>
> configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
> m4/ax_append_compile_flags.m4 PRE-CREATION
> m4/ax_append_flag.m4 PRE-CREATION
> m4/ax_require_defined.m4 PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68576/diff/3/
>
>
> Testing
> -------
>
> make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Aug. 31, 2018, 11:31 a.m., Benjamin Bannier wrote:
> > m4/ax_append_compile_flags.m4
> > Lines 1 (patched)
> > <https://reviews.apache.org/r/68576/diff/3/?file=2079149#file2079149line1>
> >
> > Let's use the file from upstream `HEAD`, e.g., from `4dfd2f87b4dda8e8a7e1fb7714a9dae66b8badb4`.
>
> Chun-Hung Hsiao wrote:
> We couldn't use the upstream `HEAD` since it requires Autoconf 2.64, which is not the default on CentOS 6 (2.63) and Ubuntu 14.04 (2.59). Based on an offline discussion, I'll pick the latest commit that supports 2.59, namely `3e4a8395a33db74da6ed7b99a561bf7e4d2b06b3`, unless you think that we don't need to maintain version consistency between these three files.
Hmm there's actually no difference between `3e4a839` and `34f9151` so I'll just update the commit message.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/#review208165
-----------------------------------------------------------
On Aug. 31, 2018, 1:23 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68576/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2018, 1:23 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and James Peach.
>
>
> Bugs: MESOS-9193
> https://issues.apache.org/jira/browse/MESOS-9193
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
> `AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
> the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
>
> The macros are retrieved from the following repository with tag
> `v2014.10.15` to support Autoconf 2.59+:
> http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=tree;f=m4;hb=34f91518355f1f2cba082678c60008a1ed8d009a
>
>
> Diffs
> -----
>
> configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
> m4/ax_append_compile_flags.m4 PRE-CREATION
> m4/ax_append_flag.m4 PRE-CREATION
> m4/ax_require_defined.m4 PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68576/diff/3/
>
>
> Testing
> -------
>
> make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/#review208165
-----------------------------------------------------------
Fix it, then Ship it!
LGTM, but to ease future maintenance I'd suggest to use the most recent m4 macro definitions; I left some versions that worked for me. We will likely lifecycle the files indepedently, so the commit message should call out where each individual file added here came from.
configure.ac
Line 635 (original), 635 (patched)
<https://reviews.apache.org/r/68576/#comment291938>
This code is pretty robust and should work with any compiler. How about moving it out of this switch statement? We could then remove the inconsistent modification for `gnu` compilers below.
m4/ax_append_compile_flags.m4
Lines 1 (patched)
<https://reviews.apache.org/r/68576/#comment291941>
Let's use the file from upstream `HEAD`, e.g., from `4dfd2f87b4dda8e8a7e1fb7714a9dae66b8badb4`.
m4/ax_append_flag.m4
Lines 55 (patched)
<https://reviews.apache.org/r/68576/#comment291940>
If all we care about is compatibility with 2.59, we can use newer versions.
I checked and this file still required 2.59 up until upstream `3e4a839`, identical for `ax_check_compile_flag.m4`. Let's use that revision instead.
m4/ax_require_defined.m4
Lines 1 (patched)
<https://reviews.apache.org/r/68576/#comment291942>
Let's use the file from upstream `HEAD`, e.g., from `4dfd2f87b4dda8e8a7e1fb7714a9dae66b8badb4`.
- Benjamin Bannier
On Aug. 31, 2018, 3:23 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68576/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2018, 3:23 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and James Peach.
>
>
> Bugs: MESOS-9193
> https://issues.apache.org/jira/browse/MESOS-9193
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
> `AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
> the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
>
> The macros are retrieved from the following repository with tag
> `v2014.10.15` to support Autoconf 2.59+:
> http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=tree;f=m4;hb=34f91518355f1f2cba082678c60008a1ed8d009a
>
>
> Diffs
> -----
>
> configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
> m4/ax_append_compile_flags.m4 PRE-CREATION
> m4/ax_append_flag.m4 PRE-CREATION
> m4/ax_require_defined.m4 PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68576/diff/3/
>
>
> Testing
> -------
>
> make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/#review208194
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On Aug. 31, 2018, 7:43 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68576/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2018, 7:43 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and James Peach.
>
>
> Bugs: MESOS-9193
> https://issues.apache.org/jira/browse/MESOS-9193
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
> `AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
> the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
>
> The macros are retrieved from the following repository:
> https://git.savannah.gnu.org/git/autoconf-archive.git
> The `ax_append_compile_flags.m4` and `ax_required_defined.m4` macros are
> retrieved from the HEAD (`4dfd2f8`) at the time; the `ax_append_flag.m4`
> macro is retrieved from commit `3e4a839` to support Autoconf 2.59+.
>
> NOTE: CentOS 6 uses Autoconf 2.63 and Ubuntu 14.04 uses 2.59.
>
>
> Diffs
> -----
>
> configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
> m4/ax_append_compile_flags.m4 PRE-CREATION
> m4/ax_append_flag.m4 PRE-CREATION
> m4/ax_require_defined.m4 PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68576/diff/4/
>
>
> Testing
> -------
>
> make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/
-----------------------------------------------------------
(Updated Aug. 31, 2018, 5:43 p.m.)
Review request for mesos, Benjamin Bannier and James Peach.
Bugs: MESOS-9193
https://issues.apache.org/jira/browse/MESOS-9193
Repository: mesos
Description (updated)
-------
This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
`AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
The macros are retrieved from the following repository:
https://git.savannah.gnu.org/git/autoconf-archive.git
The `ax_append_compile_flags.m4` and `ax_required_defined.m4` macros are
retrieved from the HEAD (`4dfd2f8`) at the time; the `ax_append_flag.m4`
macro is retrieved from commit `3e4a839` to support Autoconf 2.59+.
NOTE: CentOS 6 uses Autoconf 2.63 and Ubuntu 14.04 uses 2.59.
Diffs (updated)
-----
configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
m4/ax_append_compile_flags.m4 PRE-CREATION
m4/ax_append_flag.m4 PRE-CREATION
m4/ax_require_defined.m4 PRE-CREATION
Diff: https://reviews.apache.org/r/68576/diff/4/
Changes: https://reviews.apache.org/r/68576/diff/3-4/
Testing
-------
make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
Thanks,
Chun-Hung Hsiao
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/
-----------------------------------------------------------
(Updated Aug. 31, 2018, 5:27 p.m.)
Review request for mesos, Benjamin Bannier and James Peach.
Changes
-------
Updated the macros with the latest commit supporting Autoconf 2.59.
Bugs: MESOS-9193
https://issues.apache.org/jira/browse/MESOS-9193
Repository: mesos
Description (updated)
-------
This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
`AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
The macros are retrieved from the following repository with tag
`v2014.10.15` to support Autoconf 2.59+:
http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=tree;f=m4;hb=3e4a839
Diffs
-----
configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
m4/ax_append_compile_flags.m4 PRE-CREATION
m4/ax_append_flag.m4 PRE-CREATION
m4/ax_require_defined.m4 PRE-CREATION
Diff: https://reviews.apache.org/r/68576/diff/3/
Testing
-------
make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
Thanks,
Chun-Hung Hsiao
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/
-----------------------------------------------------------
(Updated Aug. 31, 2018, 1:23 a.m.)
Review request for mesos, Benjamin Bannier and James Peach.
Changes
-------
Supported Autoconf 2.59.
Bugs: MESOS-9193
https://issues.apache.org/jira/browse/MESOS-9193
Repository: mesos
Description (updated)
-------
This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
`AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
The macros are retrieved from the following repository with tag
`v2014.10.15` to support Autoconf 2.59+:
http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=tree;f=m4;hb=34f91518355f1f2cba082678c60008a1ed8d009a
Diffs (updated)
-----
configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
m4/ax_append_compile_flags.m4 PRE-CREATION
m4/ax_append_flag.m4 PRE-CREATION
m4/ax_require_defined.m4 PRE-CREATION
Diff: https://reviews.apache.org/r/68576/diff/3/
Changes: https://reviews.apache.org/r/68576/diff/2-3/
Testing
-------
make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
Thanks,
Chun-Hung Hsiao
Re: Review Request 68576: Mesos: Used standard macros for appending
compile flags.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68576/
-----------------------------------------------------------
(Updated Aug. 30, 2018, 9:16 p.m.)
Review request for mesos, Benjamin Bannier and James Peach.
Changes
-------
Updated inaccurate comments.
Bugs: MESOS-9193
https://issues.apache.org/jira/browse/MESOS-9193
Repository: mesos
Description
-------
This patch imports the `AX_APPEND_COMPILE_FLAGS` macro and its dependent
`AX_REQUIRE_DEFINED` and `AX_APPEND_FLAG` macros to Autotools, and used
the macro to add `-Wno-unused-local-typedef` for Clang 3.6.
Diffs (updated)
-----
configure.ac e4fbd8cab04707c1fe0c6d5ade7b7a7dca9a7cb7
m4/ax_append_compile_flags.m4 PRE-CREATION
m4/ax_append_flag.m4 PRE-CREATION
m4/ax_require_defined.m4 PRE-CREATION
Diff: https://reviews.apache.org/r/68576/diff/2/
Changes: https://reviews.apache.org/r/68576/diff/1-2/
Testing
-------
make check with clang 3.5.0/3.6.0/3.9.0/5.0.1.
Thanks,
Chun-Hung Hsiao