From: "Yordan Karadzhov (VMware)" <y.karadz@gmail.com>
To: sztsian@gmail.com, tz.stoyanov@gmail.com, rostedt@goodmis.org
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH 1/2] KernelShark: Inherit libdir from Makefile
Date: Mon, 17 Feb 2020 13:26:03 +0200 [thread overview]
Message-ID: <a775d872-a67e-af1a-d0fc-6c6365171c75@gmail.com> (raw)
In-Reply-To: <20200209034226.287464-2-sztsian@gmail.com>
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. 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. 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@gmail.com wrote:
> From: "Ziqian SUN (Zamir)" <sztsian@gmail.com>
>
> 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@gmail.com>
> ---
> 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").
>
> 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.
Thanks!
Yordan
> if (CMAKE_BUILD_TYPE MATCHES Package)
>
> diff --git a/kernel-shark/src/CMakeLists.txt b/kernel-shark/src/CMakeLists.txt
> index 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/)
>
next prev parent reply other threads:[~2020-02-17 11:26 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-09 3:42 [PATCH 0/0] Two fixes for compiling sztsian
2020-02-09 3:42 ` [PATCH 1/2] KernelShark: Inherit libdir from Makefile sztsian
2020-02-17 11:26 ` Yordan Karadzhov (VMware) [this message]
2020-02-17 13:24 ` Zamir SUN
2020-02-17 14:11 ` Yordan Karadzhov (VMware)
2020-02-21 10:46 ` Yordan Karadzhov (VMware)
2020-02-21 11:56 ` Zamir SUN
2020-02-09 3:42 ` [PATCH 2/2] trace-cmd: Clean up old gtk stuff sztsian
2020-02-10 16:39 ` [PATCH 0/0] Two fixes for compiling Steven Rostedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a775d872-a67e-af1a-d0fc-6c6365171c75@gmail.com \
--to=y.karadz@gmail.com \
--cc=linux-trace-devel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=sztsian@gmail.com \
--cc=tz.stoyanov@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).