lint: Adjust patch file length check.

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Vagrant Cascadian
Owner
unassigned
Submitted by
Vagrant Cascadian
Severity
normal
V
V
Vagrant Cascadian wrote on 19 Nov 2021 22:05
(address . guix-patches@gnu.org)
875ysnsvnt.fsf@yucca
The current guix lint check is a bit overly conservative, and reports
several results which do not in practice actually cause issues.

This patch proposes to reduce the size by two characters (leaving only
two patches on guix master that need to be adjusted), uses a version
string more like what actually might be included in a tarball built
using "make dist", and adds a comment describing what the arbitrary
string actually is supposed to represent.

This should still even leave a little wiggle-room when guix hits version
100+ and/or 1000000+ commits, by which time hopefully guix has switched
to a tarball format that doesn't have such a short arbitrary file length
limit!


live well,
vagrant
From 6ad2050a8bbc308a328d30d4f66cb229d868b79d Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian <vagrant@debian.org>
Date: Fri, 19 Nov 2021 12:14:19 -0800
Subject: [PATCH] lint: Adjust patch file length check.

* guix/lint.scm (check-patch-file-names): Adjust margin used to check for
patch file lengths.
---
guix/lint.scm | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

Toggle diff (18 lines)
diff --git a/guix/lint.scm b/guix/lint.scm
index ac2e7b3841..39b4a2ae85 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -957,7 +957,10 @@ 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/"))
+ ;; 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-12.0.0rc3-123456-abcde0/"))
(max 99))
(filter-map (match-lambda
((? string? patch)
--
2.30.2
-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQRlgHNhO/zFx+LkXUXcUY/If5cWqgUCYZgRtgAKCRDcUY/If5cW
qup5AQD81KiqA/DfiPpt9xQTyb4qvkp++22qCv9Wgl0Di+JB3wD/TyO2cTC2XkoX
dGk/JnNlOcLquHtSuQ4FmA0thjC/fgw=
=qx7F
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 22 Nov 2021 12:22
(name . Vagrant Cascadian)(address . vagrant@debian.org)(address . 51985@debbugs.gnu.org)
877dd0ph9g.fsf@gnu.org
Hi,

Vagrant Cascadian <vagrant@debian.org> skribis:

Toggle quote (8 lines)
> From 6ad2050a8bbc308a328d30d4f66cb229d868b79d Mon Sep 17 00:00:00 2001
> From: Vagrant Cascadian <vagrant@debian.org>
> Date: Fri, 19 Nov 2021 12:14:19 -0800
> Subject: [PATCH] lint: Adjust patch file length check.
>
> * guix/lint.scm (check-patch-file-names): Adjust margin used to check for
> patch file lengths.

Make sure “make check TESTS=tests/lint.scm” is still happy, but
otherwise LGTM.

Thanks!

Ludo’.
V
V
Vagrant Cascadian wrote on 24 Nov 2021 22:25
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 51985@debbugs.gnu.org)
8735nl5jri.fsf@ponder
On 2021-11-22, Ludovic Courtès wrote:
Toggle quote (12 lines)
> Vagrant Cascadian <vagrant@debian.org> skribis:
>> From 6ad2050a8bbc308a328d30d4f66cb229d868b79d Mon Sep 17 00:00:00 2001
>> From: Vagrant Cascadian <vagrant@debian.org>
>> Date: Fri, 19 Nov 2021 12:14:19 -0800
>> Subject: [PATCH] lint: Adjust patch file length check.
>>
>> * guix/lint.scm (check-patch-file-names): Adjust margin used to check for
>> patch file lengths.
>
> Make sure “make check TESTS=tests/lint.scm” is still happy, but
> otherwise LGTM.

With:

commit bdf5c16ac052af2ca9d5c3acc4acbc08fd9fdbea
Author: Ludovic Courtès <ludo@gnu.org>
Date: Tue Nov 23 09:06:49 2021 +0100

maint: "make dist" builds tarballs in 'ustar' format.

It seems like this actually needs even further updates, as that should
allow for a much longer file length in general (although a little
difficult to figure out the exact file length allowed).

And then the corresponding test suite will need changes as well...

live well,
vagrant
-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQRlgHNhO/zFx+LkXUXcUY/If5cWqgUCYZ6ttwAKCRDcUY/If5cW
qpMmAQDYNL6jp9K4UgGRdzrz8iurXUAL0cHACYmM4fV/CA5fLQEA0lyijddpNmXt
RpBO3nHkd7z/pJ//EiM2+3EkwaAedwM=
=19+k
-----END PGP SIGNATURE-----

V
V
Vagrant Cascadian wrote on 24 Nov 2021 22:27
Re: default tar format for "make dist" and patch file length
87zgpt453s.fsf@ponder
On 2021-11-19, Vagrant Cascadian wrote:
Toggle quote (13 lines)
> On 2021-11-19, Philip McGrath wrote:
>> On 11/19/21 09:54, Ludovic Courtès wrote:
>>> Vagrant Cascadian <vagrant@debian.org> skribis:
>>>> So, I guess I'm leaning towards making the guix lint check a little more
>>>> lenient.
>>>>
>>>> Thoughts?
>>>
>>> That sounds even better, I’m all for it (changing (guix lint) + fixing
>>> the two remaining issues)!
>
> Submitted the guix lint change as https://issues.guix.gnu.org/51775



Toggle quote (47 lines)
>> It might also help to change the warning given by the check.
>>
>> When a program called "lint" tells me that something is too long, I
>> understand that to mean that what I've done is generally considered bad
>> style, but there might be a very good reason to do it in some specific
>> case. For example, I might exceed a line length guideline to avoid
>> inserting linebreaks into a URL.
>
> That's a good point!
>
>
>> If instead `guix lint` is telling us about a hard limit that will break
>> things, I think it should say so clearly.
>
> Not sure how to convey succinctly, but here's an attempt at a patch
> (which ironically also probably makes the line a bit too long in the
> code):
>
> diff --git a/guix/lint.scm b/guix/lint.scm
> index ac2e7b3841..6464fb751a 100644
> --- a/guix/lint.scm
> +++ b/guix/lint.scm
> @@ -968,7 +968,7 @@ (define (starts-with-package-name? file-name)
> max)
> (make-warning
> package
> - (G_ "~a: file name is too long")
> + (G_ "~a: file name is too long and may break release tarball generation")
> (list (basename patch))
> #:field 'patch-file-names)
> #f))
> diff --git a/tests/lint.scm b/tests/lint.scm
> index 9a91dd5426..d4c3d62aaf 100644
> --- a/tests/lint.scm
> +++ b/tests/lint.scm
> @@ -509,7 +509,7 @@ (define hsab (string-append (assoc-ref inputs
> "hsab")
> (test-equal "patches: file name too long"
> (string-append "x-"
> (make-string 100 #\a)
> - ".patch: file name is too long")
> + ".patch: file name is too long and may break release tarball generation")
> (single-lint-warning-message
> (let ((pkg (dummy-package
> "x"


live well,
vagrant
-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQRlgHNhO/zFx+LkXUXcUY/If5cWqgUCYZ6uKwAKCRDcUY/If5cW
qlNPAQCKSp3tT9jJrGVN6CQqrVTvtXBEYopDyUtXS6Px0WRSegEAqfVuvxPRoVRv
VA5clCc8ynDcuDWATqqpgZGvkGgoCg8=
=kuOO
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 25 Nov 2021 14:08
Re: bug#51985: lint: Adjust patch file length check.
(name . Vagrant Cascadian)(address . vagrant@debian.org)(address . 51985@debbugs.gnu.org)
8735nke61x.fsf@gnu.org
Hi,

Vagrant Cascadian <vagrant@debian.org> skribis:

Toggle quote (14 lines)
> With:
>
> commit bdf5c16ac052af2ca9d5c3acc4acbc08fd9fdbea
> Author: Ludovic Courtès <ludo@gnu.org>
> Date: Tue Nov 23 09:06:49 2021 +0100
>
> maint: "make dist" builds tarballs in 'ustar' format.
>
> It seems like this actually needs even further updates, as that should
> allow for a much longer file length in general (although a little
> difficult to figure out the exact file length allowed).
>
> And then the corresponding test suite will need changes as well...

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.

Thoughts?

Ludo’.
V
V
Vagrant Cascadian wrote on 26 Nov 2021 22:08
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ee72fwv0.fsf@ponder
On 2021-11-25, Ludovic Courtès wrote:
Toggle quote (22 lines)
> Vagrant Cascadian <vagrant@debian.org> skribis:
>
>> With:
>>
>> commit bdf5c16ac052af2ca9d5c3acc4acbc08fd9fdbea
>> Author: Ludovic Courtès <ludo@gnu.org>
>> Date: Tue Nov 23 09:06:49 2021 +0100
>>
>> maint: "make dist" builds tarballs in 'ustar' format.
>>
>> It seems like this actually needs even further updates, as that should
>> allow for a much longer file length in general (although a little
>> difficult to figure out the exact file length allowed).
>>
>> And then the corresponding test suite will need changes as well...
>
> 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.

Updated patch integrating this and the stronger warning suggested by
Philip McGrath.

I haven't yet actually *tested* this updated patch yet, but the previous
iteration tested just fine. :)


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(-)

Toggle diff (55 lines)
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
-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQRlgHNhO/zFx+LkXUXcUY/If5cWqgUCYaFM6AAKCRDcUY/If5cW
qrj8AQD02tkebDhmrwJhDJUUy/Ya8U3RnzVIobf7lumScSirtgEAiZCehNi6fGBv
IT1c4uugIN52Oh2OqM+EsPJRbAxZOgM=
=3hkQ
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 28 Nov 2021 18:02
(name . Vagrant Cascadian)(address . vagrant@debian.org)
87lf18b4dp.fsf@gnu.org
Hi!

Vagrant Cascadian <vagrant@debian.org> skribis:

Toggle quote (2 lines)
> On 2021-11-25, Ludovic Courtès wrote:

[...]

Toggle quote (17 lines)
>> 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.

Toggle quote (14 lines)
> 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.

[...]

Toggle quote (2 lines)
> + (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’.
V
V
Vagrant Cascadian wrote on 18 Dec 2021 09:26
(name . Ludovic Courtès)(address . ludo@gnu.org)
87pmpu8gj9.fsf@ponder
On 2021-11-28, Ludovic Courtès wrote:
Toggle quote (2 lines)
> Vagrant Cascadian <vagrant@debian.org> skribis:
>> On 2021-11-25, Ludovic Courtès wrote:
...
Toggle quote (20 lines)
>> 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
-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQRlgHNhO/zFx+LkXUXcUY/If5cWqgUCYb2bTgAKCRDcUY/If5cW
qsnMAQDtqRlxdhb0TlUbiNPakFExNOK2GUTkQ8I5+7KW3Js4kAD/QKRsfe1tEqqO
lidYhHvSCxGFGgcTpRv8VZcW/laotQ0=
=s4KP
-----END PGP SIGNATURE-----

Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 51985@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 51985
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch