Re: [PATCH v2] cpupower: Improve cpupower build process description

[Shuah Khan <skhan@linuxfoundation.org>]

On 6/15/24 21:19, Roman Storozhenko wrote:
> On Sun, Jun 16, 2024 at 1:05 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> On 6/15/24 06:56, Roman Storozhenko wrote:
>>> Enhance cpupower build process description with the information on
>>> building and installing the utility to the user defined directories
>>> as well as with the information on the way of running the utility from
>>> the custom defined installation directory.
>>>
>>> Signed-off-by: Roman Storozhenko <romeusmeister@gmail.com>
>>> ---
>>> V1 -> V2:
>>>    - Improved commit description
>>>    - Make changed line lenghts 75 chars
>>>    - Refactored the description
>>>    - Link v1: https://lore.kernel.org/linux-pm/20240613-fix-cpupower-doc-v1-1-9dcdee263af1@gmail.com/
>>> ---
>>>    tools/power/cpupower/README | 160 +++++++++++++++++++++++++++++++++---
>>>    1 file changed, 150 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/tools/power/cpupower/README b/tools/power/cpupower/README
>>> index 1c68f47663b2..2678ed81d311 100644
>>> --- a/tools/power/cpupower/README
>>> +++ b/tools/power/cpupower/README
>>> @@ -22,16 +22,156 @@ interfaces [depending on configuration, see below].
>>>    compilation and installation
>>>    ----------------------------
>>>
>>> -make
>>> -su
>>> -make install
>>> -
>>> -should suffice on most systems. It builds libcpupower to put in
>>> -/usr/lib; cpupower, cpufreq-bench_plot.sh to put in /usr/bin; and
>>> -cpufreq-bench to put in /usr/sbin. If you want to set up the paths
>>> -differently and/or want to configure the package to your specific
>>> -needs, you need to open "Makefile" with an editor of your choice and
>>> -edit the block marked CONFIGURATION.
>>> +There are 2 output directories - one for the build output and another for
>>> +the installation of the build results, that is the utility, library,
>>> +man pages, etc...
>>> +
>>> +default directory
>>> +-----------------
>>> +
>>> +In the case of default directory, build and install process requires no
>>> +additional parameters:
>>> +
>>> +build
>>> +-----
>>> +
>>> +$ make
>>> +
>>> +The output directory for the 'make' command is the current directory and
>>> +its subdirs in the kernel tree:
>>> +tools/power/cpupower
>>> +
>>> +install
>>> +-------
>>> +
>>> +$ sudo make install
>>> +
>>> +'make install' command puts targets to default system dirs:
>>> +
>>> +-----------------------------------------------------------------------
>>> +| Installing file        |               System dir                   |
>>> +-----------------------------------------------------------------------
>>> +| libcpupower            | /usr/lib                                   |
>>> +-----------------------------------------------------------------------
>>> +| cpupower               | /usr/bin                                   |
>>> +-----------------------------------------------------------------------
>>> +| cpufreq-bench_plot.sh  | /usr/bin                                   |
>>> +-----------------------------------------------------------------------
>>> +| man pages              | /usr/man                                   |
>>> +-----------------------------------------------------------------------
>>> +
>>> +To put it in other words it makes build results available system-wide,
>>> +enabling any user to simply start using it without any additional steps
>>> +
>>> +custom directory
>>> +----------------
>>> +
>>> +There are 2 make's command-line variables 'O' and 'DESTDIR' that setup
>>> +appropriate dirs:
>>> +'O' - build directory
>>> +'DESTDIR' - installation directory. This variable could also be setup in
>>> +the 'CONFIGURATION' block of the "Makefile"
>>> +
>>> +build
>>> +-----
>>> +
>>> +$ make O=<your_custom_build_catalog>
>>> +
>>> +Example:
>>> +$ make O=/home/hedin/prj/cpupower/build
>>> +
>>> +install
>>> +-------
>>> +
>>> +$ make O=<your_custom_build_catalog> DESTDIR=<your_custom_install_catalog>
>>> +
>>> +Example:
>>> +$ make O=/home/hedin/prj/cpupower/build DESTDIR=/home/hedin/prj/cpupower \
>>> +> install
>>> +
>>> +Notice that both variables 'O' and 'DESTDIR' have been provided. The reason
>>> +is that the build results are saved in the custom output dir defined by 'O'
>>> +variable. So, this dir is the source for the installation step. If only
>>> +'DESTDIR' were provided then the 'install' target would assume that the
>>> +build directory is the current one, build everything there and install
>>> +from the current dir.
>>> +
>>> +The files will be installed to the following dirs:
>>> +
>>> +-----------------------------------------------------------------------
>>> +| Installing file        |               System dir                   |
>>> +-----------------------------------------------------------------------
>>> +| libcpupower            | ${DESTDIR}/usr/lib                         |
>>> +-----------------------------------------------------------------------
>>> +| cpupower               | ${DESTDIR}/usr/bin                         |
>>> +-----------------------------------------------------------------------
>>> +| cpufreq-bench_plot.sh  | ${DESTDIR}/usr/bin                         |
>>> +-----------------------------------------------------------------------
>>> +| man pages              | ${DESTDIR}/usr/man                         |
>>> +-----------------------------------------------------------------------
>>> +
>>> +If you look at the table for the default 'make' output dirs you will
>>> +notice that the only difference with the non-default case is the
>>> +${DESTDIR} prefix. So, the structure of the output dirs remains the same
>>> +regardles of the root output directory.
>>> +
>>> +
>>> +clean and uninstall
>>> +-------------------
>>> +
>>> +'clean' target is intended for cleanup the build catalog from build results
>>> +'uninstall' target is intended for removing installed files from the
>>> +installation directory
>>> +
>>> +default directory
>>> +-----------------
>>> +
>>> +This case is a straightforward one:
>>> +$ make clean
>>> +$ make uninstall
>>> +
>>> +custom directory
>>> +----------------
>>> +
>>> +Use 'O' command line variable to remove previously built files from the
>>> +build dir:
>>> +$ make O=<your_custom_build_catalog> clean
>>> +
>>> +Example:
>>> +$ make O=/home/hedin/prj/cpupower/build clean
>>> +
>>> +Use 'DESTDIR' command line variable to uninstall previously installed files
>>> +from the given dir:
>>> +$ make DESTDIR=<your_custom_install_catalog>
>>> +
>>> +Example:
>>> +make DESTDIR=/home/hedin/prj/cpupower uninstall
>>> +
>>> +
>>> +running the tool
>>> +----------------
>>> +
>>> +default directory
>>> +-----------------
>>> +
>>> +$ sudo cpupower
>>> +
>>> +custom directory
>>> +----------------
>>> +
>>> +When it comes to run the utility from the custom build catalog things
>>> +become a little bit complicated as 'just run' approach doesn't work.
>>> +Assuming that the current dir is '<your_custom_install_catalog>/usr',
>>> +issuing the following command:
>>> +
>>> +$ sudo ./bin/cpupower
>>> +will produce the following error output:
>>> +./bin/cpupower: error while loading shared libraries: libcpupower.so.1:
>>> +cannot open shared object file: No such file or directory
>>> +
>>> +The issue is that binary cannot find the 'libcpupower' library. So, we
>>> +shall point to the lib dir:
>>> +sudo LD_LIBRARY_PATH=lib64/ ./bin/cpupower
>>>
>>>
>>>    THANKS
>>
>> This "THANKS" doesn't belong in the patch.
> 
> The 'THANKS' does belong to the patch as well as the 'interfaces
> [depending on configuration, see below]' line at the top.
> Those 2 lines are parts of the original file and show the bottom and
> the top of  the changed text area.
> Just in case, I tried to re-add my changes using 'git add -i' on my
> dev machine and found that it is impossible to get rid of it.
> And it's explainable, git wants to know the area of the text to change.
> Besides, applying the downloaded patch using "git am' doesn't
> introduce the second 'THANKS' word at the bottom of the text.
> That is, the patch applies correctly.
> I decided to experiment further and removed the 'THANKS' line from the
> patch that had been sent and checked it with 'checkpatch':
> hedin@laptop:~/lkmp/patchwork/patches/powertools/cpupower/update_doc_install/tmp$
> ~/prj/linux/scripts/checkpatch.pl
> 0001-cpupower-Improve-cpupower-build-process-description.patch
>                                                         ERROR: patch
> seems to be corrupt (line wrapped?)
> #193: FILE: tools/power/cpupower/README:176:
> 
> Returned the 'THANKS':
> hedin@laptop:~/lkmp/patchwork/patches/powertools/cpupower/update_doc_install/tmp$
> ~/prj/linux/scripts/checkpatch.pl
> 0001-cpupower-Improve-cpupower-build-process-description.patch
> total: 0 errors, 0 warnings, 166 lines checked
> 0001-cpupower-Improve-cpupower-build-process-description.patch has no
> obvious style problems and is ready for submission.
> 
> So, as you can see 'THANKS' is a required part of the patch.
>>

Oh yeah. I missed that "THANKS" is in the README.

Applied for Linux 6.11-rc1
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux.git/log/?h=cpupower

thanks,
-- Shuah