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

[Zamir SUN <sztsian@gmail.com>]

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