Skip to content
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

test/fio: enable objectstore FIO plugin building without the need to install and build FIO source code #20535

Merged
merged 1 commit into from Mar 6, 2018

Conversation

Projects
None yet
3 participants
@ifed01
Copy link
Contributor

commented Feb 22, 2018

By default plugin building is still off and one should provide -DWITH_FIO=ON for do_cmake call.
However run_make_check enforces the build hence providing source code validation at jenkins.

Signed-off-by: Igor Fedotov ifedotov@suse.com

@ifed01 ifed01 added the build/ops label Feb 22, 2018

@tchaikov tchaikov self-requested a review Feb 22, 2018

@liewegas liewegas requested a review from cbodley Feb 22, 2018


#ExternalProject_Get_Property(project_luajit install_dir)
# force fio make to be called on each time
ExternalProject_Add_Step(fio_ext forcebuild

This comment has been minimized.

Copy link
@cbodley

cbodley Feb 22, 2018

Contributor

i'm guessing the forcebuild step is needed because fio_ceph_objectstore uses FIO_INCLUDE_DIR without expressing an actual dependency on this external project. i think it would be preferable to have both Findfio.cmake and BuildFIO.cmake add an imported target for fio_ceph_objectstore to use instead of using FIO_INCLUDE_DIR directly

.gitmodules Outdated
@@ -52,3 +52,6 @@
[submodule "src/rapidjson"]
path = src/rapidjson
url = https://github.com/ceph/rapidjson
[submodule "src/fio"]

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 23, 2018

Contributor

i'd suggest avoid adding dependencies which is not enabled by a typical build. by "typical build", i mean the settings specified by debian/rules and ceph.spec.in. introducing new submodules increases the size of the release tarball and the overhead of our CI process.

instead, you probably could follow the example of nvml_ext, see https://github.com/ceph/ceph/blob/master/src/os/CMakeLists.txt#L123

@@ -0,0 +1,28 @@
include(CheckCXXSourceRuns)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 23, 2018

Contributor

this is not used.

ALWAYS 1)
endfunction()

macro(build_fio)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 23, 2018

Contributor

no need to have an extra macro. the reason why BuildRocksDB.cmake uses a macro build_rocksdb is to allow us to introduce new variables, like ROCKSDB_INCLUDE_DIR, ROCKSDB_LIBRARIES and ROCKSDB_VERSION_STRING to the parent scope of the macro.

@ifed01 ifed01 force-pushed the ifed01:wip-ifed-bring-fio branch from 423f445 to 9969742 Feb 28, 2018

@ifed01

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

@tchaikov, @cbodley - updated.
@cbodley - not sure I completely understand your comment about direct use of FIO_INCLUDE_DIR. Please elaborate if needed.

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

@cbodley - not sure I completely understand your comment about direct use of FIO_INCLUDE_DIR. Please elaborate if needed.

that was just to say that target_include_directories(fio_ceph_objectstore SYSTEM PUBLIC ${FIO_INCLUDE_DIR}) didn't express the dependency needed to build the external project before fio_ceph_objectstore. you added the add_dependencies(fio_ceph_objectstore fio_ext) for this, so i'm satisfied 👍

ExternalProject_Add(fio_ext
DOWNLOAD_DIR ${CMAKE_BINARY_DIR}/src/
GIT_REPOSITORY "https://github.com/axboe/fio.git"
GIT_TAG "e09d68c8da4ab91397490577454de928106651f5"

This comment has been minimized.

Copy link
@cbodley

cbodley Feb 28, 2018

Contributor

is there anything special about this commit? why not target the master branch?

This comment has been minimized.

Copy link
@ifed01

ifed01 Feb 28, 2018

Author Contributor

After considering these two options: lock the commit or bind to arbitrary selected one I choose the latter - it will prevent from occasional build failures after some changes in fio code base.

This comment has been minimized.

Copy link
@ifed01

ifed01 Mar 1, 2018

Author Contributor

I mean attaching to master instead of "lock the commit" above.

This comment has been minimized.

Copy link
@ifed01

ifed01 Mar 1, 2018

Author Contributor

Replace specific commit with a v3.5 tag though.

find_package(fio REQUIRED)
endif(WITH_FIO)
if(NOT FIO_FOUND)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 1, 2018

Contributor

i think WITH_FIO would suffice here. the reason we want HAVE_FOO is that the C/C++ source or cmake script might want to check the existence of a certain library.

This comment has been minimized.

Copy link
@ifed01

ifed01 Mar 1, 2018

Author Contributor

@tchaikov - I removed WITH_FIO totally, assuming that we always build with fio enabled. New WITH_SYSTEM_FIO has been introduced to be able to switch between system and downloaded versions.. Given that is your comment still applicable?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 1, 2018

Contributor

i am still in favor of keeping fio an option for user. as it is not always required from user's perspective. even for a ceph developer, it is optional.

This comment has been minimized.

Copy link
@ifed01

ifed01 Mar 1, 2018

Author Contributor

One of the rationales for this patch is to avoid periodic fio plugin build failures caused by incomplete code changes. Which in turn are caused by lack of automatic fio plugin building. So I'd like to have it enabled by default.
Are you OK with having an option to disable such a build rather than enable it to cover that case?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 1, 2018

Contributor

ahh, i see now. how about disabling it by default, and enabling it in run-make-check.sh? so the fio plugin feature can be built by the jenkins' "make check" run?

BINARY_DIR ${CMAKE_BINARY_DIR}/src/fio
CONFIGURE_COMMAND ${CMAKE_BINARY_DIR}/src/fio/configure
BUILD_COMMAND $(MAKE) fio EXTFLAGS=-Wno-format-truncation
COMMAND cp ${CMAKE_BINARY_DIR}/src/fio/fio ${CMAKE_BINARY_DIR}/bin

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 1, 2018

Contributor

nit, this is the INSTALL_COMMAND =)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 1, 2018

Contributor

also, could put

INSTALL_COMMAND install <BINARY_DIR>/fio ${CMAKE_BINARY_DIR}/bin

instead, to use INSTALL_COMMAND, and to use the BINARY_DIR.

GIT_REPOSITORY "https://github.com/axboe/fio.git"
GIT_TAG "e09d68c8da4ab91397490577454de928106651f5"
SOURCE_DIR ${CMAKE_BINARY_DIR}/src/fio
BINARY_DIR ${CMAKE_BINARY_DIR}/src/fio

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 1, 2018

Contributor

nit, could use BUILD_IN_SOURCE 1 to avoid repeating BINARY_DIR.

GIT_TAG "e09d68c8da4ab91397490577454de928106651f5"
SOURCE_DIR ${CMAKE_BINARY_DIR}/src/fio
BINARY_DIR ${CMAKE_BINARY_DIR}/src/fio
CONFIGURE_COMMAND ${CMAKE_BINARY_DIR}/src/fio/configure

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 1, 2018

Contributor

nit, could use <SOURCE_DIR> as a token to reference the defined SOURCE_DIR, like

CONFIGURE_COMMAND <SOURCE_DIR>/configure

@ifed01 ifed01 force-pushed the ifed01:wip-ifed-bring-fio branch 2 times, most recently from 9b34827 to c1d288c Mar 1, 2018

@ifed01 ifed01 changed the title test/fio: bring FIO as a submodule to Ceph and enable automatic objectstore FIO… test/fio: enable objectstore FIO plugin building without the need to install and build FIO source code Mar 5, 2018

@ifed01

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

@tchaikov @cbodley - updated again.

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2018

looks good to me 👍 i'll defer to @tchaikov for approval

find_package(fio REQUIRED)
endif(WITH_FIO)
if(NOT FIO_FOUND)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 5, 2018

Contributor

this if block is not necessary, as find_package(fio REQUIRED) will bail out with an error message if fio is not found. see https://cmake.org/cmake/help/v3.0/command/find_package.html#find-package

@@ -14,5 +14,8 @@ else()
COMPILE_FLAGS "${FIO_CFLAGS}")
endif()

if (WITH_FIO)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 5, 2018

Contributor

remove the space after if for consistency.

test/fio: enable objectstore FIO plugin building without the need to …
…install and build FIO source code

Signed-off-by: Igor Fedotov <ifedotov@suse.com>

@ifed01 ifed01 force-pushed the ifed01:wip-ifed-bring-fio branch from c1d288c to 9398051 Mar 5, 2018

@ifed01

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

@tchaikov - fixed

@tchaikov tchaikov merged commit c454b1d into ceph:master Mar 6, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@ifed01 ifed01 deleted the ifed01:wip-ifed-bring-fio branch Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.