You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2017/08/18 21:10:45 UTC
Review Request 61753: Refactored CMake build system.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61753/
-----------------------------------------------------------
Review request for mesos and Joseph Wu.
Repository: mesos
Description
-------
This commit deletes extraneous CMake code. With a proper CMake
dependency graph, CMake automatically calculates the correct compiler
and linker options based on the traversal of the graph, and the
properties of each target. With the correct importing of the 3rdparty
dependencies, this allows us to remove all uses of deprecated CMake
commands such as `include_directories`, `link_directories`, and
`link_libraries`. These commands rely on setting directory-wide
side-effects, unlike their explicit `target_include_directories`
counterparts, which instead add the information to the dependency graph.
By utilizing CMake's dependency graph, most uses of `add_dependencies`
were removed, as an implicit dependency is created when
`target_link_libraries` is used. Note that there are explicit cases of
manually dependencies, such as module libraries which are not linked and
therefore are not automatically added to the graph.
Furthermore, superfluous variables were removed. Due to how the original
system was written (based on copying a working template), many variables
were introduced that contained single items, such that the declaration
and interpolation of the variable became far more verbose than simply
using the contents directly. Variables holding target names were also
removed, as they were redundant. See the changes to
`src/master/CMakeLists.txt` for an example of an executable's build
going from twelve logical lines of code to two.
"Configuration" files such as `AgentConfigure.cmake`, were deleted in
their entirety, because the information they provided was redundant when
using the CMake dependency graph. The only CMake configuration files
remaining are for actual project configuration.
Diffs
-----
CMakeLists.txt 870bd9ba2d5edfe31092d602135edd28dc4982e0
cmake/CompilationConfigure.cmake 2109697e02e58c673948f2ae67ef8255d16aa736
cmake/MesosConfigure.cmake 5ecad2c0fbd245dd14e9c09869dd6da2ca4e602a
src/CMakeLists.txt 0816c6ebaa9caaa5b6a606e601427ed17112a300
src/checks/CMakeLists.txt 4854b2765202e10f5b791f08ed1416f7740e2aad
src/cli/CMakeLists.txt 056d1d734e4424dac3eb29c323961d3a2e87a8dc
src/docker/CMakeLists.txt 2d7c30db5854853625a99227568a27dde281ec2e
src/examples/CMakeLists.txt e8b7ea4bd91c20536ce7bd8d0ffdfd4e549b056f
src/examples/cmake/ExamplesConfigure.cmake d85da4e2e236df9408c86405caeca4d8eae3a35d
src/launcher/CMakeLists.txt 97edc4a28a995da8a5e963aaa62c4c955cc59719
src/local/CMakeLists.txt 7c9a718d17772be1c08e132136739cfd9449470d
src/log/CMakeLists.txt 2fad6cca467dbde9989d26424a2a6c608cd251e8
src/master/CMakeLists.txt 0062fb3e9d67fdfcda1057112d4ba34abbd4be5f
src/master/cmake/MasterConfigure.cmake 173dc36b5a1b7ff23cd0542ea4313ae6e5e0ae3f
src/slave/CMakeLists.txt 9dbadca53b9ac34db08cfb9c9e9fea741c01114b
src/slave/cmake/AgentConfigure.cmake af2f74f82b68920427f36c0c863b97c0191b8f06
src/slave/container_loggers/CMakeLists.txt c0774ce3110fd8d2aac6dc00d2b800e336214f96
src/slave/containerizer/mesos/CMakeLists.txt fa2043efadd1c863c5ad54fdc518397c97488dcb
src/slave/qos_controllers/CMakeLists.txt 65ab338a71276a77e43af962fbc9b76e050efca6
src/slave/resource_estimators/CMakeLists.txt 1d28bd457c7e567219d892f0940903b12c5a877b
src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
src/tests/cmake/MesosTestsConfigure.cmake 48b4d2fd74b545c03162c7cb06013baf67298241
src/usage/CMakeLists.txt fc87b297832157b679039c68baf9ddc68a292060
Diff: https://reviews.apache.org/r/61753/diff/1/
Testing
-------
Tested with `make check` and `ninja-build check` on CentOS 7. All tests pass. Tested with libevent and OpenSSL, all tests passed. Tested with Java, all tests passed (the "example" Java tests which depend on a target that's not yet enabled).
Tested on Windows with and without Java, all tests passed. With Java requires some environment setup which will be documented.
Tested release configurations, all tests pass.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 61753: Refactored CMake build system.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61753/#review184465
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On Aug. 18, 2017, 2:10 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61753/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 2:10 p.m.)
>
>
> Review request for mesos and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit deletes extraneous CMake code. With a proper CMake
> dependency graph, CMake automatically calculates the correct compiler
> and linker options based on the traversal of the graph, and the
> properties of each target. With the correct importing of the 3rdparty
> dependencies, this allows us to remove all uses of deprecated CMake
> commands such as `include_directories`, `link_directories`, and
> `link_libraries`. These commands rely on setting directory-wide
> side-effects, unlike their explicit `target_include_directories`
> counterparts, which instead add the information to the dependency graph.
> By utilizing CMake's dependency graph, most uses of `add_dependencies`
> were removed, as an implicit dependency is created when
> `target_link_libraries` is used. Note that there are explicit cases of
> manually dependencies, such as module libraries which are not linked and
> therefore are not automatically added to the graph.
>
> Furthermore, superfluous variables were removed. Due to how the original
> system was written (based on copying a working template), many variables
> were introduced that contained single items, such that the declaration
> and interpolation of the variable became far more verbose than simply
> using the contents directly. Variables holding target names were also
> removed, as they were redundant. See the changes to
> `src/master/CMakeLists.txt` for an example of an executable's build
> going from twelve logical lines of code to two.
>
> "Configuration" files such as `AgentConfigure.cmake`, were deleted in
> their entirety, because the information they provided was redundant when
> using the CMake dependency graph. The only CMake configuration files
> remaining are for actual project configuration.
>
>
> Diffs
> -----
>
> CMakeLists.txt 870bd9ba2d5edfe31092d602135edd28dc4982e0
> cmake/CompilationConfigure.cmake 2109697e02e58c673948f2ae67ef8255d16aa736
> cmake/MesosConfigure.cmake 5ecad2c0fbd245dd14e9c09869dd6da2ca4e602a
> src/CMakeLists.txt 0816c6ebaa9caaa5b6a606e601427ed17112a300
> src/checks/CMakeLists.txt 4854b2765202e10f5b791f08ed1416f7740e2aad
> src/cli/CMakeLists.txt 056d1d734e4424dac3eb29c323961d3a2e87a8dc
> src/docker/CMakeLists.txt 2d7c30db5854853625a99227568a27dde281ec2e
> src/examples/CMakeLists.txt e8b7ea4bd91c20536ce7bd8d0ffdfd4e549b056f
> src/examples/cmake/ExamplesConfigure.cmake d85da4e2e236df9408c86405caeca4d8eae3a35d
> src/launcher/CMakeLists.txt 97edc4a28a995da8a5e963aaa62c4c955cc59719
> src/local/CMakeLists.txt 7c9a718d17772be1c08e132136739cfd9449470d
> src/log/CMakeLists.txt 2fad6cca467dbde9989d26424a2a6c608cd251e8
> src/master/CMakeLists.txt 0062fb3e9d67fdfcda1057112d4ba34abbd4be5f
> src/master/cmake/MasterConfigure.cmake 173dc36b5a1b7ff23cd0542ea4313ae6e5e0ae3f
> src/slave/CMakeLists.txt 9dbadca53b9ac34db08cfb9c9e9fea741c01114b
> src/slave/cmake/AgentConfigure.cmake af2f74f82b68920427f36c0c863b97c0191b8f06
> src/slave/container_loggers/CMakeLists.txt c0774ce3110fd8d2aac6dc00d2b800e336214f96
> src/slave/containerizer/mesos/CMakeLists.txt fa2043efadd1c863c5ad54fdc518397c97488dcb
> src/slave/qos_controllers/CMakeLists.txt 65ab338a71276a77e43af962fbc9b76e050efca6
> src/slave/resource_estimators/CMakeLists.txt 1d28bd457c7e567219d892f0940903b12c5a877b
> src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
> src/tests/cmake/MesosTestsConfigure.cmake 48b4d2fd74b545c03162c7cb06013baf67298241
> src/usage/CMakeLists.txt fc87b297832157b679039c68baf9ddc68a292060
>
>
> Diff: https://reviews.apache.org/r/61753/diff/1/
>
>
> Testing
> -------
>
> Tested with `make check` and `ninja-build check` on CentOS 7. All tests pass. Tested with libevent and OpenSSL, all tests passed. Tested with Java, all tests passed (the "example" Java tests which depend on a target that's not yet enabled).
>
> Tested on Windows with and without Java, all tests passed. With Java requires some environment setup which will be documented.
>
> Tested release configurations, all tests pass.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 61753: Refactored CMake build system.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61753/#review183264
-----------------------------------------------------------
Bad review!
Reviews applied: [61753, 61752, 61751, 61750, 61597, 61516, 61515, 61514, 61513, 61512, 61365]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot Windows
On Aug. 18, 2017, 9:10 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61753/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 9:10 p.m.)
>
>
> Review request for mesos and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit deletes extraneous CMake code. With a proper CMake
> dependency graph, CMake automatically calculates the correct compiler
> and linker options based on the traversal of the graph, and the
> properties of each target. With the correct importing of the 3rdparty
> dependencies, this allows us to remove all uses of deprecated CMake
> commands such as `include_directories`, `link_directories`, and
> `link_libraries`. These commands rely on setting directory-wide
> side-effects, unlike their explicit `target_include_directories`
> counterparts, which instead add the information to the dependency graph.
> By utilizing CMake's dependency graph, most uses of `add_dependencies`
> were removed, as an implicit dependency is created when
> `target_link_libraries` is used. Note that there are explicit cases of
> manually dependencies, such as module libraries which are not linked and
> therefore are not automatically added to the graph.
>
> Furthermore, superfluous variables were removed. Due to how the original
> system was written (based on copying a working template), many variables
> were introduced that contained single items, such that the declaration
> and interpolation of the variable became far more verbose than simply
> using the contents directly. Variables holding target names were also
> removed, as they were redundant. See the changes to
> `src/master/CMakeLists.txt` for an example of an executable's build
> going from twelve logical lines of code to two.
>
> "Configuration" files such as `AgentConfigure.cmake`, were deleted in
> their entirety, because the information they provided was redundant when
> using the CMake dependency graph. The only CMake configuration files
> remaining are for actual project configuration.
>
>
> Diffs
> -----
>
> CMakeLists.txt 870bd9ba2d5edfe31092d602135edd28dc4982e0
> cmake/CompilationConfigure.cmake 2109697e02e58c673948f2ae67ef8255d16aa736
> cmake/MesosConfigure.cmake 5ecad2c0fbd245dd14e9c09869dd6da2ca4e602a
> src/CMakeLists.txt 0816c6ebaa9caaa5b6a606e601427ed17112a300
> src/checks/CMakeLists.txt 4854b2765202e10f5b791f08ed1416f7740e2aad
> src/cli/CMakeLists.txt 056d1d734e4424dac3eb29c323961d3a2e87a8dc
> src/docker/CMakeLists.txt 2d7c30db5854853625a99227568a27dde281ec2e
> src/examples/CMakeLists.txt e8b7ea4bd91c20536ce7bd8d0ffdfd4e549b056f
> src/examples/cmake/ExamplesConfigure.cmake d85da4e2e236df9408c86405caeca4d8eae3a35d
> src/launcher/CMakeLists.txt 97edc4a28a995da8a5e963aaa62c4c955cc59719
> src/local/CMakeLists.txt 7c9a718d17772be1c08e132136739cfd9449470d
> src/log/CMakeLists.txt 2fad6cca467dbde9989d26424a2a6c608cd251e8
> src/master/CMakeLists.txt 0062fb3e9d67fdfcda1057112d4ba34abbd4be5f
> src/master/cmake/MasterConfigure.cmake 173dc36b5a1b7ff23cd0542ea4313ae6e5e0ae3f
> src/slave/CMakeLists.txt 9dbadca53b9ac34db08cfb9c9e9fea741c01114b
> src/slave/cmake/AgentConfigure.cmake af2f74f82b68920427f36c0c863b97c0191b8f06
> src/slave/container_loggers/CMakeLists.txt c0774ce3110fd8d2aac6dc00d2b800e336214f96
> src/slave/containerizer/mesos/CMakeLists.txt fa2043efadd1c863c5ad54fdc518397c97488dcb
> src/slave/qos_controllers/CMakeLists.txt 65ab338a71276a77e43af962fbc9b76e050efca6
> src/slave/resource_estimators/CMakeLists.txt 1d28bd457c7e567219d892f0940903b12c5a877b
> src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
> src/tests/cmake/MesosTestsConfigure.cmake 48b4d2fd74b545c03162c7cb06013baf67298241
> src/usage/CMakeLists.txt fc87b297832157b679039c68baf9ddc68a292060
>
>
> Diff: https://reviews.apache.org/r/61753/diff/1/
>
>
> Testing
> -------
>
> Tested with `make check` and `ninja-build check` on CentOS 7. All tests pass. Tested with libevent and OpenSSL, all tests passed. Tested with Java, all tests passed (the "example" Java tests which depend on a target that's not yet enabled).
>
> Tested on Windows with and without Java, all tests passed. With Java requires some environment setup which will be documented.
>
> Tested release configurations, all tests pass.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 61753: Refactored CMake build system.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61753/#review183293
-----------------------------------------------------------
Bad review!
Reviews applied: [61753, 61752, 61751, 61750, 61597, 61516, 61515, 61514, 61513, 61512, 61365]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot
On Aug. 18, 2017, 2:10 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61753/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 2:10 p.m.)
>
>
> Review request for mesos and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit deletes extraneous CMake code. With a proper CMake
> dependency graph, CMake automatically calculates the correct compiler
> and linker options based on the traversal of the graph, and the
> properties of each target. With the correct importing of the 3rdparty
> dependencies, this allows us to remove all uses of deprecated CMake
> commands such as `include_directories`, `link_directories`, and
> `link_libraries`. These commands rely on setting directory-wide
> side-effects, unlike their explicit `target_include_directories`
> counterparts, which instead add the information to the dependency graph.
> By utilizing CMake's dependency graph, most uses of `add_dependencies`
> were removed, as an implicit dependency is created when
> `target_link_libraries` is used. Note that there are explicit cases of
> manually dependencies, such as module libraries which are not linked and
> therefore are not automatically added to the graph.
>
> Furthermore, superfluous variables were removed. Due to how the original
> system was written (based on copying a working template), many variables
> were introduced that contained single items, such that the declaration
> and interpolation of the variable became far more verbose than simply
> using the contents directly. Variables holding target names were also
> removed, as they were redundant. See the changes to
> `src/master/CMakeLists.txt` for an example of an executable's build
> going from twelve logical lines of code to two.
>
> "Configuration" files such as `AgentConfigure.cmake`, were deleted in
> their entirety, because the information they provided was redundant when
> using the CMake dependency graph. The only CMake configuration files
> remaining are for actual project configuration.
>
>
> Diffs
> -----
>
> CMakeLists.txt 870bd9ba2d5edfe31092d602135edd28dc4982e0
> cmake/CompilationConfigure.cmake 2109697e02e58c673948f2ae67ef8255d16aa736
> cmake/MesosConfigure.cmake 5ecad2c0fbd245dd14e9c09869dd6da2ca4e602a
> src/CMakeLists.txt 0816c6ebaa9caaa5b6a606e601427ed17112a300
> src/checks/CMakeLists.txt 4854b2765202e10f5b791f08ed1416f7740e2aad
> src/cli/CMakeLists.txt 056d1d734e4424dac3eb29c323961d3a2e87a8dc
> src/docker/CMakeLists.txt 2d7c30db5854853625a99227568a27dde281ec2e
> src/examples/CMakeLists.txt e8b7ea4bd91c20536ce7bd8d0ffdfd4e549b056f
> src/examples/cmake/ExamplesConfigure.cmake d85da4e2e236df9408c86405caeca4d8eae3a35d
> src/launcher/CMakeLists.txt 97edc4a28a995da8a5e963aaa62c4c955cc59719
> src/local/CMakeLists.txt 7c9a718d17772be1c08e132136739cfd9449470d
> src/log/CMakeLists.txt 2fad6cca467dbde9989d26424a2a6c608cd251e8
> src/master/CMakeLists.txt 0062fb3e9d67fdfcda1057112d4ba34abbd4be5f
> src/master/cmake/MasterConfigure.cmake 173dc36b5a1b7ff23cd0542ea4313ae6e5e0ae3f
> src/slave/CMakeLists.txt 9dbadca53b9ac34db08cfb9c9e9fea741c01114b
> src/slave/cmake/AgentConfigure.cmake af2f74f82b68920427f36c0c863b97c0191b8f06
> src/slave/container_loggers/CMakeLists.txt c0774ce3110fd8d2aac6dc00d2b800e336214f96
> src/slave/containerizer/mesos/CMakeLists.txt fa2043efadd1c863c5ad54fdc518397c97488dcb
> src/slave/qos_controllers/CMakeLists.txt 65ab338a71276a77e43af962fbc9b76e050efca6
> src/slave/resource_estimators/CMakeLists.txt 1d28bd457c7e567219d892f0940903b12c5a877b
> src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e
> src/tests/cmake/MesosTestsConfigure.cmake 48b4d2fd74b545c03162c7cb06013baf67298241
> src/usage/CMakeLists.txt fc87b297832157b679039c68baf9ddc68a292060
>
>
> Diff: https://reviews.apache.org/r/61753/diff/1/
>
>
> Testing
> -------
>
> Tested with `make check` and `ninja-build check` on CentOS 7. All tests pass. Tested with libevent and OpenSSL, all tests passed. Tested with Java, all tests passed (the "example" Java tests which depend on a target that's not yet enabled).
>
> Tested on Windows with and without Java, all tests passed. With Java requires some environment setup which will be documented.
>
> Tested release configurations, all tests pass.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>