Re: 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/#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.
--- 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.
--- 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
Re: Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C` and `CXX` versions.
> 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.
--- 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.
--- 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 > >