| Message ID | 20190813041206.20715-1-itsatharva@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3,1/3] autobuild-run: maintain consistency in diffoscope command | expand |
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
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!
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)
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(-)