| Message ID | 20190820145231.15507-3-itsatharva@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/5] autobuild-run: check if reproducibile_results exists before checking its size | expand |
On 20/08/2019 16:52, Atharva Lele wrote: > Signed-off-by: Atharva Lele <itsatharva@gmail.com> > --- > Changes v1 -> v3: > - Account for changed name of diffoscope output > --- > scripts/autobuild-run | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/scripts/autobuild-run b/scripts/autobuild-run > index 384cf73..876feb2 100755 > --- a/scripts/autobuild-run > +++ b/scripts/autobuild-run > @@ -689,15 +689,26 @@ class Builder: > > def get_failure_reason(): > # Output is a tuple (package, version), or None. > - lastlines = decode_bytes(subprocess.Popen( > - ["tail", "-n", "3", os.path.join(self.outputdir, "logfile")], > - stdout=subprocess.PIPE).communicate()[0]).splitlines() > - > - regexp = re.compile(r'make: \*\*\* .*/(?:build|toolchain)/([^/]*)/') > - for line in lastlines: > - m = regexp.search(line) > - if m: > - return m.group(1).rsplit('-', 1) > + # Output is "package-reproducible" in case of reproducibility failure. > + > + reproducible_results = os.path.join(self.resultdir, "diffoscope-results.json") Since file is now used in several functions, it would make sense to move it to the class itself, like we do for so many others. > + if os.path.exists(reproducible_results) and os.stat(reproducible_results).st_size > 0: > + if self.sysinfo.has("diffoscope"): It would make more sense to move this condition up - if diffoscope is not installed, there will be no diffoscope-results.json. Oh, actually there will be, but it's not a JSON file... So, actually, in case diffoscope is not installed, we should not create diffoscope-results.json, but diffoscope-results.txt (and no json). Regards, Arnout > + reason = get_reproducibility_failure_reason(reproducible_results) > + reason.append("nonreproducible") > + return reason > + else: > + return ["nonreproducible"] > + else: > + lastlines = decode_bytes(subprocess.Popen( > + ["tail", "-n", "3", os.path.join(self.outputdir, "logfile")], > + stdout=subprocess.PIPE).communicate()[0]).splitlines() > + > + regexp = re.compile(r'make: \*\*\* .*/(?:build|toolchain)/([^/]*)/') > + for line in lastlines: > + m = regexp.search(line) > + if m: > + return m.group(1).rsplit('-', 1) > > # not found > return None >
On Sun, Sep 8, 2019 at 10:43 PM Arnout Vandecappelle <arnout@mind.be> wrote: > > > > On 20/08/2019 16:52, Atharva Lele wrote: > > Signed-off-by: Atharva Lele <itsatharva@gmail.com> > > --- > > Changes v1 -> v3: > > - Account for changed name of diffoscope output > > --- > > scripts/autobuild-run | 29 ++++++++++++++++++++--------- > > 1 file changed, 20 insertions(+), 9 deletions(-) > > > > diff --git a/scripts/autobuild-run b/scripts/autobuild-run > > index 384cf73..876feb2 100755 > > --- a/scripts/autobuild-run > > +++ b/scripts/autobuild-run > > @@ -689,15 +689,26 @@ class Builder: > > > > def get_failure_reason(): > > # Output is a tuple (package, version), or None. > > - lastlines = decode_bytes(subprocess.Popen( > > - ["tail", "-n", "3", os.path.join(self.outputdir, "logfile")], > > - stdout=subprocess.PIPE).communicate()[0]).splitlines() > > - > > - regexp = re.compile(r'make: \*\*\* .*/(?:build|toolchain)/([^/]*)/') > > - for line in lastlines: > > - m = regexp.search(line) > > - if m: > > - return m.group(1).rsplit('-', 1) > > + # Output is "package-reproducible" in case of reproducibility failure. > > + > > + reproducible_results = os.path.join(self.resultdir, "diffoscope-results.json") > > Since file is now used in several functions, it would make sense to move it to > the class itself, like we do for so many others. Yup will do. > > > + if os.path.exists(reproducible_results) and os.stat(reproducible_results).st_size > 0: > > + if self.sysinfo.has("diffoscope"): > > It would make more sense to move this condition up - if diffoscope is not > installed, there will be no diffoscope-results.json. Oh, actually there will be, > but it's not a JSON file... So, actually, in case diffoscope is not installed, > we should not create diffoscope-results.json, but diffoscope-results.txt (and no > json). > We're now outputting two files: 1) diffoscope-results.json for tooling purposes (reproducible_results) 2) diffoscope-results.txt for human-reading (reproducible_results_txt) diffoscope-results.txt is used if diffoscope is not installed (check patch 4 in the series). So I guess after patch 4, checking if diffoscope is installed here doesn't make sense. If reproducible_results exists, it would mean that its a diffoscope outputted file. > Regards, > Arnout > > > + reason = get_reproducibility_failure_reason(reproducible_results) > > + reason.append("nonreproducible") > > + return reason > > + else: > > + return ["nonreproducible"] > > + else: > > + lastlines = decode_bytes(subprocess.Popen( > > + ["tail", "-n", "3", os.path.join(self.outputdir, "logfile")], > > + stdout=subprocess.PIPE).communicate()[0]).splitlines() > > + > > + regexp = re.compile(r'make: \*\*\* .*/(?:build|toolchain)/([^/]*)/') > > + for line in lastlines: > > + m = regexp.search(line) > > + if m: > > + return m.group(1).rsplit('-', 1) > > > > # not found > > return None > > Thanks for the review!
On 12/09/2019 14:59, Atharva Lele wrote: > On Sun, Sep 8, 2019 at 10:43 PM Arnout Vandecappelle <arnout@mind.be> wrote: >> >> >> >> On 20/08/2019 16:52, Atharva Lele wrote: [snip] >>> + if os.path.exists(reproducible_results) and os.stat(reproducible_results).st_size > 0: >>> + if self.sysinfo.has("diffoscope"): >> >> It would make more sense to move this condition up - if diffoscope is not >> installed, there will be no diffoscope-results.json. Oh, actually there will be, >> but it's not a JSON file... So, actually, in case diffoscope is not installed, >> we should not create diffoscope-results.json, but diffoscope-results.txt (and no >> json). >> > > We're now outputting two files: > 1) diffoscope-results.json for tooling purposes (reproducible_results) > 2) diffoscope-results.txt for human-reading (reproducible_results_txt) > diffoscope-results.txt is used if diffoscope is not installed (check > patch 4 in the series). > > So I guess after patch 4, checking if diffoscope is installed here > doesn't make sense. If reproducible_results exists, it would mean that > its a diffoscope outputted file. Ah yes. It would make sense to move patch 4 earlier in the series I think. It's for sure one of the less controversial changes in the series. But that may be a pretty complicated rebase... Regards, Arnout [snip]
diff --git a/scripts/autobuild-run b/scripts/autobuild-run index 384cf73..876feb2 100755 --- a/scripts/autobuild-run +++ b/scripts/autobuild-run @@ -689,15 +689,26 @@ class Builder: def get_failure_reason(): # Output is a tuple (package, version), or None. - lastlines = decode_bytes(subprocess.Popen( - ["tail", "-n", "3", os.path.join(self.outputdir, "logfile")], - stdout=subprocess.PIPE).communicate()[0]).splitlines() - - regexp = re.compile(r'make: \*\*\* .*/(?:build|toolchain)/([^/]*)/') - for line in lastlines: - m = regexp.search(line) - if m: - return m.group(1).rsplit('-', 1) + # Output is "package-reproducible" in case of reproducibility failure. + + reproducible_results = os.path.join(self.resultdir, "diffoscope-results.json") + if os.path.exists(reproducible_results) and os.stat(reproducible_results).st_size > 0: + if self.sysinfo.has("diffoscope"): + reason = get_reproducibility_failure_reason(reproducible_results) + reason.append("nonreproducible") + return reason + else: + return ["nonreproducible"] + else: + lastlines = decode_bytes(subprocess.Popen( + ["tail", "-n", "3", os.path.join(self.outputdir, "logfile")], + stdout=subprocess.PIPE).communicate()[0]).splitlines() + + regexp = re.compile(r'make: \*\*\* .*/(?:build|toolchain)/([^/]*)/') + for line in lastlines: + m = regexp.search(line) + if m: + return m.group(1).rsplit('-', 1) # not found return None
Signed-off-by: Atharva Lele <itsatharva@gmail.com> --- Changes v1 -> v3: - Account for changed name of diffoscope output --- scripts/autobuild-run | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)