Re: [PATCH 1/2] KernelShark: Inherit libdir from Makefile

["Yordan Karadzhov (VMware)" <y.karadz@gmail.com>]

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