mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Srinivas Brahmaroutu <srbra...@us.ibm.com>
Subject Re: Review Request 49688: Added cmake build variables for mesos tests.
Date Tue, 19 Jul 2016 21:35:21 GMT


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > cmake/MesosConfigure.cmake, lines 96-97
> > <https://reviews.apache.org/r/49688/diff/5/?file=1441337#file1441337line96>
> >
> >     Hmm, this seems like it should break the build. We're defining these scripts
in `MesosConfigure.cmake` before we define `MESOS_TARGET`, yet `CONTAINERIZER_TEST_LIBS` lists
`MESOS_TARGET` as a dependency, no? So we should fail to emit flags that link to libmesos.
> >     
> >     Am I missing something obvious?

Hmm, not sure, the build worked fine even with clean builds.


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 21
> > <https://reviews.apache.org/r/49688/diff/5/?file=1441338#file1441338line21>
> >
> >     It looks like at least some of the following tests are missing, and I don't
see them grep'ing the codebase. Rough guess: we should be including these in the relevant
test subdirectories? If this is the rationale, can we please add them all now? If this is
not the rationale, can we please leave a comment explaining?
> >     
> >     
> >     ```
> >       tests/common/command_utils_tests.cpp				\
> >       tests/common/http_tests.cpp					\
> >       tests/common/recordio_tests.cpp				\
> >       tests/containerizer/composing_containerizer_tests.cpp		\
> >       tests/containerizer/docker_containerizer_tests.cpp		\
> >       tests/containerizer/docker_spec_tests.cpp			\
> >       tests/containerizer/docker_tests.cpp				\
> >       tests/containerizer/external_containerizer_test.cpp		\
> >       tests/containerizer/isolator_tests.cpp			\
> >       tests/containerizer/launcher.cpp				\
> >       tests/containerizer/memory_test_helper.cpp			\
> >       tests/containerizer/mesos_containerizer_tests.cpp		\
> >       tests/containerizer/provisioner_appc_tests.cpp		\
> >       tests/containerizer/provisioner_backend_tests.cpp		\
> >       tests/containerizer/provisioner_docker_tests.cpp
> >     ```
> >     
> >     Also, though not a test, the following file also seems to be not listed here:
> >     
> >     ```
> >     slave/qos_controllers/load.cpp
> >     ```

I am a bit undecided to see if all the code under test be pulled into one CMakeLists script.
I think common files should make their way here but containerizer also has a main routine.
I need clarification how we handle multiple 'main' routines in a project? 
slave/qos_controllers should end up in a library right?


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 35
> > <https://reviews.apache.org/r/49688/diff/5/?file=1441338#file1441338line35>
> >
> >     I don't think we have a `HAS_JAVA` option, do we? Does it make sense to add
an `option` in the root `CMakeLists.txt`, in the same way we have for the `VERBOSE`, `REBUNDLED`,
and `ENABLE_LIBEVENT` options?
> >     
> >     (If you don't know, an `option` exposes a binary choice to the CMake command
line, so you can do something like `cmake .. -DENABLE_LIBEVENT=1`, which would compile Mesos
with libevent.)

I think, conditional builds for Java and Python should be enabled at some point in time, are
you planning to do so?


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 43
> > <https://reviews.apache.org/r/49688/diff/5/?file=1441338#file1441338line43>
> >
> >     Double space here intentional?

Not intentional. I though each 'set' should be separated for readability. Fixed it.


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 71
> > <https://reviews.apache.org/r/49688/diff/5/?file=1441338#file1441338line71>
> >
> >     Can we please use the Mesos style TODO format? Specifically:
> >     
> >     * add semicolons after the `TODO(xxx):`
> >     * capitalize the first letter of the message
> >     * end message with a period
> >     * indent to line up with the rest of the block (i.e., put 2 spaces between the
start of the line and the `#` character)
> >     
> >     In this case, it should look something like:
> >     
> >     ```
> >     TODO(srbrahma): Needs leveldb to compile.
> >     ```
> >     
> >     Also would be good to have a bug # that corresponds to the issue.
> 
> Alex Clemmer wrote:
>     Actually, it looks like all of these `TODO`s are not quite correct -- we do support
leveldb build, and we don't need anything else except to build the qos controller that I mentioned
earlier. These can all be safely uncommented as soon as you add that to the build.
>     
>     Please do keep in mind the comment about the `TODO` formatting, though.
> 
> Alex Clemmer wrote:
>     That said, I do believe you will have to add `state/leveldb.cpp` to the build before
that works.
> 
> Alex Clemmer wrote:
>     (Also, don't forget that `ldcache_tests.cpp` is Linux-specific.)

I think leveldb support required for tests need to enabled to run some tests. I think as we
handle piece meal we can get there. I started of now with all tests commented out. This way
dependencies are unknown. As I enable the tests I will fix the dependencies. I hope this works?
I have taken out my comments for now but when ever I add comments I make sure that they adhre
to your suggestion.


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/cmake/TestsConfigure.cmake, line 17
> > <https://reviews.apache.org/r/49688/diff/5/?file=1441339#file1441339line17>
> >
> >     Though this file will likely go away, please in the future be wary of double-`include`ing
config files.

Reason is that I did not want to touch the MesosTestsConfigure yet. I eventually add everything
back into TestsConfigure( I believe there is no need for prefix 'Mesos')


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/cmake/TestsConfigure.cmake, line 1
> > <https://reviews.apache.org/r/49688/diff/5/?file=1441339#file1441339line1>
> >
> >     Just looking at this file, it seems that this should probably be combined with
`MesosTestsConfigure.cmake`. Thoughts? It might be structured as follow:
> >     
> >     ```
> >     set(MESOS_TESTS_DEPENDENCIES
> >       ${MESOS_TESTS_DEPENDENCIES}
> >       ${MESOS_DEPENDENCIES}
> >       ${MESOS_TARGET}
> >       ${GMOCK_TARGET}
> >       )
> >     
> >     set(CONTAINERIZER_TEST_DEPENDENCIES
> >       ${CONTAINERIZER_TEST_DEPENDENCIES}
> >       ${MESOS_TESTS_DEPENDENCIES}
> >       )
> >     ```
> >     
> >     i.e., we could take the relevant rules and put them next to each other, and
make the relevant containerizer variables just depend on the mesos test variables. NOTE: you
will probably have to uncomment the `GMOCK_LFLAG` definition in `MesosTestsConfigure.cmake`
in order to combine the code in this way.
> >     
> >     (And if that `MesosTestsConfigure.cmake` seems like a mess, the truth is that
file is a pile of technical debt that shoudl be cleaned up soon, by me, since I'm the one
who created it in the big death march to Mesos v1. But, for now, these variables do fit in
there, at least IMO.)

Doing everything in one scoop will make this very large patch. I will work on 'common' and
'containerizer' when needed.


- Srinivas


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


On July 16, 2016, 8:33 p.m., Srinivas Brahmaroutu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49688/
> -----------------------------------------------------------
> 
> (Updated July 16, 2016, 8:33 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5792
>     https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added cmake build variables for mesos tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/cmake/MesosTestsConfigure.cmake caecce14ca884dcc09ae4ba7649a09f7ae7c1fdf

>   src/tests/containerizer/CMakeLists.txt 41e792a2c9ec588d4897d60d012e67c606bbe601 
> 
> Diff: https://reviews.apache.org/r/49688/diff/
> 
> 
> Testing
> -------
> 
> cmake ..
> cmake check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>


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