[1/2] kernel-shark: Make FreeSans a compulsory component

Commit Message

Solomon Tan April 20, 2022, 3:38 p.m. UTC

This patch is proposed for two reasons.

Firstly, with reference to
https://lore.kernel.org/linux-trace-devel/Yl9WBztqKGD0PpmJ@ArchDesktop/
when the CMake does not detect FreeSans, it should cease generating the
Makefile since kernelshark requires FreeSans to output its graph
properly.

Secondly, when the make instructions on the README are followed, even
when FreeSans is not detected, `make` continues to compile and `make
install` (or `install_gui`) would install without indicating an error.
This gives the impression of a successful install, but `kernelshark`
will be missing.

Therefore, making CMake quit with error when FreeSans is not installed
solves these two issues.

Signed-off-by: Solomon Tan <[e-mail]>
---
 CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yordan Karadzhov April 21, 2022, 12:04 p.m. UTC | #1

On 20.04.22 г. 18:38 ч., Solomon Tan wrote:
> This patch is proposed for two reasons.
> 
> Firstly, with reference to
> https://lore.kernel.org/linux-trace-devel/Yl9WBztqKGD0PpmJ@ArchDesktop/
> when the CMake does not detect FreeSans, it should cease generating the
> Makefile since kernelshark requires FreeSans to output its graph
> properly.
> 
> Secondly, when the make instructions on the README are followed, even
> when FreeSans is not detected, `make` continues to compile and `make
> install` (or `install_gui`) would install without indicating an error.
> This gives the impression of a successful install, but `kernelshark`
> will be missing.
> 
> Therefore, making CMake quit with error when FreeSans is not installed
> solves these two issues.
> 
> Signed-off-by: Solomon Tan <[e-mail]>
> ---
>   CMakeLists.txt | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index e6a76d8..5473bfa 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -69,7 +69,7 @@ if (TT_FONT_FILE)
>   
>   else (TT_FONT_FILE)
>   
> -  message(WARNING "\nCould not find font ${KS_FONT}!\n")
> +  message(FATAL_ERROR "\nCould not find font ${KS_FONT}!\n")

Hi Solomon,

Having this message as warning instead of a fatal error is done on purpose. We have other projects, that are using the 
kernelshark library (libkshark) but do not need the kernelshark GUI. So the library have to be able to build even if the 
third party packages needed by the GUI are not installed.

On the other hand I understand your argument that this can be confusing for someone who wants to build the GUI and do 
not care about the library. My suggestion is to keep the warning, but to make the message more informative, so that the 
user will be aware that the GUI is not going build.

And please send your patch-sets as new email threads.

Thanks!
Yordan

>   
>   endif (TT_FONT_FILE)
>

Patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index e6a76d8..5473bfa 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -69,7 +69,7 @@  if (TT_FONT_FILE)
 
 else (TT_FONT_FILE)
 
-  message(WARNING "\nCould not find font ${KS_FONT}!\n")
+  message(FATAL_ERROR "\nCould not find font ${KS_FONT}!\n")
 
 endif (TT_FONT_FILE)