Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Joseph Wu]

> On March 29, 2017, 12:30 p.m., Andrew Schwartzmeyer wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 38 (patched)
> > 
> >
> > Are we going to enable verbose Cotire logging? It's useful.
> 
> Andrew Schwartzmeyer wrote:
> (Or note for users that `-DCOTIRE_VERBOSE=1` does the trick.

I'll add `set(COTIRE_VERBOSE ${VERBOSE})`


> On March 29, 2017, 12:30 p.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 541 (patched)
> > 
> >
> > Can we add a comment explaining why these specific files are excluded? 
> > (As the reasons are various.)

I can add this before committing.


> On March 29, 2017, 12:30 p.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 549 (patched)
> > 
> >
> > Can there be a comment explaining why we use this particular header 
> > path versus alternatives?

I can add this before committing.

(Basically, we mostly care about the agent on Windows, so the sources come from 
the agent.)


> On March 29, 2017, 12:30 p.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 551 (patched)
> > 
> >
> > When are we going to cotire other targets too?

This can be done on a target-by-target basis.  Libmesos is the biggest chunk 
though, so who knows?


- Joseph


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


On March 28, 2017, 5:56 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 28, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Andrew Schwartzmeyer]

> On March 29, 2017, 7:30 p.m., Andrew Schwartzmeyer wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 38 (patched)
> > 
> >
> > Are we going to enable verbose Cotire logging? It's useful.

(Or note for users that `-DCOTIRE_VERBOSE=1` does the trick.


- Andrew


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


On March 29, 2017, 12:56 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 29, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Andrew Schwartzmeyer]

> On March 29, 2017, 6:54 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 39-40 (patched)
> > 
> >
> > I'll add a comment explaining unity builds:
> > ```
> >   # By default Cotire generates both precompiled headers and a "unity" 
> > build.
> >   # A unity build is where all the source files in a target are 
> > combined into
> >   # a single source file to reduce the number of files that need to be 
> > opened
> >   # and read. We disable "unity" builds for now.
> > ```
> 
> Andrew Schwartzmeyer wrote:
> I'd also add why we don't use Unity builds (they're a terrible idea that 
> should never be used haha).
> 
> Joseph Wu wrote:
> Hmm...  I don't have a good reason for this, other than "the build will 
> probably OOM if we enable this" :)

(And it doesn't work at all! Too many collisions.)


- Andrew


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


On March 29, 2017, 12:56 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 29, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Joseph Wu]

> On March 29, 2017, 11:54 a.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 34-36 (patched)
> > 
> >
> > I think we can remove this check, as it would block people that want to 
> > fix the compilation issues on non-Windows + PCHs.
> > 
> > For now, a WARNING should be sufficient.
> 
> Andrew Schwartzmeyer wrote:
> Sure, but a dev fixing compilation issues would also need to disable the 
> warning when it's supported. What I'm saying is, they'll see it either way 
> and have to change it too. A `FATAL_ERROR` seems safer to keep people from 
> complaining.

Sounds reasonable.  I'll open and reference this issue to track non-Windows 
PCHs: https://issues.apache.org/jira/browse/MESOS-7322


> On March 29, 2017, 11:54 a.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 39-40 (patched)
> > 
> >
> > I'll add a comment explaining unity builds:
> > ```
> >   # By default Cotire generates both precompiled headers and a "unity" 
> > build.
> >   # A unity build is where all the source files in a target are 
> > combined into
> >   # a single source file to reduce the number of files that need to be 
> > opened
> >   # and read. We disable "unity" builds for now.
> > ```
> 
> Andrew Schwartzmeyer wrote:
> I'd also add why we don't use Unity builds (they're a terrible idea that 
> should never be used haha).

Hmm...  I don't have a good reason for this, other than "the build will 
probably OOM if we enable this" :)


- Joseph


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


On March 28, 2017, 5:56 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 28, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Joseph Wu]

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




src/CMakeLists.txt
Lines 545 (patched)


Looks like this file also introduces some nasty namespace conflicts, so 
I'll exclude it before committing:
```

"${CMAKE_SOURCE_DIR}/include/mesos/authentication/http/combined_authenticator.hpp"
```


- Joseph Wu


On March 28, 2017, 5:56 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 28, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Andrew Schwartzmeyer]

> On March 29, 2017, 6:54 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 32 (patched)
> > 
> >
> > You need to surround `WIN32` with `${}`.  Otherwise, this will default 
> > to `OFF` on Windows.
> > 
> > You probably didn't catch this because CMake caches options.  If you 
> > call `cmake .. -DENABLE_PRECOMPILED_HEADERS=1` in the past, then all future 
> > builds will use PCHs.  The default values for options only take effect if 
> > you delete the `CMakeCache.txt` file.

I remember saying we need to be sure to test this ;). Looking around the file 
it's not clear to me if `${}` is required only in strings i.e. `"${VAR}"` or 
everywhere (since there are certainly naked variables in use without the 
braces).

Edit: I double checked [syntax 
rules](https://cmake.org/Wiki/CMake/Language_Syntax). `${x}` is required for 
substitution; otherwise definition is tested (is it set or unset, but not what 
it is). Annoying, but now I know.


> On March 29, 2017, 6:54 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 34-36 (patched)
> > 
> >
> > I think we can remove this check, as it would block people that want to 
> > fix the compilation issues on non-Windows + PCHs.
> > 
> > For now, a WARNING should be sufficient.

Sure, but a dev fixing compilation issues would also need to disable the 
warning when it's supported. What I'm saying is, they'll see it either way and 
have to change it too. A `FATAL_ERROR` seems safer to keep people from 
complaining.


> On March 29, 2017, 6:54 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 39-40 (patched)
> > 
> >
> > I'll add a comment explaining unity builds:
> > ```
> >   # By default Cotire generates both precompiled headers and a "unity" 
> > build.
> >   # A unity build is where all the source files in a target are 
> > combined into
> >   # a single source file to reduce the number of files that need to be 
> > opened
> >   # and read. We disable "unity" builds for now.
> > ```

I'd also add why we don't use Unity builds (they're a terrible idea that should 
never be used haha).


- Andrew


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


On March 29, 2017, 12:56 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 29, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Andrew Schwartzmeyer]

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




cmake/CompilationConfigure.cmake
Lines 33 (patched)


Nit-pick but we should match tense with the other options (i.e. "enable" 
versus "enabled").



cmake/CompilationConfigure.cmake
Lines 38 (patched)


Are we going to enable verbose Cotire logging? It's useful.



src/CMakeLists.txt
Lines 541 (patched)


Can we add a comment explaining why these specific files are excluded? (As 
the reasons are various.)



src/CMakeLists.txt
Lines 549 (patched)


Can there be a comment explaining why we use this particular header path 
versus alternatives?



src/CMakeLists.txt
Lines 551 (patched)


When are we going to cotire other targets too?


- Andrew Schwartzmeyer


On March 29, 2017, 12:56 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 29, 2017, 12:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Joseph Wu]

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


Fix it, then Ship it!




Only minor issues, so I'll tweak them before testing and committing.


cmake/CompilationConfigure.cmake
Lines 32 (patched)


You need to surround `WIN32` with `${}`.  Otherwise, this will default to 
`OFF` on Windows.

You probably didn't catch this because CMake caches options.  If you call 
`cmake .. -DENABLE_PRECOMPILED_HEADERS=1` in the past, then all future builds 
will use PCHs.  The default values for options only take effect if you delete 
the `CMakeCache.txt` file.



cmake/CompilationConfigure.cmake
Lines 34-36 (patched)


I think we can remove this check, as it would block people that want to fix 
the compilation issues on non-Windows + PCHs.

For now, a WARNING should be sufficient.



cmake/CompilationConfigure.cmake
Lines 39-40 (patched)


I'll add a comment explaining unity builds:
```
  # By default Cotire generates both precompiled headers and a "unity" 
build.
  # A unity build is where all the source files in a target are combined 
into
  # a single source file to reduce the number of files that need to be 
opened
  # and read. We disable "unity" builds for now.
```


- Joseph Wu


On March 28, 2017, 5:56 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58014/
> ---
> 
> (Updated March 28, 2017, 5:56 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7226
> https://issues.apache.org/jira/browse/MESOS-7226
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sets several cotire variables in the 'src' directory to enable
> pre-compiled headers for the 'mesos-agent' target. The excluded
> headers were removed due to namespace issues or breaking incremental
> builds.
> 
> All cotire variables are conditionally set if option
> ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
> WIN32 builds, off for all others.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
>   src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 
> 
> 
> Diff: https://reviews.apache.org/r/58014/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done at end of chain
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Review Request 58014: CMake: Setup cotire for 'mesos-agent' target.

[Jeff Coffler]

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

Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


Bugs: MESOS-7226
https://issues.apache.org/jira/browse/MESOS-7226


Repository: mesos


Description
---

This sets several cotire variables in the 'src' directory to enable
pre-compiled headers for the 'mesos-agent' target. The excluded
headers were removed due to namespace issues or breaking incremental
builds.

All cotire variables are conditionally set if option
ENABLE_PRECOMPILED_HEADERS is set. In this commit, this is ON for
WIN32 builds, off for all others.


Diffs
-

  cmake/CompilationConfigure.cmake 5936be07557395ad08b06aa376b5a29f9b11c143 
  src/CMakeLists.txt b67b512cafd90558abf712a872310ab913fa38ae 


Diff: https://reviews.apache.org/r/58014/diff/1/


Testing
---

Testing done at end of chain


Thanks,

Jeff Coffler