From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1E69846D01; Tue, 12 Aug 2025 05:17:18 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DE12E40270; Tue, 12 Aug 2025 05:17:17 +0200 (CEST) Received: from mail-pl1-f178.google.com (mail-pl1-f178.google.com [209.85.214.178]) by mails.dpdk.org (Postfix) with ESMTP id E3A694026C for ; Tue, 12 Aug 2025 05:17:15 +0200 (CEST) Received: by mail-pl1-f178.google.com with SMTP id d9443c01a7336-2405c0c431cso46971855ad.1 for ; Mon, 11 Aug 2025 20:17:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1754968635; x=1755573435; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=NooYKB6+MVIZbRwzY7amQtZtQP9vUI9A10OY+4C1fzA=; b=SjJceVCHwa2PqOwRgasYrTXSJ3trrYEEVPiu5G7x/odaGnvRjaykXil5/r2MAthKus OxaXICUbr33JwkwAnzqNu0Mzoxo1JKM1RWwNoS3zdCx71AAcbsJRwVZMAtaOEezE1sq5 kSImPm93KArPJGfN+HSd7OZkfFvSIhl/yxsjE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1754968635; x=1755573435; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=NooYKB6+MVIZbRwzY7amQtZtQP9vUI9A10OY+4C1fzA=; b=Crjgek6uQBwro4Ecefz+yjKj/e51D0OgjqDA6HfGPKPN01FdefBtOQLOIRL9P1RLlM 0hrrezYwa/KenqacngzykhN5TEk8UPIPPxEjZQbTU1Owp6oQEiC7j3R2pqiPfdy7vOg3 zhKm22SWO98MUSHwN3tI+trumVe5m1SHMg5J+zlrTMepHvkujD0xWdcu234jaW+FFpjE LWs5aAfEo2/e+Fl6k2JmcTRpHqMgVqstT8latjyViU6fmTorGccIrlyfjeaZfQkn1ui8 hNnXP4/1uM1WyYn6CkK9wQWRE2ySF9i+STTKZ0f/joh9l2+jO/s06Mqzkz/zpq/vQGrT hS9Q== X-Gm-Message-State: AOJu0YzHim9YBe3DwfP881pClWoFWej3Or329p46S4jAneeNER1PkGyw bX01xtZre5oKHVYiMtOm8xjqrH92PveTIMO7aW7G0TKV+cuRgXDJQ7jtH5ZPVj1mP9Yok/Rp61h XI1OX881KCyXHwzu/H5fo4/nEIB2Km8/N9dyWMFDj9eUyaqbZMOPDrlM= X-Gm-Gg: ASbGncut9/XaWxn7U2sf7766ZpDOQ5IOGJsTjjxrqNPbSWKqThuHP++kGIe5N0Xwj6S RBmb4nwkdj82YcfRKtL7IkuYbbpPOqI2t/+LCWu02a4nBuUN8kUzKlKfL84CRCeDw408McPaIT/ Jk4W21UH1ETtd2wRNvmHpzKECtcILawP+bhu8ebnU+KewPEU3EoKzQ5/2iU9ulUrBO+cKgn2zed SmBAgPa/WD8nuBYjn876qfdomeMcoSHog== X-Google-Smtp-Source: AGHT+IEFV62gpqd/DAYEL4+vFqi/6Kaptl69BirGfX/XKMs+EUZBbLF8t7C4kxWiC+R5h+2BySChcQV/BAjbP3vfyIQ= X-Received: by 2002:a17:902:7681:b0:240:bf59:26bb with SMTP id d9443c01a7336-242fc242d8dmr18668635ad.19.1754968634936; Mon, 11 Aug 2025 20:17:14 -0700 (PDT) MIME-Version: 1.0 References: <20250725151503.87374-1-luca.vizzarro@arm.com> <20250725151503.87374-6-luca.vizzarro@arm.com> In-Reply-To: <20250725151503.87374-6-luca.vizzarro@arm.com> From: Patrick Robb Date: Mon, 11 Aug 2025 23:10:54 -0400 X-Gm-Features: Ac12FXz5a2aVwfT77Fo4xjXfXPa87BMpNHkCgTaP3KCGci-CSVRcVS3aZQ_o-tk Message-ID: Subject: Re: [PATCH 5/6] dts: make log files into artifacts To: Luca Vizzarro Cc: dev@dpdk.org, Paul Szczepanek Content-Type: multipart/alternative; boundary="00000000000013ea3d063c227a5f" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --00000000000013ea3d063c227a5f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Jul 25, 2025 at 11:15=E2=80=AFAM Luca Vizzarro 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 > Reviewed-by: Paul Szczepanek > --- > > - def set_stage(self, stage: str, log_file_path: Path | None =3D None)= -> > None: > - """Set the DTS execution stage and optionally log to files. > + def set_stage(self, stage: str, log_file_name: str | None =3D None) = -> > None: > + """Set the DTS execution stage and optionally log to artifact > files. > > Set the DTS execution stage of the DTSLog class and optionally a= dd > - 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 debu= g > 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 !=3D stage: > self.info(f"Moving from stage '{DTSLogger._stage}' to stage > '{stage}'.") > DTSLogger._stage =3D 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 Tested-by: Patrick Robb --00000000000013ea3d063c227a5f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri, Jul 25,= 2025 at 11:15=E2=80=AFAM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
Make log files behave like artifacts as di= ctated 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>
---

-=C2=A0 =C2=A0 def set_stage(self, stage: str, log_file_path: Path | None = =3D None) -> None:
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Set the DTS execution stage = and optionally log to files.
+=C2=A0 =C2=A0 def set_stage(self, stage: str, log_file_name: str | None = =3D None) -> None:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Set the DTS execution stage = and optionally log to artifact files.

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Set the DTS execution stage of the DTSLog= class and optionally add
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 file handlers to the instance if the log file = name is provided.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 artifact handlers to the instance if the log a= rtifact file name is provided.

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 The file handlers log all messages. One is a r= egular human-readable log file and
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 the other one is a machine-readable log file w= ith extra debug information.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 The artifact handlers log all messages. One is= a regular human-readable log artifact and
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 the other one is a machine-readable log artifa= ct with extra debug information.

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Args:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0stage: The DTS stage to set= .
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 log_file_path: An optional path = of the log file to use. This should be a full path
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (either relative o= r absolute) without suffix (which will be appended).
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 log_file_name: An optional name = of the log artifact file to use. This should be without
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 suffix (which will= be appended).
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"""
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._remove_extra_file_handlers()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._remove_extra_artifact_handlers()

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if DTSLogger._stage !=3D stage:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self.info(f"Moving from sta= ge '{DTSLogger._stage}' to stage '{stage}'.")
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DTSLogger._stage =3D stage<= br>
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 if log_file_path:
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._extra_file_handlers.extend= (self._add_file_handlers(log_file_path))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if log_file_name:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._extra_artifact_handlers.ex= tend(self._add_artifact_handlers(log_file_name))

<= /div>
I have run from this patch and although the new tree structure is= a welcome addition for organizing (assuming we want to maintain testcase l= og 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 respec= tive artifact handlers are being popped prematurely with the set_stage() fu= nction above? I am out of time but that seems the most likely source of thi= s issue. Or, their handlers are being misconfigured in _add_artifact_handle= rs() 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 t= estsuite log file is being written to the testsuite dir and the testcase di= r. I don't feel super strongly about this but in my opinion the more in= tuitive way to present the logs would be without writing 1 file to two loca= tions.
3. Another open question... is it better to provide this c= urrent level of granularity (testcase logs), or better to just provide the = top level log file and the testsuite logs? My initial impression is that wi= th 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>
--00000000000013ea3d063c227a5f--