| Message ID | 20190810035902.14047-3-itsatharva@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/5] autobuild-run: use different output directories for reproducible builds testing | expand |
Hello Atharva, On Sat, 10 Aug 2019 09:29:00 +0530 Atharva Lele <itsatharva@gmail.com> wrote: > reproducible_results = os.path.join(self.outputdir, "results", "reproducible_results") I change the name of the JSON file to: diffoscope-results.json > + reproducible_results_text = os.path.join(self.outputdir, "results", "reproducible_results_text") And the name of the text file to: diffoscope-results.txt These names seem more sensible to me, and match better what we're already using for other files in the build results. I've applied to buildroot-test with this change. I have another suggestion below, though. > # Using only tar images for now > build_1_image = os.path.join(self.outputdir, "images", "rootfs.tar") > build_2_image = os.path.join(self.outputdir_2, "images", "rootfs.tar") > @@ -453,7 +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], stdout=diff, stderr=self.log) > + "--tool-prefix-binutils", prefix, "--json", "-", Instead of outputting the result to stdout, and then redirecting the stdout to "diff", what about using diffoscope ability to directly output to a file: "--json", reproducible_results, "--text", reproducible_results_text Thanks! Thomas
On Sun, Aug 11, 2019 at 7:11 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello Atharva, > > On Sat, 10 Aug 2019 09:29:00 +0530 > Atharva Lele <itsatharva@gmail.com> wrote: > > > reproducible_results = os.path.join(self.outputdir, "results", "reproducible_results") > > I change the name of the JSON file to: > > diffoscope-results.json > > > + reproducible_results_text = os.path.join(self.outputdir, "results", "reproducible_results_text") > > And the name of the text file to: > > diffoscope-results.txt > > These names seem more sensible to me, and match better what we're > already using for other files in the build results. > Changing the names does make sense, thanks! > I've applied to buildroot-test with this change. I have another > suggestion below, though. > > > # Using only tar images for now > > build_1_image = os.path.join(self.outputdir, "images", "rootfs.tar") > > build_2_image = os.path.join(self.outputdir_2, "images", "rootfs.tar") > > @@ -453,7 +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], stdout=diff, stderr=self.log) > > + "--tool-prefix-binutils", prefix, "--json", "-", > > Instead of outputting the result to stdout, and then redirecting the > stdout to "diff", what about using diffoscope ability to directly > output to a file: > > "--json", reproducible_results, "--text", reproducible_results_text > You're right. It'd be better to have it consistent. I'll prepare a patch to change it. > Thanks! > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Thanks for merging and thanks for the review!
On Sun, Aug 11, 2019 at 7:11 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello Atharva, > > On Sat, 10 Aug 2019 09:29:00 +0530 > Atharva Lele <itsatharva@gmail.com> wrote: > > > reproducible_results = os.path.join(self.outputdir, "results", "reproducible_results") > > I change the name of the JSON file to: > > diffoscope-results.json > > > + reproducible_results_text = os.path.join(self.outputdir, "results", "reproducible_results_text") > There's a typo here. Variable is named reproducible_results_txt in the patch, but in the diffoscope command I have used reproducible_results_text. I'll send a patch fixing this and the command consistency. > And the name of the text file to: > > diffoscope-results.txt > > These names seem more sensible to me, and match better what we're > already using for other files in the build results. > > I've applied to buildroot-test with this change. I have another > suggestion below, though. > > > # Using only tar images for now > > build_1_image = os.path.join(self.outputdir, "images", "rootfs.tar") > > build_2_image = os.path.join(self.outputdir_2, "images", "rootfs.tar") > > @@ -453,7 +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], stdout=diff, stderr=self.log) > > + "--tool-prefix-binutils", prefix, "--json", "-", > > Instead of outputting the result to stdout, and then redirecting the > stdout to "diff", what about using diffoscope ability to directly > output to a file: > > "--json", reproducible_results, "--text", reproducible_results_text > > Thanks! > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hello Atharva, On Tue, 13 Aug 2019 09:37:27 +0530 Atharva Lele <itsatharva@gmail.com> wrote: > > > + reproducible_results_text = os.path.join(self.outputdir, "results", "reproducible_results_text") > > > > There's a typo here. Variable is named reproducible_results_txt in the > patch, but in the diffoscope command I have used > reproducible_results_text. I'll send a patch fixing this and the > command consistency. My bad. I initially started changing the variable names to something more sensible, such as "diffoscope_results_json" and "diffoscope_results_txt" and then changed my mind, as it should have been a separate patch. But apparently, I undid my change incorrectly. Sorry about this. Thomas
diff --git a/scripts/autobuild-run b/scripts/autobuild-run index 69766b2..6adfa99 100755 --- a/scripts/autobuild-run +++ b/scripts/autobuild-run @@ -440,6 +440,7 @@ class Builder: """ reproducible_results = os.path.join(self.outputdir, "results", "reproducible_results") + reproducible_results_text = os.path.join(self.outputdir, "results", "reproducible_results_text") # Using only tar images for now build_1_image = os.path.join(self.outputdir, "images", "rootfs.tar") build_2_image = os.path.join(self.outputdir_2, "images", "rootfs.tar") @@ -453,7 +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], stdout=diff, stderr=self.log) + "--tool-prefix-binutils", prefix, "--json", "-", + "--text", reproducible_results_text, "--max-text-report-size", "40000"], + stdout=diff, 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)
Normal diffoscope output is readable by humans but not really convenient when working with it in code. JSON can be easily read and extracted information from. We still keep the normal text version because it's easy to parse by a human, but we limit its size to 40KB. Signed-off-by: Atharva Lele <itsatharva@gmail.com> --- Changes v1 -> v2: - Save text output as well as JSON --- scripts/autobuild-run | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)