> On March 14, 2018, 9:29 a.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 220 (patched) > > <https://reviews.apache.org/r/66008/diff/1/?file=1973215#file1973215line220> > > > > Can we at least add a `TODO` here for what was advertised in the commit > > message, > > > > > This also replaces the use of string(APPEND CMAKE_CXX_FLAGS) with > > the canonical command add_compile_options (though setting these on a > > per-target basis would be even better, it's a separate issue). > > > > Ideally we would reference a ticket. > > > > No need to add this everywhere below, but this one sticks out > > especially since it says `GLOBAL` :/
I'm sorry, I thought I had. It's [MESOS-8033](https://issues.apache.org/jira/browse/MESOS-8033). Adding to the review. And this says `GLOBAL` because `add_compile_options` is a global-ish side-effect. It affects all targets from "this" directory downward (and while I don't like this design, and think we should fix it at some point, this configuration file is included in the top-level `CMakeLists.txt` file, making this essentially global). Oh wait, I get what you were saying now. So, adding compile options per target instead of here (and below) is "a separate issue" because it's a design change. It's not something we necessarily have to or even should do; as these flags are set similarly to the old build system. It is generally in CMake more appropriate to set flags on a per-target basis, so my commit message was just calling out why we didn't do that. I'll amend it and make it more clear. > On March 14, 2018, 9:29 a.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 224 (patched) > > <https://reviews.apache.org/r/66008/diff/1/?file=1973215#file1973215line224> > > > > Nit: let's put these each flag on a separate line already now, it'll be > > much nicer to evolve over time. ;) I went back and forth on this three times. I'm happy to go either way. > On March 14, 2018, 9:29 a.m., Benjamin Bannier wrote: > > cmake/CompilationConfigure.cmake > > Lines 226 (patched) > > <https://reviews.apache.org/r/66008/diff/1/?file=1973215#file1973215line226> > > > > We definitely do not want to include `-Weverything` ever. It is > > intended for testing diagnostics by upstream developers, but would warn > > about issues we would not want to address (e.g., struct packing). You > > probably meant `-Wextra` like in the `GNU` case. Good to know, thanks. > On March 14, 2018, 9:29 a.m., Benjamin Bannier wrote: > > src/CMakeLists.txt > > Lines 485 (patched) > > <https://reviews.apache.org/r/66008/diff/1/?file=1973216#file1973216line485> > > > > We should add a cmake configuration flag to disable `-Werror`. We have > > an equivalent configure flag `--disable-werror` since it is extremely > > unlikely that we'll support every possible setup (e.g., bleeding edge > > compilers, packaging setups). Let's give users a way to opt out. Hm... okay. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66008/#review199176 ----------------------------------------------------------- On March 9, 2018, 2:38 p.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66008/ > ----------------------------------------------------------- > > (Updated March 9, 2018, 2:38 p.m.) > > > Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John > Kordich, Joseph Wu, and Michael Park. > > > Bugs: MESOS-8658 > https://issues.apache.org/jira/browse/MESOS-8658 > > > Repository: mesos > > > Description > ------- > > We had previously been using the default sets of warnings, but now we > use the same warnings as on Autotools. This also means that we can > safely turn on the treat-warnings-as-errors (at least for the Mesos > code). However, doing so requires that we disable two common > possible-loss-of-data warnings on Windows that are not part of the > GNU/Clang default warnings. > > This also replaces the use of `string(APPEND CMAKE_CXX_FLAGS)` with > the canonical command `add_compile_options` (though setting these on a > per-target basis would be even better, it's a separate issue). > > > Diffs > ----- > > cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 > src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef > > > Diff: https://reviews.apache.org/r/66008/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrew Schwartzmeyer > >
