[v3,1/3] autobuild-run: maintain consistency in diffoscope command
diff mbox series

Message ID 20190813041206.20715-1-itsatharva@gmail.com
State Accepted
Headers show
Series
  • [v3,1/3] autobuild-run: maintain consistency in diffoscope command
Related show

Commit Message

Atharva Lele Aug. 13, 2019, 4:12 a.m. UTC
Since we are outputting to two file types, we can't just redirect stdout.
Instead, we use diffoscope's ability to output to a file to maintain
consistency in the command.

This patch also fixes a typo in the command. For text output,
it should be reproducibility_results_txt instead of
reproducibility_results_text.

Signed-off-by: Atharva Lele <itsatharva@gmail.com>
---
 scripts/autobuild-run | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Thomas Petazzoni Aug. 13, 2019, 2:19 p.m. UTC | #1
Hello Atharva,

On Tue, 13 Aug 2019 09:42:04 +0530
Atharva Lele <itsatharva@gmail.com> wrote:

> Since we are outputting to two file types, we can't just redirect stdout.
> Instead, we use diffoscope's ability to output to a file to maintain
> consistency in the command.
> 
> This patch also fixes a typo in the command. For text output,
> it should be reproducibility_results_txt instead of
> reproducibility_results_text.
> 
> Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> ---
>  scripts/autobuild-run | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Applied to buildroot-test, thanks. However, the code running diffoscope
is still executed inside a with open(reproducible_results, 'w') as
diff, which is no longer needed, as diffoscope outputs directly to the
reproducible_results file.

You should move that this file opening inside the else condition which
is used when diffoscope is not available.

Best regards,

Thomas
Atharva Lele Aug. 16, 2019, 1:56 p.m. UTC | #2
On Tue, Aug 13, 2019 at 7:49 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Atharva,
>
> On Tue, 13 Aug 2019 09:42:04 +0530
> Atharva Lele <itsatharva@gmail.com> wrote:
>
> > Since we are outputting to two file types, we can't just redirect stdout.
> > Instead, we use diffoscope's ability to output to a file to maintain
> > consistency in the command.
> >
> > This patch also fixes a typo in the command. For text output,
> > it should be reproducibility_results_txt instead of
> > reproducibility_results_text.
> >
> > Signed-off-by: Atharva Lele <itsatharva@gmail.com>
> > ---
> >  scripts/autobuild-run | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
>
> Applied to buildroot-test, thanks. However, the code running diffoscope
> is still executed inside a with open(reproducible_results, 'w') as
> diff, which is no longer needed, as diffoscope outputs directly to the
> reproducible_results file.
>
> You should move that this file opening inside the else condition which
> is used when diffoscope is not available.
>

Oh right, slipped my mind. I'll send a patch to fix it!

Apologies for the late reply, but your email got buried between
message delivery failures since emails to Yann are bouncing.

> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thank you for the review!

Patch
diff mbox series

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index dbb497c..ead81a0 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -454,9 +454,9 @@  class Builder:
                 prefix = prefix[13:-1]
                 log_write(self.log, "INFO: running diffoscope on images")
                 subprocess.call(["diffoscope", build_1_image, build_2_image,
-                                 "--tool-prefix-binutils", prefix, "--json", "-",
-                                 "--text", reproducible_results_text, "--max-text-report-size", "40000"],
-                                stdout=diff, stderr=self.log)
+                                 "--tool-prefix-binutils", prefix, "--json", reproducible_results,
+                                 "--text", reproducible_results_txt, "--max-text-report-size", "40000"],
+                                stderr=self.log)
             else:
                 log_write(self.log, "INFO: diffoscope not installed, falling back to cmp")
                 subprocess.call(["cmp", "-b", build_1_image, build_2_image], stdout=diff, stderr=self.log)