Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/0] Two fixes for compiling
@ 2020-02-09  3:42 sztsian
  2020-02-09  3:42 ` [PATCH 1/2] KernelShark: Inherit libdir from Makefile sztsian
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: sztsian @ 2020-02-09  3:42 UTC (permalink / raw)
  To: tz.stoyanov, rostedt; +Cc: sztsian, linux-trace-devel

This contains two fixes of compiling.

The KernelShark one is to inherit libdir into kernelshark CMakeLists so
that the libdir parsed to Makefile is honored in kernelshark.

The trace-cmd one is to remove python-gui which depends on the obsoleted
gtk stuff.

In order to compile and test this change, the following patch from
Tzvetomir need to be applied.
[PATCH] kernel-shark: Fix search path for libtracefs
https://lore.kernel.org/linux-trace-devel/20200205070743.13836-1-tz.stoyanov@gmail.com/T/#u

Note: I always send email and git commits with signature "Zamir SUN"
before. However I see DCO requires to sign using real name, so I changed
to fulfill the DCO.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] KernelShark: Inherit libdir from Makefile
  2020-02-09  3:42 [PATCH 0/0] Two fixes for compiling sztsian
@ 2020-02-09  3:42 ` sztsian
  2020-02-17 11:26   ` Yordan Karadzhov (VMware)
  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
  2 siblings, 1 reply; 9+ messages in thread
From: sztsian @ 2020-02-09  3:42 UTC (permalink / raw)
  To: tz.stoyanov, rostedt; +Cc: sztsian, linux-trace-devel

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")
 
 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}/")
+
 
 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/)
-- 
2.24.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] trace-cmd: Clean up old gtk stuff
  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-09  3:42 ` sztsian
  2020-02-10 16:39 ` [PATCH 0/0] Two fixes for compiling Steven Rostedt
  2 siblings, 0 replies; 9+ messages in thread
From: sztsian @ 2020-02-09  3:42 UTC (permalink / raw)
  To: tz.stoyanov, rostedt; +Cc: sztsian, linux-trace-devel

From: "Ziqian SUN (Zamir)" <sztsian@gmail.com>

With ab37762ba9ad06a4f3902ad53ac2e9104bfd651f the gtk stuff was removed
from kernel-shark. This patch removes the python-gui which depends on
the trace-view from the gtk stuff.

Signed-off-by: Ziqian SUN (Zamir) <sztsian@gmail.com>
---
 Makefile              |  6 ---
 python/Makefile       | 14 +------
 python/ctracecmdgui.i | 88 -------------------------------------------
 3 files changed, 2 insertions(+), 106 deletions(-)
 delete mode 100644 python/ctracecmdgui.i

diff --git a/Makefile b/Makefile
index 1aca807..75bf1ef 100644
--- a/Makefile
+++ b/Makefile
@@ -126,7 +126,6 @@ endif
 
 ifndef NO_PYTHON
 PYTHON		:= ctracecmd.so
-PYTHON_GUI	:= ctracecmd.so ctracecmdgui.so
 
 PYTHON_VERS ?= python
 PYTHON_PKGCONFIG_VERS ?= $(PYTHON_VERS)
@@ -473,14 +472,9 @@ export PYGTK_CFLAGS
 ctracecmd.so: force $(LIBTRACECMD_STATIC)
 	$(Q)$(MAKE) -C $(src)/python $@
 
-ctracecmdgui.so: force $(LIBTRACECMD_STATIC) trace-view
-	$(Q)$(MAKE) -C $(src)/python $@
-
 PHONY += python
 python: $(PYTHON)
 
-PHONY += python-gui
-python-gui: $(PYTHON_GUI)
 
 dist:
 	git archive --format=tar --prefix=trace-cmd-$(TRACECMD_VERSION)/ HEAD \
diff --git a/python/Makefile b/python/Makefile
index 85224af..deaa83e 100644
--- a/python/Makefile
+++ b/python/Makefile
@@ -1,13 +1,9 @@
 include $(src)/scripts/utils.mk
 
-TRACE_VIEW_OBJS =
-TRACE_VIEW_OBJS += $(obj)/kernel-shark/trace-view.o
-TRACE_VIEW_OBJS += $(obj)/kernel-shark/trace-view-store.o
-
 ifdef BUILD_PYTHON_WORKS
 PYTHON_SO_INSTALL := ctracecmd.install
 PYTHON_PY_PROGS := event-viewer.install
-PYTHON_PY_LIBS := tracecmd.install tracecmdgui.install
+PYTHON_PY_LIBS := tracecmd.install
 endif
 
 ctracecmd.so: ctracecmd.i $(LIBTRACECMD_STATIC)
@@ -15,12 +11,6 @@ ctracecmd.so: ctracecmd.i $(LIBTRACECMD_STATIC)
 	$(CC) -fpic -c $(CPPFLAGS) $(CFLAGS) $(PYTHON_INCLUDES)  ctracecmd_wrap.c
 	$(CC) --shared $(LIBTRACECMD_STATIC) $(LDFLAGS) ctracecmd_wrap.o -o ctracecmd.so $(TRACE_LIBS)
 
-ctracecmdgui.so: ctracecmdgui.i $(LIBTRACECMD_STATIC) $(TRACE_VIEW_OBJS)
-	swig -Wall -python -noproxy -I$(src)/kernel-shark/include ctracecmdgui.i
-	$(CC) -fpic -c  $(CPPFLAGS) $(CFLAGS) $(INCLUDES) $(PYTHON_INCLUDES) $(PYGTK_CFLAGS) ctracecmdgui_wrap.c
-	$(CC) --shared $(TRACE_VIEW_OBJS) $(LIBTRACECMD_STATIC) $(LDFLAGS) $(LIBS) $(CONFIG_LIBS) ctracecmdgui_wrap.o -o ctracecmdgui.so $(TRACE_LIBS)
-
-
 $(PYTHON_SO_INSTALL): %.install : %.so force
 	$(Q)$(call do_install_data,$<,$(python_dir_SQ))
 
@@ -34,7 +24,7 @@ install_python: $(PYTHON_SO_INSTALL) $(PYTHON_PY_PROGS) $(PYTHON_PY_LIBS)
 
 
 clean:
-	$(RM) *.a *.so *.o .*.d ctracecmd_wrap.* ctracecmdgui_wrap.*
+	$(RM) *.a *.so *.o .*.d ctracecmd_wrap.*
 
 force:
 .PHONY: clean force
diff --git a/python/ctracecmdgui.i b/python/ctracecmdgui.i
deleted file mode 100644
index 4a7c6ac..0000000
--- a/python/ctracecmdgui.i
+++ /dev/null
@@ -1,88 +0,0 @@
-// ctracecmdgui.i
-%module ctracecmdgui
-%include typemaps.i
-
-%{
-#include "trace-view-store.h"
-#include <pygobject.h>
-#include <pyglib.h>
-#include <Python.h>
-#include <memoryobject.h>
-
-extern GtkTreeModel *trace_view_store_as_gtk_tree_model(struct trace_view_store *store);
-
-PyObject *
-pytype_from_gtype(GType gtype)
-{
-    PyTypeObject *pt = NULL;
-    switch (gtype) {
-    case G_TYPE_INT:
-    case G_TYPE_UINT:
-        pt = &PyLong_Type;
-        break;
-    case G_TYPE_STRING:
-        pt = &PyUnicode_Type;
-        break;
-    default:
-        return Py_None;
-    }
-    return (PyObject *)pt;
-}
-%}
-
-
-/* return python longs from unsigned long long functions */
-%typemap(out) unsigned long long {
-    $result = PyLong_FromUnsignedLongLong((unsigned long long) $1);
-}
-
-/* help swig cope with g* types */
-#if PY_MAJOR_VERSION >= 3
-%typemap(in) gint {
-    $1 = PyLong_AsLong($input);
-}
-%typemap(out) gint {
-    $result = PyLong_FromLong($1);
-}
-#else
-%typemap(in) gint {
-    $1 = PyInt_AsLong($input);
-}
-%typemap(out) gint {
-    $result = PyInt_FromLong($1);
-}
-#endif
-%typemap(in) guint {
-    $1 = PyLong_AsUnsignedLong($input);
-}
-%typemap(out) guint {
-    $result = PyLong_FromUnsignedLong($1);
-}
-%typemap(in) guint64 {
-    $1 = PyLong_AsUnsignedLongLong($input);
-}
-%typemap(out) guint64 {
-    $result = PyLong_FromUnsignedLongLong($1);
-}
-%typemap(out) GType {
-    $result = pytype_from_gtype($1);
-}
-%typemap(out) GtkTreeModelFlags {
-    $result = PyLong_FromLong($1);
-}
-
-
-%inline %{
-GtkTreeModel *trace_view_store_as_gtk_tree_model(struct trace_view_store *store)
-{
-    return GTK_TREE_MODEL(store);
-}
-%}
-
-
-/* SWIG can't grok these, define them to nothing */
-#define __trace
-#define __attribute__(x)
-#define __thread
-
-%include "trace-view-store.h"
-- 
2.24.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/0] Two fixes for compiling
  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-09  3:42 ` [PATCH 2/2] trace-cmd: Clean up old gtk stuff sztsian
@ 2020-02-10 16:39 ` Steven Rostedt
  2 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2020-02-10 16:39 UTC (permalink / raw)
  To: sztsian; +Cc: tz.stoyanov, linux-trace-devel, Yordan Karadzhov


Hi Zamir!

Thanks for the patches.

On Sun,  9 Feb 2020 11:42:24 +0800
sztsian@gmail.com wrote:

> This contains two fixes of compiling.
> 
> The KernelShark one is to inherit libdir into kernelshark CMakeLists so
> that the libdir parsed to Makefile is honored in kernelshark.

Yodan, can you review this one?

> 
> The trace-cmd one is to remove python-gui which depends on the obsoleted
> gtk stuff.
> 
> In order to compile and test this change, the following patch from
> Tzvetomir need to be applied.
> [PATCH] kernel-shark: Fix search path for libtracefs
> https://lore.kernel.org/linux-trace-devel/20200205070743.13836-1-tz.stoyanov@gmail.com/T/#u

I will start applying these.

> 
> Note: I always send email and git commits with signature "Zamir SUN"
> before. However I see DCO requires to sign using real name, so I changed
> to fulfill the DCO.
> 

Yes, thank you for doing so.

-- Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] KernelShark: Inherit libdir from Makefile
  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
  0 siblings, 1 reply; 9+ messages in thread
From: Yordan Karadzhov (VMware) @ 2020-02-17 11:26 UTC (permalink / raw)
  To: sztsian, tz.stoyanov, rostedt; +Cc: linux-trace-devel

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/)
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] KernelShark: Inherit libdir from Makefile
  2020-02-17 11:26   ` Yordan Karadzhov (VMware)
@ 2020-02-17 13:24     ` Zamir SUN
  2020-02-17 14:11       ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 9+ messages in thread
From: Zamir SUN @ 2020-02-17 13:24 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware); +Cc: tz.stoyanov, rostedt, linux-trace-devel



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!
> 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/)
>>

-- 
Zamir SUN
Fedora user
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] KernelShark: Inherit libdir from Makefile
  2020-02-17 13:24     ` Zamir SUN
@ 2020-02-17 14:11       ` Yordan Karadzhov (VMware)
  2020-02-21 10:46         ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 9+ messages in thread
From: Yordan Karadzhov (VMware) @ 2020-02-17 14:11 UTC (permalink / raw)
  To: Zamir SUN; +Cc: tz.stoyanov, rostedt, linux-trace-devel



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/)
>>>
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] KernelShark: Inherit libdir from Makefile
  2020-02-17 14:11       ` Yordan Karadzhov (VMware)
@ 2020-02-21 10:46         ` Yordan Karadzhov (VMware)
  2020-02-21 11:56           ` Zamir SUN
  0 siblings, 1 reply; 9+ messages in thread
From: Yordan Karadzhov (VMware) @ 2020-02-21 10:46 UTC (permalink / raw)
  To: Zamir SUN; +Cc: tz.stoyanov, rostedt, linux-trace-devel



On 17.02.20 г. 16:11 ч., Yordan Karadzhov (VMware) wrote:
>>>> 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.
> 

Hi Ziqian,

I had a conversation with Steven about the problem and he suggested 
another solution how to make sure that the users know the libraries are 
not ready to be used directly yet. Just send new version of the patch 
with the small changes I asked for. Do not change the README file. I 
will implement what Steven suggested on top of your patch.

Thanks!
Yordan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] KernelShark: Inherit libdir from Makefile
  2020-02-21 10:46         ` Yordan Karadzhov (VMware)
@ 2020-02-21 11:56           ` Zamir SUN
  0 siblings, 0 replies; 9+ messages in thread
From: Zamir SUN @ 2020-02-21 11:56 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware); +Cc: tz.stoyanov, rostedt, linux-trace-devel



On 2/21/20 6:46 PM, Yordan Karadzhov (VMware) wrote:
> 
> 
> On 17.02.20 г. 16:11 ч., Yordan Karadzhov (VMware) wrote:
>>>>> 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.
>>
> 
> Hi Ziqian,
> 
> I had a conversation with Steven about the problem and he suggested 
> another solution how to make sure that the users know the libraries are 
> not ready to be used directly yet. Just send new version of the patch 
> with the small changes I asked for. Do not change the README file. I 
> will implement what Steven suggested on top of your patch.
> 

Hi Yordan,

Sure, I'll re-send with above fixed no later than this weekend.

Thanks for the heads-up.

> Thanks!
> Yordan

-- 
Zamir SUN
Fedora user
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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)
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

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git