diff mbox series

[1/2] ptest-cargo: move run-ptest rc variable initialisation

Message ID 20250828120544.2982231-1-yoann.congal@smile.fr
State New
Headers show
Series [1/2] ptest-cargo: move run-ptest rc variable initialisation | expand

Commit Message

Yoann Congal Aug. 28, 2025, 12:05 p.m. UTC 
From: Yoann Congal <yoann.congal@smile.fr>

ptest-cargo run-ptest can be generated in two fashions: generated from
scratch or appended to an exiting run-ptest file. The rc variable used
to track tests failure was only initialized in "generated from scratch"
case. Which lead to errors in the "appended" case.

Move the rc variable initialisation to the common code of both case to
fix this problem.

Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
Cc: Gyorgy Sarvari <skandigraun@gmail.com>
---
 meta/classes-recipe/ptest-cargo.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gyorgy Sarvari Aug. 28, 2025, 12:45 p.m. UTC | #1 
On 8/28/25 14:05, Yoann Congal wrote:
> From: Yoann Congal <yoann.congal@smile.fr>
>
> ptest-cargo run-ptest can be generated in two fashions: generated from
> scratch or appended to an exiting run-ptest file. The rc variable used
> to track tests failure was only initialized in "generated from scratch"
> case. Which lead to errors in the "appended" case.
>
> Move the rc variable initialisation to the common code of both case to
> fix this problem.
>
> Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
> Cc: Gyorgy Sarvari <skandigraun@gmail.com>
> ---
>  meta/classes-recipe/ptest-cargo.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meta/classes-recipe/ptest-cargo.bbclass b/meta/classes-recipe/ptest-cargo.bbclass
> index ece25ff1eb..e35de1042e 100644
> --- a/meta/classes-recipe/ptest-cargo.bbclass
> +++ b/meta/classes-recipe/ptest-cargo.bbclass
> @@ -103,10 +103,10 @@ python do_install_ptest_cargo() {
>      with open(ptest_script, "a") as f:
>          if not script_exists:
>              f.write("#!/bin/sh\n")
> -            f.write("rc=0\n")                
>          else:
>              f.write(f"\necho \"\"\n")
>              f.write(f"echo \"## starting to run rust tests ##\"\n")               
> +        f.write("rc=0\n")
>          for test_path in test_paths:
>              script = textwrap.dedent(f"""\
>                  if ! {test_path} {rust_test_args}

I'm not sure if this is correct. This value is what's returned at the
end[1], but here it is set to 0 unconditionally, even though it might
have a previous value.

As an example, I have almost sent an updated librsvg ptest patch[2] -
while writing v2, I noticed that it has not only C, but also Rust tests,
so I started to add them too. The resulting run-ptest script first
starts with the C test that I execute, and partial result is stored in
the same variable. And then at the end ptest-cargo appends its own
tests, and continues to use the same rc variable.

One thing I gotta admit is that this rc variable is kinda hidden in this
class.

[1]:
https://git.yoctoproject.org/poky/tree/meta/classes-recipe/ptest-cargo.bbclass#n122
[2]: when I get around it again, I still believe that it will happen...
Yoann Congal Aug. 28, 2025, 2:01 p.m. UTC | #2 
Le jeu. 28 août 2025 à 14:45, Gyorgy Sarvari <skandigraun@gmail.com> a
écrit :

> On 8/28/25 14:05, Yoann Congal wrote:
> > From: Yoann Congal <yoann.congal@smile.fr>
> >
> > ptest-cargo run-ptest can be generated in two fashions: generated from
> > scratch or appended to an exiting run-ptest file. The rc variable used
> > to track tests failure was only initialized in "generated from scratch"
> > case. Which lead to errors in the "appended" case.
> >
> > Move the rc variable initialisation to the common code of both case to
> > fix this problem.
> >
> > Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
> > Cc: Gyorgy Sarvari <skandigraun@gmail.com>
> > ---
> >  meta/classes-recipe/ptest-cargo.bbclass | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/meta/classes-recipe/ptest-cargo.bbclass
> b/meta/classes-recipe/ptest-cargo.bbclass
> > index ece25ff1eb..e35de1042e 100644
> > --- a/meta/classes-recipe/ptest-cargo.bbclass
> > +++ b/meta/classes-recipe/ptest-cargo.bbclass
> > @@ -103,10 +103,10 @@ python do_install_ptest_cargo() {
> >      with open(ptest_script, "a") as f:
> >          if not script_exists:
> >              f.write("#!/bin/sh\n")
> > -            f.write("rc=0\n")
> >          else:
> >              f.write(f"\necho \"\"\n")
> >              f.write(f"echo \"## starting to run rust tests ##\"\n")
>
> > +        f.write("rc=0\n")
> >          for test_path in test_paths:
> >              script = textwrap.dedent(f"""\
> >                  if ! {test_path} {rust_test_args}
>
> I'm not sure if this is correct. This value is what's returned at the
> end[1], but here it is set to 0 unconditionally, even though it might
> have a previous value.
>
> As an example, I have almost sent an updated librsvg ptest patch[2] -
> while writing v2, I noticed that it has not only C, but also Rust tests,
> so I started to add them too. The resulting run-ptest script first
> starts with the C test that I execute, and partial result is stored in
> the same variable. And then at the end ptest-cargo appends its own
> tests, and continues to use the same rc variable.
>

You're right!

How about we initialize rc to 0 in ptest-cargo ONLY IF it is not already
defined at that point?
That would allow custom run-ptest scripts to setup their rc variable but it
would also allow scripts that don't want to bother with it (like
rpm-sequoia) to completely ignore it.


> One thing I gotta admit is that this rc variable is kinda hidden in this
> class.
>
> [1]:
>
> https://git.yoctoproject.org/poky/tree/meta/classes-recipe/ptest-cargo.bbclass#n122
> [2]: when I get around it again, I still believe that it will happen...
>
Gyorgy Sarvari Aug. 28, 2025, 3:37 p.m. UTC | #3 
On 8/28/25 16:01, Yoann Congal wrote:
>
>
> Le jeu. 28 août 2025 à 14:45, Gyorgy Sarvari <skandigraun@gmail.com> a
> écrit :
>
>     On 8/28/25 14:05, Yoann Congal wrote:
>     > From: Yoann Congal <yoann.congal@smile.fr>
>     >
>     > ptest-cargo run-ptest can be generated in two fashions:
>     generated from
>     > scratch or appended to an exiting run-ptest file. The rc
>     variable used
>     > to track tests failure was only initialized in "generated from
>     scratch"
>     > case. Which lead to errors in the "appended" case.
>     >
>     > Move the rc variable initialisation to the common code of both
>     case to
>     > fix this problem.
>     >
>     > Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
>     > Cc: Gyorgy Sarvari <skandigraun@gmail.com>
>     > ---
>     >  meta/classes-recipe/ptest-cargo.bbclass | 2 +-
>     >  1 file changed, 1 insertion(+), 1 deletion(-)
>     >
>     > diff --git a/meta/classes-recipe/ptest-cargo.bbclass
>     b/meta/classes-recipe/ptest-cargo.bbclass
>     > index ece25ff1eb..e35de1042e 100644
>     > --- a/meta/classes-recipe/ptest-cargo.bbclass
>     > +++ b/meta/classes-recipe/ptest-cargo.bbclass
>     > @@ -103,10 +103,10 @@ python do_install_ptest_cargo() {
>     >      with open(ptest_script, "a") as f:
>     >          if not script_exists:
>     >              f.write("#!/bin/sh\n")
>     > -            f.write("rc=0\n")               
>     >          else:
>     >              f.write(f"\necho \"\"\n")
>     >              f.write(f"echo \"## starting to run rust tests
>     ##\"\n")               
>     > +        f.write("rc=0\n")
>     >          for test_path in test_paths:
>     >              script = textwrap.dedent(f"""\
>     >                  if ! {test_path} {rust_test_args}
>
>     I'm not sure if this is correct. This value is what's returned at the
>     end[1], but here it is set to 0 unconditionally, even though it might
>     have a previous value.
>
>     As an example, I have almost sent an updated librsvg ptest patch[2] -
>     while writing v2, I noticed that it has not only C, but also Rust
>     tests,
>     so I started to add them too. The resulting run-ptest script first
>     starts with the C test that I execute, and partial result is stored in
>     the same variable. And then at the end ptest-cargo appends its own
>     tests, and continues to use the same rc variable.
>
>
> You're right!
>
> How about we initialize rc to 0 in ptest-cargo ONLY IF it is not
> already defined at that point?
> That would allow custom run-ptest scripts to setup their rc variable
> but it would also allow scripts that don't want to bother with it
> (like rpm-sequoia) to completely ignore it.

This doesn't sound bad, personally I don't have anything against it - as
long as the original value is also kept, I think that would do.

Thinking a bit about it, it could be also removed from the rpm-sequoia
script in any case - by the end of the script it is either defined by an
error, or the last "exit $rc" will just become "exit" which implicitly
turns into "exit $?", which is also not that bad. (Though not everyone
likes such variables, and personally I'm not extremely attached either)

>  
>
>     One thing I gotta admit is that this rc variable is kinda hidden
>     in this
>     class.
>
>     [1]:
>     https://git.yoctoproject.org/poky/tree/meta/classes-recipe/ptest-cargo.bbclass#n122
>     [2]: when I get around it again, I still believe that it will
>     happen...
>
>
>
> -- 
> Yoann Congal
> Smile ECS
diff mbox series

Patch

diff --git a/meta/classes-recipe/ptest-cargo.bbclass b/meta/classes-recipe/ptest-cargo.bbclass
index ece25ff1eb..e35de1042e 100644
--- a/meta/classes-recipe/ptest-cargo.bbclass
+++ b/meta/classes-recipe/ptest-cargo.bbclass
@@ -103,10 +103,10 @@  python do_install_ptest_cargo() {
     with open(ptest_script, "a") as f:
         if not script_exists:
             f.write("#!/bin/sh\n")
-            f.write("rc=0\n")                
         else:
             f.write(f"\necho \"\"\n")
             f.write(f"echo \"## starting to run rust tests ##\"\n")               
+        f.write("rc=0\n")
         for test_path in test_paths:
             script = textwrap.dedent(f"""\
                 if ! {test_path} {rust_test_args}