CMake: Compile in OSMesa on Linux, if found.

CMake: Compile in OSMesa on Linux, if found.

CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Mac10.9-Clang-x86_64-Release-CMake-Trybot,Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot

Committed: https://skia.googlesource.com/skia/+/4ae26258ef1705faa303eb9eb6b779695b9143f8

[hal.canary, 2015-09-21 15:17:54]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-21 15:18:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1353383003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1353383003/1

[hal.canary, 2015-09-21 15:18:15]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-09-21 15:23:45]

halcanary@google.com changed reviewers:
+ mtklein@google.com

[hal.canary, 2015-09-21 15:23:45]

PTAL

[mtklein, 2015-09-21 15:49:37]

https://codereview.chromium.org/1353383003/diff/20001/cmake/CMakeLists.txt
File cmake/CMakeLists.txt (right):

https://codereview.chromium.org/1353383003/diff/20001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:201: if (UNIX AND NOT APPLE AND EXISTS
"/usr/include/GL/osmesa.h")
Can you not do this like one of the other dependencies, either using
find_package (many examples) or find_path/find_library (webp, fontconfig)?

[mtklein, 2015-09-21 15:51:56]

https://codereview.chromium.org/1353383003/diff/40001/cmake/CMakeLists.txt
File cmake/CMakeLists.txt (right):

https://codereview.chromium.org/1353383003/diff/40001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:218: target_compile_definitions(skia
Even if this works, I'd prefer it if you switch things over to appending to
${public_defines} and ${private_defines}, and call target_compile_definitions
exactly once.

[hal.canary, 2015-09-21 16:10:59]

Patchset #4 (id:60001) has been deleted

[mtklein_C, 2015-09-21 16:14:39]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2015-09-21 16:14:39]

lgtm with some nits

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt
File cmake/CMakeLists.txt (right):

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:201: list (APPEND private_defines "-DSKIA_DLL")
As a nit, let's move this out of where all the deps are found.  Maybe, line ~217
just above the target_compile_definitions call?

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:203: find_library(OSMESA_LIBRARIES "OSMesa")
Let's put the find_* together with the others above for consistency?

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:206: list (APPEND libs  ${OSMESA_LIBRARIES})
Add OSMESA_INCLUDE_DIRS to private_includes?

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:208: message("-- Found OSMesa: " ${OSMESA_LIBRARIES})
Remove this.  It'll print anyway

[hal.canary, 2015-09-21 16:35:20]

Patchset #5 (id:100001) has been deleted

[hal.canary, 2015-09-21 16:35:29]

Patchset #5 (id:120001) has been deleted

[hal.canary, 2015-09-21 16:35:58]

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt
File cmake/CMakeLists.txt (right):

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:201: list (APPEND private_defines "-DSKIA_DLL")
On 2015/09/21 16:14:39, mtklein_C wrote:
> As a nit, let's move this out of where all the deps are found.  Maybe, line
~217
> just above the target_compile_definitions call?

Done.

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:203: find_library(OSMESA_LIBRARIES "OSMesa")
On 2015/09/21 16:14:39, mtklein_C wrote:
> Let's put the find_* together with the others above for consistency?

Done.

https://codereview.chromium.org/1353383003/diff/80001/cmake/CMakeLists.txt#ne...
cmake/CMakeLists.txt:206: list (APPEND libs  ${OSMESA_LIBRARIES})
On 2015/09/21 16:14:39, mtklein_C wrote:
> Add OSMESA_INCLUDE_DIRS to private_includes?

It was just /usr/include;  but no harm.  done.

[hal.canary, 2015-09-21 16:37:23]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-09-21 16:37:24]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org
Link to the patchset: https://codereview.chromium.org/1353383003/#ps140001
(title: "2015-09-21 (Monday) 12:34:55 EDT")

[commit-bot: I haz the power, 2015-09-21 16:37:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1353383003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1353383003/140001

[commit-bot: I haz the power, 2015-09-21 16:42:27]

Committed patchset #5 (id:140001) as
https://skia.googlesource.com/skia/+/4ae26258ef1705faa303eb9eb6b779695b9143f8