From 940bd10a1d9cd3d2ba25d7491f81355d5b0cdb70 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 11 Apr 2025 12:39:09 +0100 Subject: [PATCH 1/4] Prefer normal job run log file over legacy one Signed-off-by: Babak K. Shandiz --- pkg/cmd/run/view/view.go | 42 ++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index d46c2c9a9..2922fb602 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -551,9 +551,19 @@ func getJobNameForLogFilename(name string) string { return sanitizedJobName } +// A job run log file is a top-level .txt file whose name starts with an ordinal +// number; e.g., "0_jobname.txt". func jobLogFilenameRegexp(job shared.Job) *regexp.Regexp { sanitizedJobName := getJobNameForLogFilename(job.Name) - re := fmt.Sprintf(`^-?\d+_%s\.txt`, regexp.QuoteMeta(sanitizedJobName)) + re := fmt.Sprintf(`^\d+_%s\.txt`, regexp.QuoteMeta(sanitizedJobName)) + return regexp.MustCompile(re) +} + +// A legacy job run log file is a top-level .txt file whose name starts with a +// negative number which is the ID of the run; e.g., "-2147483648_jobname.txt". +func legacyJobLogFilenameRegexp(job shared.Job) *regexp.Regexp { + sanitizedJobName := getJobNameForLogFilename(job.Name) + re := fmt.Sprintf(`^-\d+_%s\.txt`, regexp.QuoteMeta(sanitizedJobName)) return regexp.MustCompile(re) } @@ -658,26 +668,30 @@ func truncateAsUTF16(str string, max int) string { // where the ID can apparently be negative. func attachRunLog(rlz *zip.Reader, jobs []shared.Job) { for i, job := range jobs { - re := jobLogFilenameRegexp(job) - for _, file := range rlz.File { - if re.MatchString(file.Name) { - jobs[i].Log = file - break - } + // the normal job run log file is preferred over the legacy one. So, we + // try to find the normal log file, and if we couldn't find it then we + // look for the legacy one, if any. + jobLog := matchFileInZIPArchive(rlz, jobLogFilenameRegexp(job)) + if jobLog == nil { + jobLog = matchFileInZIPArchive(rlz, legacyJobLogFilenameRegexp(job)) } + jobs[i].Log = jobLog for j, step := range job.Steps { - re := stepLogFilenameRegexp(job, step) - for _, file := range rlz.File { - if re.MatchString(file.Name) { - jobs[i].Steps[j].Log = file - break - } - } + jobs[i].Steps[j].Log = matchFileInZIPArchive(rlz, stepLogFilenameRegexp(job, step)) } } } +func matchFileInZIPArchive(zr *zip.Reader, re *regexp.Regexp) *zip.File { + for _, file := range zr.File { + if re.MatchString(file.Name) { + return file + } + } + return nil +} + func displayRunLog(w io.Writer, jobs []shared.Job, failed bool) error { for _, job := range jobs { // To display a run log, we first try to compile it from individual step From 2d21b4624ca71087aa393de91c5c51652b711645 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 11 Apr 2025 12:40:26 +0100 Subject: [PATCH 2/4] Test normal job run log is preferred over legacy one Signed-off-by: Babak K. Shandiz --- pkg/cmd/run/view/fixtures/run_log.zip | Bin 8148 -> 8646 bytes pkg/cmd/run/view/view_test.go | 14 ++++++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/run/view/fixtures/run_log.zip b/pkg/cmd/run/view/fixtures/run_log.zip index 60701d9254cbcacdda1d9aff426de744b6d574d7..425ba09ddce377dc26bffb4eaf7cdc2c99bc92ab 100644 GIT binary patch delta 408 zcmca&f6RG9i{RuvEF3 Date: Fri, 11 Apr 2025 12:41:31 +0100 Subject: [PATCH 3/4] Add `$` anchor to log file regexps Signed-off-by: Babak K. Shandiz This is to make sure we do not pick up the wrong file if there is a `.txt` sequence in the middle of a job name. --- pkg/cmd/run/view/view.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index 2922fb602..b769028bc 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -555,7 +555,7 @@ func getJobNameForLogFilename(name string) string { // number; e.g., "0_jobname.txt". func jobLogFilenameRegexp(job shared.Job) *regexp.Regexp { sanitizedJobName := getJobNameForLogFilename(job.Name) - re := fmt.Sprintf(`^\d+_%s\.txt`, regexp.QuoteMeta(sanitizedJobName)) + re := fmt.Sprintf(`^\d+_%s\.txt$`, regexp.QuoteMeta(sanitizedJobName)) return regexp.MustCompile(re) } @@ -563,13 +563,13 @@ func jobLogFilenameRegexp(job shared.Job) *regexp.Regexp { // negative number which is the ID of the run; e.g., "-2147483648_jobname.txt". func legacyJobLogFilenameRegexp(job shared.Job) *regexp.Regexp { sanitizedJobName := getJobNameForLogFilename(job.Name) - re := fmt.Sprintf(`^-\d+_%s\.txt`, regexp.QuoteMeta(sanitizedJobName)) + re := fmt.Sprintf(`^-\d+_%s\.txt$`, regexp.QuoteMeta(sanitizedJobName)) return regexp.MustCompile(re) } func stepLogFilenameRegexp(job shared.Job, step shared.Step) *regexp.Regexp { sanitizedJobName := getJobNameForLogFilename(job.Name) - re := fmt.Sprintf(`^%s\/%d_.*\.txt`, regexp.QuoteMeta(sanitizedJobName), step.Number) + re := fmt.Sprintf(`^%s\/%d_.*\.txt$`, regexp.QuoteMeta(sanitizedJobName), step.Number) return regexp.MustCompile(re) } From f337ce90a07284634ab1db67cf823fcbc8f25799 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 11 Apr 2025 14:51:39 +0100 Subject: [PATCH 4/4] Explain job log resolution reason Signed-off-by: Babak K. Shandiz --- pkg/cmd/run/view/view.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index b769028bc..55be7f5f4 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -668,9 +668,10 @@ func truncateAsUTF16(str string, max int) string { // where the ID can apparently be negative. func attachRunLog(rlz *zip.Reader, jobs []shared.Job) { for i, job := range jobs { - // the normal job run log file is preferred over the legacy one. So, we - // try to find the normal log file, and if we couldn't find it then we - // look for the legacy one, if any. + // As a highest priority, we try to use the step logs first. We have seen zips that surprisingly contain + // step logs, normal job logs and legacy job logs. In this case, both job logs would be ignored. We have + // never seen a zip containing both job logs and no step logs, however, it may be possible. In that case + // let's prioritise the normal log over the legacy one. jobLog := matchFileInZIPArchive(rlz, jobLogFilenameRegexp(job)) if jobLog == nil { jobLog = matchFileInZIPArchive(rlz, legacyJobLogFilenameRegexp(job))