> On March 1, 2017, 2:13 a.m., Joseph Wu wrote: > > cmake/CompilationConfigure.cmake > > Line 291 (original), 291 (patched) > > <https://reviews.apache.org/r/57052/diff/2/?file=1652318#file1652318line291> > > > > Not yours, but looks like this one actually doesn't work on Posix. > > > > It ends up as: > > ``` > > #define BUILD_TIME "%s" > > ``` > > > > I'll fix it while I'm modifying this part of the build system: > > ``` > > execute_process( > > COMMAND date +%s > > OUTPUT_VARIABLE BUILD_TIME > > OUTPUT_STRIP_TRAILING_WHITESPACE > > ) > > ```
I believe this is a CMake versioning issue. I had to explicitly call `cmake3` for this to work (but AFAIK we don't "officially" support 2.x, I say that because we already make use of a lot of 3.x features). > On March 1, 2017, 2:13 a.m., Joseph Wu wrote: > > cmake/CompilationConfigure.cmake > > Lines 297 (patched) > > <https://reviews.apache.org/r/57052/diff/2/?file=1652318#file1652318line297> > > > > Would be better with a comment :D > > ``` > > # Emit the BUILD_DATE, BUILD_TIME, and BUILD_USER variables into a file. > > # This will be updated each time `cmake` is run. > > ``` Oh, yes it would :D > On March 1, 2017, 2:13 a.m., Joseph Wu wrote: > > cmake/CompilationConfigure.cmake > > Lines 299 (patched) > > <https://reviews.apache.org/r/57052/diff/2/?file=1652318#file1652318line299> > > > > Just for symmetry, let's put this in > > > > "${CMAKE_BINARY_DIR}/src/common/build_config.hpp" I'm fine with that if you were. I went with include/mesos as that's the only currently generated folder inside build/include. Definitely prefer include/common. > On March 1, 2017, 2:13 a.m., Joseph Wu wrote: > > cmake/CompilationConfigure.cmake > > Line 308 (original), 312 (patched) > > <https://reviews.apache.org/r/57052/diff/2/?file=1652318#file1652318line312> > > > > What do you feel about "USE_CMAKE_BUILD_CONFIG" instead? > > "HAVE_BUILD_CONFIG_H" seems too much like a header guard. No preference. `HAVE_CONFIG_H` is very common (the `HAVE_FEATURE` is itself a common pattern), but I don't care one way or another. > On March 1, 2017, 2:13 a.m., Joseph Wu wrote: > > src/common/build.cpp > > Lines 26 (patched) > > <https://reviews.apache.org/r/57052/diff/2/?file=1652319#file1652319line26> > > > > Would help to have a nice beefy comment :) > > ``` > > // NOTE: On CMake, instead of defining `BUILD_DATE|TIME|USER` as > > // compiler flags, we instead emit a header file with the definitions. > > // This facilitates incremental builds as the compiler flags will > > // no longer change with every invocation of the build. > > // TODO(josephw): After deprecating autotools, remove this guard. > > ``` I knew there was something I was forgetting. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57052/#review167198 ----------------------------------------------------------- On March 1, 2017, 12:42 a.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57052/ > ----------------------------------------------------------- > > (Updated March 1, 2017, 12:42 a.m.) > > > Review request for mesos, Alex Clemmer, Joseph Wu, and Michael Park. > > > Bugs: MESOS-7172 > https://issues.apache.org/jira/browse/MESOS-7172 > > > Repository: mesos > > > Description > ------- > > Commit c7fc1377b introduced a bug that prevented any incremental > recompilation. By defining the `BUILD_TIME` in `MESOS_CPPFLAGS`, > every build was seen as having a root dependency (the flags) > changed, causing a rebuild of every single source in Mesos. > > This patch introduces a CMake `configure_file` directive which > takes in `build_config.hpp.in` and emits `build_config.hpp` with the > `BUILD_TIME`, `BUILD_DATE`, and `BUILD_USER` variables defined. > It also sets `HAVE_BUILD_CONFIG_H` which `build.cpp` uses to `#include` > the configuration file. The result is that the date, time, and user > are set at the point of configuration (invocation of CMake) instead > of at build, thus allowing for incremental rebuilds. > > > Diffs > ----- > > cmake/CompilationConfigure.cmake 7808d1a0086a7999991af8dcb2dd796b9424c7d5 > src/common/build.cpp 090b59fb22bfc00516d65f501f8f18cd85a5b4cd > src/common/build_config.hpp.in PRE-CREATION > > > Diff: https://reviews.apache.org/r/57052/diff/2/ > > > Testing > ------- > > cmake; make && make check on Linux > cmake; VS build stout-tests and mesos-tests, then repeat and see no > compilation take place a second time. Also support\windows-build.bat twice. > > > Thanks, > > Andrew Schwartzmeyer > >
