mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 57052: CMake: Add `build_config.hpp.in` for `BUILD_TIME` variables.
Date Wed, 01 Mar 2017 02:13:01 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57052/#review167198
-----------------------------------------------------------


Ship it!




I'll make the changes and ship this if you agree with the renaming (see below).


cmake/CompilationConfigure.cmake (line 291)
<https://reviews.apache.org/r/57052/#comment239367>

    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
      )
    ```



cmake/CompilationConfigure.cmake (line 297)
<https://reviews.apache.org/r/57052/#comment239369>

    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.
    ```



cmake/CompilationConfigure.cmake (line 299)
<https://reviews.apache.org/r/57052/#comment239368>

    Just for symmetry, let's put this in
    
    "${CMAKE_BINARY_DIR}/src/common/build_config.hpp"



cmake/CompilationConfigure.cmake (line 312)
<https://reviews.apache.org/r/57052/#comment239366>

    What do you feel about "USE_CMAKE_BUILD_CONFIG" instead?  "HAVE_BUILD_CONFIG_H" seems
too much like a header guard.



src/common/build.cpp (line 26)
<https://reviews.apache.org/r/57052/#comment239371>

    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.
    ```


- Joseph Wu


On Feb. 28, 2017, 4:42 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57052/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2017, 4:42 p.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/
> 
> 
> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message