[PATCH] kernel-shark: Do not hardcode /usr prefix for polkit policies

[PATCH] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Michal Sojka]

---
 kernel-shark/src/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel-shark/src/CMakeLists.txt b/kernel-shark/src/CMakeLists.txt
index 457c100..687e150 100644
--- a/kernel-shark/src/CMakeLists.txt
+++ b/kernel-shark/src/CMakeLists.txt
@@ -92,7 +92,7 @@ if (Qt5Widgets_FOUND AND Qt5Network_FOUND)
             DESTINATION ${_INSTALL_PREFIX}/share/icons/${KS_APP_NAME})
 
     install(FILES "${KS_DIR}/org.freedesktop.kshark-record.policy"
-            DESTINATION /usr/share/polkit-1/actions/)
+            DESTINATION ${_INSTALL_PREFIX}/share/polkit-1/actions/)
 
     install(PROGRAMS "${KS_DIR}/bin/kshark-su-record"
             DESTINATION ${_INSTALL_PREFIX}/bin/)
-- 
2.30.1

Re: [PATCH] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Steven Rostedt]

On Thu, 11 Mar 2021 14:12:21 +0100
Michal Sojka <michal.sojka@cvut.cz> wrote:

Hi Michal,

Thanks for the fix!

Note, even when submitting an "obvious" patch, it's always good to add some
content to the change log body. Basically, why it shouldn't be hard coded.
That is, what failed because of it. I can think of a few things, but we try
to have content in the change log body for all commits, even obvious ones.
The only exception is spelling fixes don't need content.

Thanks!

-- Steve


> ---
>  kernel-shark/src/CMakeLists.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel-shark/src/CMakeLists.txt b/kernel-shark/src/CMakeLists.txt
> index 457c100..687e150 100644
> --- a/kernel-shark/src/CMakeLists.txt
> +++ b/kernel-shark/src/CMakeLists.txt
> @@ -92,7 +92,7 @@ if (Qt5Widgets_FOUND AND Qt5Network_FOUND)
>              DESTINATION ${_INSTALL_PREFIX}/share/icons/${KS_APP_NAME})
>  
>      install(FILES "${KS_DIR}/org.freedesktop.kshark-record.policy"
> -            DESTINATION /usr/share/polkit-1/actions/)
> +            DESTINATION ${_INSTALL_PREFIX}/share/polkit-1/actions/)
>  
>      install(PROGRAMS "${KS_DIR}/bin/kshark-su-record"
>              DESTINATION ${_INSTALL_PREFIX}/bin/)

Re: [PATCH] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Michal Sojka]

Hi Steven,

On Thu, Mar 11 2021, Steven Rostedt wrote:
> On Thu, 11 Mar 2021 14:12:21 +0100
> Michal Sojka <michal.sojka@cvut.cz> wrote:
>
> Hi Michal,
>
> Thanks for the fix!
>
> Note, even when submitting an "obvious" patch, it's always good to add
> some content to the change log body.

Understood. Sending v2...

-Michal

[PATCH v2] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Michal Sojka]

When one wants to install kernel-shark to a non-standard location,
e.g., by configuring it as follows:

    cmake -D_INSTALL_PREFIX=$HOME ..

then "make install" fails, with the following error:

    CMake Error at src/cmake_install.cmake:225 (file):
      file INSTALL cannot copy file
      "/home/user/src/trace-cmd/kernel-shark/org.freedesktop.kshark-record.policy"
      to "/usr/share/polkit-1/actions/org.freedesktop.kshark-record.policy".

This commit fixes that by ensuring that even the
org.freedesktop.kshark-record.policy file is installed to the
user-specified prefix and not to /usr where the user has no write
permission.
---
 kernel-shark/src/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel-shark/src/CMakeLists.txt b/kernel-shark/src/CMakeLists.txt
index 457c100..687e150 100644
--- a/kernel-shark/src/CMakeLists.txt
+++ b/kernel-shark/src/CMakeLists.txt
@@ -92,7 +92,7 @@ if (Qt5Widgets_FOUND AND Qt5Network_FOUND)
             DESTINATION ${_INSTALL_PREFIX}/share/icons/${KS_APP_NAME})
 
     install(FILES "${KS_DIR}/org.freedesktop.kshark-record.policy"
-            DESTINATION /usr/share/polkit-1/actions/)
+            DESTINATION ${_INSTALL_PREFIX}/share/polkit-1/actions/)
 
     install(PROGRAMS "${KS_DIR}/bin/kshark-su-record"
             DESTINATION ${_INSTALL_PREFIX}/bin/)
-- 
2.30.1

Re: [PATCH v2] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Steven Rostedt]

On Thu, 11 Mar 2021 15:50:59 +0100
Michal Sojka <michal.sojka@cvut.cz> wrote:

> When one wants to install kernel-shark to a non-standard location,
> e.g., by configuring it as follows:
> 
>     cmake -D_INSTALL_PREFIX=$HOME ..
> 
> then "make install" fails, with the following error:
> 
>     CMake Error at src/cmake_install.cmake:225 (file):
>       file INSTALL cannot copy file
>       "/home/user/src/trace-cmd/kernel-shark/org.freedesktop.kshark-record.policy"
>       to "/usr/share/polkit-1/actions/org.freedesktop.kshark-record.policy".
> 
> This commit fixes that by ensuring that even the
> org.freedesktop.kshark-record.policy file is installed to the
> user-specified prefix and not to /usr where the user has no write
> permission.

Thanks for the updated change log Michal!

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

Re: [PATCH v2] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Yordan Karadzhov (VMware)]

Hi Michal,

Thank you very much for helping us improving kernelshark!


On 11.03.21 г. 16:50, Michal Sojka wrote:
> When one wants to install kernel-shark to a non-standard location,
> e.g., by configuring it as follows:
> 
>      cmake -D_INSTALL_PREFIX=$HOME ..
> 
> then "make install" fails, with the following error:
> 
>      CMake Error at src/cmake_install.cmake:225 (file):
>        file INSTALL cannot copy file
>        "/home/user/src/trace-cmd/kernel-shark/org.freedesktop.kshark-record.policy"
>        to "/usr/share/polkit-1/actions/org.freedesktop.kshark-record.policy".
> 
> This commit fixes that by ensuring that even the
> org.freedesktop.kshark-record.policy file is installed to the
> user-specified prefix and not to /usr where the user has no write
> permission.
In your case the installation fails to install the policy file used by
Polkit. Note that this doesn't mean that the kernelshark installation 
itself fails.

As far as I know the policy file can only go to a special locations so 
that Polkit can find it. Otherwise it will have no effect (I may be 
wrong on this).

If you know how to tell Polkit to search in an arbitrary location for 
those files, please let me know.

Thanks!
Yordan

> ---
>   kernel-shark/src/CMakeLists.txt | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel-shark/src/CMakeLists.txt b/kernel-shark/src/CMakeLists.txt
> index 457c100..687e150 100644
> --- a/kernel-shark/src/CMakeLists.txt
> +++ b/kernel-shark/src/CMakeLists.txt
> @@ -92,7 +92,7 @@ if (Qt5Widgets_FOUND AND Qt5Network_FOUND)
>               DESTINATION ${_INSTALL_PREFIX}/share/icons/${KS_APP_NAME})
>   
>       install(FILES "${KS_DIR}/org.freedesktop.kshark-record.policy"
> -            DESTINATION /usr/share/polkit-1/actions/)
> +            DESTINATION ${_INSTALL_PREFIX}/share/polkit-1/actions/)
>   
>       install(PROGRAMS "${KS_DIR}/bin/kshark-su-record"
>               DESTINATION ${_INSTALL_PREFIX}/bin/)
> 

Re: [PATCH v2] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Michal Sojka]

Hi Yordan,

On Thu, Mar 11 2021, Yordan Karadzhov (VMware) wrote:
> In your case the installation fails to install the policy file used by
> Polkit. Note that this doesn't mean that the kernelshark installation 
> itself fails.

Yes, I know, but when something fails, users are confused ;-)

> As far as I know the policy file can only go to a special locations so 
> that Polkit can find it. Otherwise it will have no effect (I may be 
> wrong on this).

You're right. I looked at polkit sources and it really seems that only
one location is supported. It is determined at configuration time so on
some systems it may be different from /usr/share/... I'm talking
specifically about NixOS, but it already has the patch I sent.

So I leave it up to you whether to apply the patch or not. I think that
supporting seamless installation into $HOME is useful if one wants to
quickly use a newer version not available in their distribution.

BTW, thanks for great tool!

-Michal

Re: [PATCH v2] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Yordan Karadzhov (VMware)]

Hi Michal,

On 11.03.21 г. 19:57, Michal Sojka wrote:
> Hi Yordan,
> 
> On Thu, Mar 11 2021, Yordan Karadzhov (VMware) wrote:
>> In your case the installation fails to install the policy file used by
>> Polkit. Note that this doesn't mean that the kernelshark installation
>> itself fails.
> 
> Yes, I know, but when something fails, users are confused ;-)

You are right here. We have to think how to make this message look more 
like a warning.

> 
>> As far as I know the policy file can only go to a special locations so
>> that Polkit can find it. Otherwise it will have no effect (I may be
>> wrong on this).
> 
> You're right. I looked at polkit sources and it really seems that only
> one location is supported. It is determined at configuration time so on
> some systems it may be different from /usr/share/... I'm talking
> specifically about NixOS, but it already has the patch I sent.
> 
> So I leave it up to you whether to apply the patch or not. I think that
> supporting seamless installation into $HOME is useful if one wants to
> quickly use a newer version not available in their distribution.

Building, installing as root and testing the latest version shouldn't 
cause any problems/conflicts on your system. Note that there is a script 
in kernel-shark/build called "cmake_uninstall.sh". It is guaranteed that 
this script removes every single file that has been installed.

And BTW from the patch I see that you still use the old version that 
comes together with trace-cmd. If you are keen to try the latest 
version, checkout this one:

https://git.kernel.org/pub/scm/utils/trace-cmd/kernel-shark.git/

This is a brand new version that has a lot of changes under the hood and 
needs user testing. We will be extremely happy to receive bug reports ;) 
or patches for this new version.

Thanks!
Yordan

Re: [PATCH v2] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 2112 bytes --]

On Fri, 2021-03-12 at 08:46 +0200, Yordan Karadzhov (VMware) wrote:
> Hi Michal,
> 
Hey Michal! Nice to see you here too... well, it's a small world, I
guess? :-P

> On 11.03.21 г. 19:57, Michal Sojka wrote:
> > You're right. I looked at polkit sources and it really seems that
> > only
> > one location is supported. It is determined at configuration time
> > so on
> > some systems it may be different from /usr/share/... I'm talking
> > specifically about NixOS, but it already has the patch I sent.
> > 
> > So I leave it up to you whether to apply the patch or not. I think
> > that
> > supporting seamless installation into $HOME is useful if one wants
> > to
> > quickly use a newer version not available in their distribution.
> 
> Building, installing as root and testing the latest version shouldn't
> cause any problems/conflicts on your system. Note that there is a
> script 
> in kernel-shark/build called "cmake_uninstall.sh". It is guaranteed
> that 
> this script removes every single file that has been installed.
> 
Not completely sure, but since NixOS is being mentioned, I think at
least part of the point here is  making sure that a development/testing
version of KS can be installed in those "special" systems that have
read-only filesystems (or similar configurations).

In such a system, even if you are root, if the makefile tries to put
stuff in a part of the fs which is not writable, it's pretty much
game-over.

In fact, I don't know much about NixOS, but I'm on one of those
"immutable" systems myself (openSUSE MicroOS, FTR) so I can relate. :-)

So, although it's definitely a niche use-case (for now!! :-P), I think
that either this patch, or at least not making the above issue fatal
(as you're saying yourself) would make the life of some users easier.

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Yordan Karadzhov (VMware)]

Hi Dario and Michal,

On 12.03.21 г. 13:47, Dario Faggioli wrote:
> On Fri, 2021-03-12 at 08:46 +0200, Yordan Karadzhov (VMware) wrote:
>> Hi Michal,
>>
> Hey Michal! Nice to see you here too... well, it's a small world, I
> guess? :-P
> 
>> On 11.03.21 г. 19:57, Michal Sojka wrote:
>>> You're right. I looked at polkit sources and it really seems that
>>> only
>>> one location is supported. It is determined at configuration time
>>> so on
>>> some systems it may be different from /usr/share/... I'm talking
>>> specifically about NixOS, but it already has the patch I sent.
>>>
>>> So I leave it up to you whether to apply the patch or not. I think
>>> that
>>> supporting seamless installation into $HOME is useful if one wants
>>> to
>>> quickly use a newer version not available in their distribution.
>>
>> Building, installing as root and testing the latest version shouldn't
>> cause any problems/conflicts on your system. Note that there is a
>> script
>> in kernel-shark/build called "cmake_uninstall.sh". It is guaranteed
>> that
>> this script removes every single file that has been installed.
>>
> Not completely sure, but since NixOS is being mentioned, I think at
> least part of the point here is  making sure that a development/testing
> version of KS can be installed in those "special" systems that have
> read-only filesystems (or similar configurations).
> 
> In such a system, even if you are root, if the makefile tries to put
> stuff in a part of the fs which is not writable, it's pretty much
> game-over.
> 
> In fact, I don't know much about NixOS, but I'm on one of those
> "immutable" systems myself (openSUSE MicroOS, FTR) so I can relate. :-)

OK, now I see your problem. Please excuse my ignorance.

Here's the deal. What you want can be achieved with an almost trivial 
patch in kernelshark 2. Have a look here:

https://git.kernel.org/pub/scm/utils/trace-cmd/kernel-shark.git/tree/src/CMakeLists.txt#n129

and make the installation of the policy file to be a separate component.
Make the installation of "kshark-su-record" part of the new component as 
well since it relies on the policy file. I guess "kernelshark.desktop" 
must be there as well.

Then you can edit the helper script "install_gui.sh" and make it install 
your new component as well. You can also add another helper script that 
install only the part you want. Send me a patch and I will take it.

Thanks!
Yordan

> So, although it's definitely a niche use-case (for now!! :-P), I think
> that either this patch, or at least not making the above issue fatal
> (as you're saying yourself) would make the life of some users easier.
> 
> Regards
> 

Re: [PATCH v2] kernel-shark: Do not hardcode /usr prefix for polkit policies

[Michal Sojka]

Hi Yordan,

On Fri, Mar 12 2021, Yordan Karadzhov (VMware) wrote:
> If you are keen to try the latest version, checkout this one:
>
> https://git.kernel.org/pub/scm/utils/trace-cmd/kernel-shark.git/
>
> This is a brand new version that has a lot of changes under the hood and 
> needs user testing. We will be extremely happy to receive bug reports ;) 
> or patches for this new version.

Great! I've compiled this version and so far everything works. But I use
just basic features - opening the trace, searching and zooming.

On Fri, Mar 12 2021, Yordan Karadzhov (VMware) wrote:
> OK, now I see your problem. Please excuse my ignorance.
>
> Here's the deal. What you want can be achieved with an almost trivial 
> patch in kernelshark 2. Have a look here:
>
> https://git.kernel.org/pub/scm/utils/trace-cmd/kernel-shark.git/tree/src/CMakeLists.txt#n129
>
> and make the installation of the policy file to be a separate component.
> Make the installation of "kshark-su-record" part of the new component as 
> well since it relies on the policy file.

Sounds good. I'm working on the patch. I'll also send other changes that
will make my life easier.

> I guess "kernelshark.desktop" must be there as well.

I don't think so. Desktop files are looked for at multiple places,
including ~/.local/share/application or $XDG_<SOMETHING>.


-Michal