| Message ID | 87ee72fwv0.fsf@ponder |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#51985] lint: Adjust patch file length check. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
Hi! Vagrant Cascadian <vagrant@debian.org> skribis: > On 2021-11-25, Ludovic Courtès wrote: [...] >> I think independently of the switch to ustar, it’s a good idea for ‘guix >> lint’ to warn about long patch file names, but to warn a bit less >> frequently than today. >> >> In that spirit, your patch is still relevant and worth applying IMO. > > Sure, although while I'm mucking around... I went ahead and did some > real-world testing of file lengths usable by ustar. > > Using ustar adds a significant buffer, though less than one might think > in the case of guix-VERSION/gnu/packages/patches/*.patch (~151 > characters vs. ~99). > > I'm guessing this is plenty buffer though, most existing patches were > only a theoretical problem... almost to the point that maybe even > removing the check entirely might be fine. Like I wrote, I think it’s good to have this check even if there were no hard limits, mostly as a style guideline. > From c0738574a3571977855d655c157ab0ea0f9be6ef Mon Sep 17 00:00:00 2001 > From: Vagrant Cascadian <vagrant@debian.org> > Date: Fri, 26 Nov 2021 12:13:45 -0800 > Subject: [PATCH] lint: Adjust patch file length check. > > With the switch to "ustar" format in commit > bdf5c16ac052af2ca9d5c3acc4acbc08fd9fdbea, the maximum file length has > increased. > > * guix/lint.scm (check-patch-file-names): Adjust margin used to check for > patch file lengths. Increase allowable patch file length appropriate to new > tar format. Extend warning to explain that long files may break 'make dist'. > * tests/lint.scm: Update tests accordingly. [...] > + (G_ "~a: file name is too long which may break 'make dist'") I think you need a comma before “which”, but other than that LGTM! :-) Thanks, Ludo’.
On 2021-11-28, Ludovic Courtès wrote: > Vagrant Cascadian <vagrant@debian.org> skribis: >> On 2021-11-25, Ludovic Courtès wrote: ... >> From c0738574a3571977855d655c157ab0ea0f9be6ef Mon Sep 17 00:00:00 2001 >> From: Vagrant Cascadian <vagrant@debian.org> >> Date: Fri, 26 Nov 2021 12:13:45 -0800 >> Subject: [PATCH] lint: Adjust patch file length check. >> >> With the switch to "ustar" format in commit >> bdf5c16ac052af2ca9d5c3acc4acbc08fd9fdbea, the maximum file length has >> increased. >> >> * guix/lint.scm (check-patch-file-names): Adjust margin used to check for >> patch file lengths. Increase allowable patch file length appropriate to new >> tar format. Extend warning to explain that long files may break 'make dist'. >> * tests/lint.scm: Update tests accordingly. > > [...] > >> + (G_ "~a: file name is too long which may break 'make dist'") > > I think you need a comma before “which”, but other than that LGTM! :-) Pushed as 5f547a5c425cc99553ea713703e09a8db9f3c38b with the suggested comma, and a brief comment explaining what the magic number "151" was about. That should bright all the patches into compliance with lint as far as file length goes, and should work with the ustar format for the "make dist" produced tarballs. Don't get too crazy with the extra ~50 characters! live well, vagrant
From c0738574a3571977855d655c157ab0ea0f9be6ef Mon Sep 17 00:00:00 2001 From: Vagrant Cascadian <vagrant@debian.org> Date: Fri, 26 Nov 2021 12:13:45 -0800 Subject: [PATCH] lint: Adjust patch file length check. With the switch to "ustar" format in commit bdf5c16ac052af2ca9d5c3acc4acbc08fd9fdbea, the maximum file length has increased. * guix/lint.scm (check-patch-file-names): Adjust margin used to check for patch file lengths. Increase allowable patch file length appropriate to new tar format. Extend warning to explain that long files may break 'make dist'. * tests/lint.scm: Update tests accordingly. --- guix/lint.scm | 9 ++++++--- tests/lint.scm | 8 ++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/guix/lint.scm b/guix/lint.scm index ac2e7b3841..4a5573e86e 100644 --- a/guix/lint.scm +++ b/guix/lint.scm @@ -957,8 +957,11 @@ patch could not be found." ;; Check whether we're reaching tar's maximum file name length. (let ((prefix (string-length (%distro-directory))) - (margin (string-length "guix-2.0.0rc3-10000-1234567890/")) - (max 99)) + ;; Margin approximating the largest path that "make dist" might + ;; create, with a release candidate version, 123456 commits, and + ;; git commit hash abcde0. + (margin (string-length "guix-92.0.0rc3-123456-abcde0/")) + (max 151)) (filter-map (match-lambda ((? string? patch) (if (> (+ margin (if (string-prefix? (%distro-directory) @@ -968,7 +971,7 @@ patch could not be found." max) (make-warning package - (G_ "~a: file name is too long") + (G_ "~a: file name is too long which may break 'make dist'") (list (basename patch)) #:field 'patch-file-names) #f)) diff --git a/tests/lint.scm b/tests/lint.scm index 9a91dd5426..8fa0172cf7 100644 --- a/tests/lint.scm +++ b/tests/lint.scm @@ -506,17 +506,17 @@ (file-name "x.patch"))))))))) (check-patch-file-names pkg))) -(test-equal "patches: file name too long" +(test-equal "patches: file name too long which may break 'make dist'" (string-append "x-" - (make-string 100 #\a) - ".patch: file name is too long") + (make-string 152 #\a) + ".patch: file name is too long which may break 'make dist'") (single-lint-warning-message (let ((pkg (dummy-package "x" (source (dummy-origin (patches (list (string-append "x-" - (make-string 100 #\a) + (make-string 152 #\a) ".patch")))))))) (check-patch-file-names pkg)))) -- 2.30.2