On 17.02.20 г. 15:24 ч., Zamir SUN wrote:
On 2/17/20 7:26 PM, Yordan Karadzhov (VMware) wrote:Hi Ziqian, Thanks a lot for helping us improving the build system! Sorry for the delay of my reply. I was on a ski vacation.Please explain us your motivation for this change. I have one general objection and couple of comments in the patch itself.Hi Yordan,To be a little verbose, the background of the whole patchset about trying to fulfill packaging guideline of distribution.I am the Fedora packager and recently updated trace-cmd to 2.8.3. Kernel shark lives as a sub package of trace-cmd in Fedora before I start with the packaging. By the time I update trace-cmd, I see some failure because libraries goes to lib directory rather than lib64 on x86_64, while the latter is the expected one for Fedora packages. So I have to come up with some sort of fix to make it happen.First of all, libkshark is not ready to be called officially a library, because we are still playing with the API. Our plan is to introduce the library (and its API) with the release of KernelShark 2.0. Before this, I would prefer all KernelShark libraries to be installed together with the executable. For me, separating the libraries and the executable looks like an encouragement for using directly the libraries, but we do not want to do this yet.Sorry for make this confusing. Actually even when I've already make it into lib64, I don't have the motivation to make it a separate library (from packaging point of view). If you think moving these can potential make users think the libraries is ready to use, it might be better for me to carry on the patch within Fedora only just to make this fulfill guidelines.Also please note that we are in the process of splitting the repository. When released, KernelShark 2.0 will be in a separate repo and will have its own build system that uses the trace-cmd libraries as external dependency.> On 9.02.20 г. 5:42 ч., sztsian@xxxxxxxxx wrote:From: "Ziqian SUN (Zamir)" <sztsian@xxxxxxxxx> The trace-cmd makefile supports install lib into a different name like lib64. Now this patch implemented the same in kernel-shark. And in order to make debuginfo also lives in the same file structure, the compiling dir is changed to reflect libdir and prefix as well. Signed-off-by: Ziqian SUN (Zamir) <sztsian@xxxxxxxxx> --- Makefile | 2 +- kernel-shark/CMakeLists.txt | 13 +++++++++---- kernel-shark/src/CMakeLists.txt | 6 +++--- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index d75f143..1aca807 100644 --- a/Makefile +++ b/Makefile @@ -295,7 +295,7 @@ CMAKE_COMMAND = /usr/bin/cmake BUILD_TYPE ?= RelWithDebInfo $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt- $(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -D_INSTALL_PREFIX=$(prefix) .. + $(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -D_INSTALL_PREFIX=$(prefix) -D_LIBDIR=$(libdir) ..gui: force $(CMD_TARGETS) $(kshark-dir)/build/Makefile $(Q)$(MAKE) $(S) -C $(kshark-dir)/build diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt index 8786b83..6652d08 100644 --- a/kernel-shark/CMakeLists.txt +++ b/kernel-shark/CMakeLists.txt @@ -17,6 +17,10 @@ if (NOT _INSTALL_PREFIX) set(_INSTALL_PREFIX "/usr/local") endif (NOT _INSTALL_PREFIX) +if (NOT _LIBDIR) + set(_LIBDIR "${_INSTALL_PREFIX}/lib") +endif (NOT _LIBDIR) + include(${KS_DIR}/build/FindTraceCmd.cmake) include(${KS_DIR}/build/FindJSONC.cmake) @@ -34,8 +38,8 @@ if (Qt5Widgets_FOUND) endif (Qt5Widgets_FOUND) -set(LIBRARY_OUTPUT_PATH "${KS_DIR}/lib") -set(EXECUTABLE_OUTPUT_PATH "${KS_DIR}/bin") +set(LIBRARY_OUTPUT_PATH "${KS_DIR}/${_LIBDIR}") +set(EXECUTABLE_OUTPUT_PATH "${KS_DIR}/${_INSTALL_PREFIX}/bin")Not sure what is your idea here, but this looks wrong to me. When you are building from source (just typing "make") you don't expect the object files and the executables to be placed outside the trunk of the repository(${KS_DIR}). Do not confuse building the source with installing (when typing "make install").In the beginning this was meant to make sure the debuginfo package is generated within corresponding lib64 directory. However I just compiled again and find it's not needed now.set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -pthread -fPIC")set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -std=c++11 -pthread -fPIC")@@ -54,14 +58,15 @@ if (NOT CMAKE_CXX_FLAGS_PACKAGE) set(CMAKE_CXX_FLAGS_PACKAGE "-O3") endif (NOT CMAKE_CXX_FLAGS_PACKAGE)-set(KS_PLUGIN_INSTALL_PREFIX ${_INSTALL_PREFIX}/lib/${KS_APP_NAME}/plugins/)+set(KS_PLUGIN_INSTALL_PREFIX ${_LIBDIR}/${KS_APP_NAME}/plugins/) set(KS_ICON KS_icon_shark.svg) set(KS_ICON_FIN KS_icon_fin.svg) set(KS_LOGO KS_logo_symbol.svg) set(KS_LOGO_LABEL KS_logo_horizontal.svg) -set(CMAKE_INSTALL_RPATH "${_INSTALL_PREFIX}/lib/${KS_APP_NAME}/") +SET(CMAKE_INSTALL_RPATH "${_LIBDIR}/${KS_APP_NAME}/")Please stick to lower-case characters with all CMake commands.Ah sorry, this is definitely a typo when converting to capital letters in bulks.Thanks for the review. As I don't know the background beforehand, if you think part of this patch still makes sense, I can make v2 and drop the bits you don't need yet.
Thanks for the clarification. Let's wait a bit to see if someone else is going to comment on this. Maybe you can add to the README file some explanation for the _LIBDIR Command-Line option, saying that it was added only to fulfill the Fedora packaging requirements.
cheers, Yordan
Thanks! Yordanif (CMAKE_BUILD_TYPE MATCHES Package)diff --git a/kernel-shark/src/CMakeLists.txt b/kernel-shark/src/CMakeLists.txtindex 33b5db8..9666b18 100644 --- a/kernel-shark/src/CMakeLists.txt +++ b/kernel-shark/src/CMakeLists.txt @@ -15,7 +15,7 @@ target_link_libraries(kshark ${TRACEEVENT_LIBRARY}set_target_properties(kshark PROPERTIES SUFFIX ".so.${KS_VERSION_STRING}") -install(TARGETS kshark LIBRARY DESTINATION ${_INSTALL_PREFIX}/lib/${KS_APP_NAME})+install(TARGETS kshark LIBRARY DESTINATION ${_LIBDIR}/${KS_APP_NAME}) if (OPENGL_FOUND AND GLUT_FOUND) @@ -29,7 +29,7 @@ if (OPENGL_FOUND AND GLUT_FOUND)set_target_properties(kshark-plot PROPERTIES SUFFIX ".so.${KS_VERSION_STRING}") - install(TARGETS kshark-plot LIBRARY DESTINATION ${_INSTALL_PREFIX}/lib/${KS_APP_NAME}) + install(TARGETS kshark-plot LIBRARY DESTINATION ${_LIBDIR}/${KS_APP_NAME})endif (OPENGL_FOUND AND GLUT_FOUND) @@ -85,7 +85,7 @@ if (Qt5Widgets_FOUND AND Qt5Network_FOUND) install(TARGETS ${KS_APP_NAME} kshark-record kshark-gui RUNTIME DESTINATION ${_INSTALL_PREFIX}/bin/ - LIBRARY DESTINATION ${_INSTALL_PREFIX}/lib/${KS_APP_NAME}/) + LIBRARY DESTINATION ${_LIBDIR}/${KS_APP_NAME}/) install(FILES "${KS_DIR}/${KS_APP_NAME}.desktop" DESTINATION ${_INSTALL_PREFIX}/share/applications/)
|