You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/03/09 22:39:28 UTC
Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C` and
`CXX` versions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66012/
-----------------------------------------------------------
Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
Repository: mesos
Description
-------
By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
libevent, and zlib, we avoid CMake warning us about set but unused
variables (since they do not use the `CXX` variables).
Diffs
-----
3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191
Diff: https://reviews.apache.org/r/66012/diff/1/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C`
and `CXX` versions.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66012/#review199032
-----------------------------------------------------------
Ship it!
I'm fine with these changes, but the description can be improved a bit I think. I know that this change relates to some of the later patches, so maybe mention that several of the fixes are built on top of this one (I guess that's implied, though). Maybe mention that this allows for granularity between C/CPP cmake flags, even though it's rather clear from the code.
- John Kordich
On March 9, 2018, 10:39 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66012/
> -----------------------------------------------------------
>
> (Updated March 9, 2018, 10:39 p.m.)
>
>
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
> libevent, and zlib, we avoid CMake warning us about set but unused
> variables (since they do not use the `CXX` variables).
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191
>
>
> Diff: https://reviews.apache.org/r/66012/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C`
and `CXX` versions.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On March 14, 2018, 9:44 a.m., Benjamin Bannier wrote:
> > Are we sure that this does not break other flags we forwarded to external projects? We seem to go from forwarding an open set of flags to only forwarding flags related to `C` and `CXX`. I am thinking e.g., about `CMAKE_GENERATOR`, see https://github.com/apache/mesos/blob/master/3rdparty/CMakeLists.txt#L101-L102
> >
> > # TODO(andschwa): Set the CMAKE_GENERATOR explicitly as an argmuent to
> > # `ExternalProject_Add`.
> >
> > but I could imagine this change to affect other areas as well. Or do I completely misunderstand this change?
You misunderstand, but I don't blame you :)
Both `CMAKE_CXX_FORWARD_ARGS` and `CMAKE_C_FORWARD_ARGS` start by including `CMAKE_FORWARD_ARGS`, which has all the "shared" flags like `CMAKE_GENERATOR`. The only difference now is instead of adding _both_ `CXX` and `C` flags to `CMAKE_FORWARD_ARGS`, I split it correctly into the first two mentioned variables, and they forward the respective `CXX/C` flags variables.
Otherwise during configuration, `C` projects complain about being given (but not using) `CXX` flags (and vice versa, if we had `C` flags...).
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66012/#review199181
-----------------------------------------------------------
On March 9, 2018, 2:39 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66012/
> -----------------------------------------------------------
>
> (Updated March 9, 2018, 2:39 p.m.)
>
>
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
> libevent, and zlib, we avoid CMake warning us about set but unused
> variables (since they do not use the `CXX` variables).
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191
>
>
> Diff: https://reviews.apache.org/r/66012/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C`
and `CXX` versions.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66012/#review199181
-----------------------------------------------------------
Are we sure that this does not break other flags we forwarded to external projects? We seem to go from forwarding an open set of flags to only forwarding flags related to `C` and `CXX`. I am thinking e.g., about `CMAKE_GENERATOR`, see https://github.com/apache/mesos/blob/master/3rdparty/CMakeLists.txt#L101-L102
# TODO(andschwa): Set the CMAKE_GENERATOR explicitly as an argmuent to
# `ExternalProject_Add`.
but I could imagine this change to affect other areas as well. Or do I completely misunderstand this change?
- Benjamin Bannier
On March 9, 2018, 11:39 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66012/
> -----------------------------------------------------------
>
> (Updated March 9, 2018, 11:39 p.m.)
>
>
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
> libevent, and zlib, we avoid CMake warning us about set but unused
> variables (since they do not use the `CXX` variables).
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191
>
>
> Diff: https://reviews.apache.org/r/66012/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C`
and `CXX` versions.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66012/#review199462
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On March 19, 2018, 12:13 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66012/
> -----------------------------------------------------------
>
> (Updated March 19, 2018, 12:13 p.m.)
>
>
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
> libevent, and zlib, we avoid CMake warning us about set but unused
> variables (since they do not use the `CXX` variables).
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt e0c1538eaaef4bf6a198e89867a13763f264deb3
>
>
> Diff: https://reviews.apache.org/r/66012/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C`
and `CXX` versions.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66012/
-----------------------------------------------------------
(Updated March 19, 2018, 12:13 p.m.)
Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
Changes
-------
Rebased.
Repository: mesos
Description
-------
By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
libevent, and zlib, we avoid CMake warning us about set but unused
variables (since they do not use the `CXX` variables).
Diffs (updated)
-----
3rdparty/CMakeLists.txt e0c1538eaaef4bf6a198e89867a13763f264deb3
Diff: https://reviews.apache.org/r/66012/diff/3/
Changes: https://reviews.apache.org/r/66012/diff/2-3/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C`
and `CXX` versions.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66012/
-----------------------------------------------------------
(Updated March 14, 2018, 4:15 p.m.)
Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
Changes
-------
Rebased.
Repository: mesos
Description
-------
By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
libevent, and zlib, we avoid CMake warning us about set but unused
variables (since they do not use the `CXX` variables).
Diffs (updated)
-----
3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191
Diff: https://reviews.apache.org/r/66012/diff/2/
Changes: https://reviews.apache.org/r/66012/diff/1-2/
Testing
-------
Thanks,
Andrew Schwartzmeyer