New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dmclock: initial commit of dmclock QoS library #14330

Merged
merged 4 commits into from May 2, 2017

Conversation

Projects
None yet
5 participants
@ivancich
Member

ivancich commented Apr 4, 2017

This commit brings in the dmclock library, located in a separate repo, as a git subtree. It also contains a README.git-subtree to describe this technique and some of the pitfalls surrounding it. And it links the cmake files so the library and support can be built.

Make targets include "dmclock", the static library; "dmclock-tests", unit tests for the dmclock code and supporting data structures; and "dmclock-sims", simulations that can be used to test and understand the library.

@ivancich ivancich requested review from liewegas and jdurgin Apr 4, 2017

@ivancich

This comment has been minimized.

Member

ivancich commented Apr 4, 2017

Hmm, the two commits created by the "git subtree pull ..." command are not signed, and there do not appear to be command line options to add a signature.

@liewegas liewegas added the build/ops label Apr 4, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 4, 2017

This looks good to me... @jdurgin ?

@jdurgin

This comment has been minimized.

Member

jdurgin commented Apr 4, 2017

Don't worry about the signed-off-by for the subtree commits.

The 'make check' error looks real though - it got a failure from not running the dmclock tests.

@ivancich

This comment has been minimized.

Member

ivancich commented Apr 5, 2017

I'll handle the make check issue.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 7, 2017

retest this please

if(WITH_TESTS)
install(PROGRAMS
${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/dmclock-tests

This comment has been minimized.

@tchaikov

tchaikov Apr 7, 2017

Contributor

might want to include this in "ceph-test". see debian/ceph-test.install and ceph.spec.in (search for %files -n ceph-test)

This comment has been minimized.

@ivancich

ivancich Apr 7, 2017

Member

Will do.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 20, 2017

-- Set runtime path of "/tmp/buildd/ceph-12.0.1-1259-g49dfa5a/debian/tmp/usr/bin/ceph-mds" to "/usr/lib/ceph"
CMake Error at src/cmake_install.cmake:193 (file):
file INSTALL cannot find
"/tmp/buildd/ceph-12.0.1-1259-g49dfa5a/obj-x86_64-linux-gnu/bin/dmclock-tests".
Call Stack (most recent call first):
cmake_install.cmake:37 (include)

https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=xenial,DIST=xenial,MACHINE_SIZE=huge/2765//consoleFull

@ivancich ivancich changed the title from dmclock: initial commit of dmclock QoS library to [DNM yet] dmclock: initial commit of dmclock QoS library Apr 25, 2017

ivancich added some commits Apr 4, 2017

Add a README to explain some of the nature of git subtrees and some of
the pitfalls surrounding them.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
Squashed 'src/dmclock/' content from commit d6586d7
git-subtree-dir: src/dmclock
git-subtree-split: d6586d73679f4a1bdf335235d309e2352f0c76c6
Modify ceph cmake files to bring in dmclock library and tests. Include
dmclock tests in ceph-test for builds.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>

@ivancich ivancich changed the title from [DNM yet] dmclock: initial commit of dmclock QoS library to dmclock: initial commit of dmclock QoS library May 1, 2017

@liewegas liewegas merged commit 5f1c03e into ceph:master May 2, 2017

2 of 3 checks passed

Signed-off-by one or more commits in this PR are not signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 19, 2017

This breaks SUSE builds:

[  158s] CMake Error at src/dmclock/CMakeLists.txt:20 (find_package):
[  158s]   By not providing "Findgtest.cmake" in CMAKE_MODULE_PATH this project has
[  158s]   asked CMake to find a package configuration file provided by "gtest", but
[  158s]   CMake did not find one.
[  158s] 
[  158s]   Could not find a package configuration file provided by "gtest" with any of
[  158s]   the following names:
[  158s] 
[  158s]     gtestConfig.cmake
[  158s]     gtest-config.cmake
[  158s] 
[  158s]   Add the installation prefix of "gtest" to CMAKE_PREFIX_PATH or set
[  158s]   "gtest_DIR" to a directory containing one of the above files.  If "gtest"
[  158s]   provides a separate development package or SDK, be sure it has been
[  158s]   installed.
[  158s] 
[  158s] 
[  158s] -- Configuring incomplete, errors occurred!

smithfarm added a commit to smithfarm/ceph that referenced this pull request May 19, 2017

build/ops: fix case-sensitive find_package call
The FindGTest.cmake module is part of the upstream cmake [1] (so it should
be the same in all distros), and it is named "FindGTest.cmake", not
"Findgtest.cmake". Fix the find_package() call introduced by
ceph#14330 so the case matches.

This fixes SUSE builds broken by that PR.

[1] https://cmake.org/cmake/help/v3.0/module/FindGTest.html

Signed-off-by: Nathan Cutler <ncutler@suse.com>
@ivancich

This comment has been minimized.

Member

ivancich commented May 22, 2017

Hi Nathan, Thanks for looking into that. When ceph is built, gtest and gtest_main are targets, and I thought that would have prevented the find_package of gtest. Would you mind looking into that?

Nonetheless, I'll fix the cases to match cmake's upstream. I'll likely include that in #14997 , which is currently under eval.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 22, 2017

@ivancich in #14997 you are dropping (i.e. not packaging) the binaries dmclock-tests and dmclock-data-struct-tests, right? In that case, instead of if(WITH_TESTS) you could guard the build of these binaries with some other conditional that defaults to OFF?

I ask because WITH_TESTS=ON means "the binaries in ceph-test are to be built". It does not mean "ctest is to be run". . .

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 22, 2017

@ivancich See #10872 for how/why of WITH_TESTS

${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/dmclock-tests
${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/dmclock-data-struct-tests
DESTINATION bin)
endif(WITH_TESTS)

This comment has been minimized.

@smithfarm

smithfarm May 22, 2017

Contributor

@ivancich This is where the build of dmclock-tests and dmclock-data-struct-tests is made conditional upon WITH_TESTS.

This comment has been minimized.

@smithfarm

smithfarm May 22, 2017

Contributor

If we could change the guard to some other conditional (and not package these two binaries), then the problem I'm having will be solved I think!

This comment has been minimized.

@smithfarm

smithfarm May 22, 2017

Contributor

e.g. in CMakeLists.txt add a line that says set(WITH_DMCLOCK_TESTS "enable the build of dmclock-tests and dmclock-data-struct-tests binaries" OFF)

and then s/WITH_TESTS/WITH_DMCLOCK_TESTS/

This comment has been minimized.

@smithfarm

smithfarm May 22, 2017

Contributor

Then somebody who wanted to build these binaries could pass -DWITH_DMCLOCK_TESTS=ON to cmake, but for purposes of building RPMs and debs we would use -DWITH_DMCLOCK_TESTS=OFF.

This comment has been minimized.

@ivancich

ivancich May 22, 2017

Member

If you look at where this is heading (https://github.com/ivancich/ceph-fork/tree/wip-bring-in-dmclock-p2-gtest) all that is gone. dmclock tests won't even be built unless you do something like "make dmclock-tests". They're detached from ceph's tests. I think that'll give you what you want, no?

This comment has been minimized.

@smithfarm

smithfarm May 22, 2017

Contributor

dmclock tests won't even be built unless you do something like "make dmclock-tests". They're detached from ceph's tests. I think that'll give you what you want, no?

Yes.

This comment has been minimized.

@smithfarm
# dmClock
add_subdirectory(dmclock) # after gmock
add_dependencies(tests dmclock-tests dmclock-data-struct-tests)

This comment has been minimized.

@smithfarm

smithfarm May 22, 2017

Contributor

this line would get moved into the proposed WITH_DMCLOCK_TESTS conditional

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment