On Mon, Jul 21, 2025 at 10:23:18AM +0200, Gabriele Monaco wrote:
> The current behaviour of rvgen when running with the -a option is to
> append the necessary lines at the end of the configuration for Kconfig,
> Makefile and tracepoints.
> This is not always the desired behaviour in case of nested monitors:
> while tracepoints are not affected by nesting and the Makefile's only
> requirement is that the parent monitor is built before its children, in
> the Kconfig it is better to have children defined right after their
> parent, otherwise the result has wrong indentation:
>
> [*] foo_parent monitor
> [*] foo_child1 monitor
> [*] foo_child2 monitor
> [*] bar_parent monitor
> [*] bar_child1 monitor
> [*] bar_child2 monitor
> [*] foo_child3 monitor
> [*] foo_child4 monitor
>
> Adapt rvgen to look for a different marker for nested monitors in the
> Kconfig file and append the line right after the last sibling, instead
> of the last monitor.
> Also add the marker when creating a new parent monitor.
>
> Signed-off-by: Gabriele Monaco <gmon...@redhat.com>
Some nitpicks below. But regardless:
Reviewed-by: Nam Cao <nam...@linutronix.de>
> - def __patch_file(self, file, marker, line):
> + def _patch_file(self, file, marker, line):
> + assert(self.auto_patch)
Nit: follows PEP8 unless there is a reason not to: assert self.auto_patch
> file_to_patch = os.path.join(self.rv_dir, file)
> content = self._read_file(file_to_patch)
> content = content.replace(marker, line + "\n" + marker)
> @@ -146,7 +147,7 @@ class RVGenerator:
> def fill_tracepoint_tooltip(self):
> monitor_class_type = self.fill_monitor_class_type()
> if self.auto_patch:
> - self.__patch_file("rv_trace.h",
> + self._patch_file("rv_trace.h",
> "// Add new monitors based on CONFIG_%s here" %
> monitor_class_type,
> "#include <monitors/%s/%s_trace.h>" %
> (self.name, self.name))
> return " - Patching %s/rv_trace.h, double check the result" %
> self.rv_dir
> @@ -156,10 +157,15 @@ Add this line where other tracepoints are included and
> %s is defined:
> #include <monitors/%s/%s_trace.h>
> """ % (self.rv_dir, monitor_class_type, self.name, self.name)
>
> + def _container_marker(self, container = None) -> str:
> + return "# Add new %smonitors here" % (container + " "
> + if container else "")
PEP8: container=None
And this function name is misleading, it is not marker for only containers.
Perhaps _kconfig_marker() is more appropriate.
