From: "Yordan Karadzhov (VMware)" <y.karadz@gmail.com>
To: Zamir SUN <sztsian@gmail.com>
Cc: tz.stoyanov@gmail.com, rostedt@goodmis.org,
linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH 1/2] KernelShark: Inherit libdir from Makefile
Date: Mon, 17 Feb 2020 16:11:38 +0200 [thread overview]
Message-ID: <79cdd43b-263a-e0c7-622b-b878526b5686@gmail.com> (raw)
In-Reply-To: <6aa37828-8122-3eb6-031d-f72167951f1c@gmail.com>
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@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").
>
> 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!
>> 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 14:11 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)
2020-02-17 13:24 ` Zamir SUN
2020-02-17 14:11 ` Yordan Karadzhov (VMware) [this message]
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=79cdd43b-263a-e0c7-622b-b878526b5686@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).