Re: [PATCH 5/6] dts: make log files into artifacts

[Patrick Robb <probb@iol.unh.edu>]

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

On Fri, Jul 25, 2025 at 11:15 AM Luca Vizzarro <luca.vizzarro@arm.com>
wrote:

> Make log files behave like artifacts as dictated by the Artifact class.
> Implicitly, this will automatically place all the logs in a structured
> manner.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
>
> -    def set_stage(self, stage: str, log_file_path: Path | None = None) ->
> None:
> -        """Set the DTS execution stage and optionally log to files.
> +    def set_stage(self, stage: str, log_file_name: str | None = None) ->
> None:
> +        """Set the DTS execution stage and optionally log to artifact
> files.
>
>          Set the DTS execution stage of the DTSLog class and optionally add
> -        file handlers to the instance if the log file name is provided.
> +        artifact handlers to the instance if the log artifact file name
> is provided.
>
> -        The file handlers log all messages. One is a regular
> human-readable log file and
> -        the other one is a machine-readable log file with extra debug
> information.
> +        The artifact handlers log all messages. One is a regular
> human-readable log artifact and
> +        the other one is a machine-readable log artifact with extra debug
> information.
>
>          Args:
>              stage: The DTS stage to set.
> -            log_file_path: An optional path of the log file to use. This
> should be a full path
> -                (either relative or absolute) without suffix (which will
> be appended).
> +            log_file_name: An optional name of the log artifact file to
> use. This should be without
> +                suffix (which will be appended).
>          """
> -        self._remove_extra_file_handlers()
> +        self._remove_extra_artifact_handlers()
>
>          if DTSLogger._stage != stage:
>              self.info(f"Moving from stage '{DTSLogger._stage}' to stage
> '{stage}'.")
>              DTSLogger._stage = stage
>
> -        if log_file_path:
> -
> self._extra_file_handlers.extend(self._add_file_handlers(log_file_path))
> +        if log_file_name:
> +
> self._extra_artifact_handlers.extend(self._add_artifact_handlers(log_file_name))
>

I have run from this patch and although the new tree structure is a welcome
addition for organizing (assuming we want to maintain testcase log files),
I think there are some issues or ideas I would like to raise:

1. Most importantly, I notice that the testsuite and testcase files are no
longer tracking testsuite information - they're basically empty except for
transition information. I wonder if their respective artifact handlers are
being popped prematurely with the set_stage() function above? I am out of
time but that seems the most likely source of this issue. Or, their
handlers are being misconfigured in _add_artifact_handlers() but I don't
think so. I can DM you an output dir from dts for-main and also from my
branch with your series applied.
2. I see the testsuite log file is being written to the testsuite dir and
the testcase dir. I don't feel super strongly about this but in my opinion
the more intuitive way to present the logs would be without writing 1 file
to two locations.
3. Another open question... is it better to provide this current level of
granularity (testcase logs), or better to just provide the top level log
file and the testsuite logs? My initial impression is that with the tree
structure, the high number of log files is basically harmless, and would
provide a quick path to finding the specific logs one wants (when they know
the failing testcase) so I'm not requesting a change... just raising the
point for discussion.

>
> 2.43.0
>
>
Reviewed-by: Patrick Robb <probb@iol.unh.edu>
Tested-by: Patrick Robb <probb@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 5001 bytes --]