[-- Attachment #1: Type: text/plain, Size: 3556 bytes --] When running "make dist", guix apparently uses a very old tar format (presumably for maximal compatibility with tar implementations), which has a hard limit of 99 characters. For this reason, there is a guix lint check for filename lengths, but there are quite a few lint warnings in guix master and core-updates-frozen. On guix master db5907138cbf9139e885fa4b3860d604aff0be9c, there appear to be 14 such patches: $ guix lint --checkers=patch-file-names | grep 'too long' | nl 1 gnu/packages/admin.scm:2669:2: debops@1.1.0: debops-constants-for-external-program-names.patch: file name is too long 2 gnu/packages/astronomy.scm:865:2: xplanet@1.3.1: xplanet-1.3.1-libdisplay_DisplayOutput.cpp.patch: file name is too long 3 gnu/packages/astronomy.scm:865:2: xplanet@1.3.1: xplanet-1.3.1-xpUtil-Add2017LeapSecond.cpp.patch: file name is too long 4 gnu/packages/geo.scm:312:2: libgeotiff@1.5.1: libgeotiff-adapt-test-script-for-proj-6.2.patch: file name is too long 5 gnu/packages/glib.scm:478:2: gobject-introspection@1.62.0: gobject-introspection-absolute-shlib-path.patch: file name is too long 6 gnu/packages/java.scm:9032:2: java-tunnelvisionlabs-antlr4-runtime@4.7.4: java-tunnelvisionlabs-antlr-code-too-large.patch: file name is too long 7 gnu/packages/java.scm:8926:2: java-tunnelvisionlabs-antlr4-runtime-annotations@4.7.4: java-tunnelvisionlabs-antlr-code-too-large .patch: file name is too long 8 gnu/packages/java.scm:13120:2: java-apache-ivy@2.4.0: java-apache-ivy-port-to-latest-bouncycastle.patch: file name is too long 9 gnu/packages/java.scm:9040:2: java-tunnelvisionlabs-antlr4@4.7.4: java-tunnelvisionlabs-antlr-code-too-large.patch: file name is too long 10 gnu/packages/kde-frameworks.scm:3419:2: plasma-framework@5.70.1: plasma-framework-fix-KF5PlasmaMacros.cmake.patch: file name is too long 11 gnu/packages/llvm.scm:98:2: clang-runtime@3.9.1: clang-runtime-3.9-libsanitizer-mode-field.patch: file name is too long 12 gnu/packages/llvm.scm:852:4: clang-runtime@3.5.2: clang-runtime-3.5-libsanitizer-mode-field.patch: file name is too long 13 gnu/packages/llvm.scm:98:2: clang-runtime@3.7.1: clang-runtime-3.8-libsanitizer-mode-field.patch: file name is too long 14 gnu/packages/llvm.scm:98:2: clang-runtime@3.8.1: clang-runtime-3.8-libsanitizer-mode-field.patch: file name is too long Looks like a similar number of patches on core-updates-frozen, too, probably mostly the same ones. Luckily, the lint check is conservative and actually gives a bit of wiggle-room, and none of these appear to break "make dist" from generating a tarball at the moment, but there was one in core-updates-frozen recently that did break "make dist" (fixed in 6cdf4e5bf230fdbe17e592c2ec74fb34dba70eb5). Ideally, "guix lint" would be run and issues fixed before applying patches ... ! Is it worth adding an inexpensive check to etc/git/pre-push that also checks for file-length and fails to push due to this issue potentially breaking "make dist"? A different angle might be to actually use a different tar format: https://www.gnu.org/software/tar/manual/html_section/Formats.html I would guess "make dist" is using the tar "v7" format, based on the 99 character length limit for files. Most of the other formats have no file length limit or a longer limit. Well, thanks for following my rambling this far! live well, vagrant [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 227 bytes --]
Vagrant Cascadian <vagrant@debian.org> skribis: > Ideally, "guix lint" would be run and issues fixed before applying > patches ... ! On the bright side, that there’s just a dozen of issues on 20K packages suggests it’s usually run. :-) I think we’re used to running it for new packages but not when modifying an existing package, which is probably when issues like that are introduced. > Is it worth adding an inexpensive check to etc/git/pre-push that also > checks for file-length and fails to push due to this issue potentially > breaking "make dist"? Could be. > A different angle might be to actually use a different tar format: > > https://www.gnu.org/software/tar/manual/html_section/Formats.html > > I would guess "make dist" is using the tar "v7" format, based on the 99 > character length limit for files. Most of the other formats have no file > length limit or a longer limit. Yes, we could also do that. Thanks, Ludo’.
[-- Attachment #1: Type: text/plain, Size: 2218 bytes --] On 2021-11-17, Ludovic Courtès wrote: > Vagrant Cascadian <vagrant@debian.org> skribis: > >> Ideally, "guix lint" would be run and issues fixed before applying >> patches ... ! > > On the bright side, that there’s just a dozen of issues on 20K packages > suggests it’s usually run. :-) > > I think we’re used to running it for new packages but not when modifying > an existing package, which is probably when issues like that are > introduced. Sounds plausible. My guess is this is triggered from folks using "git format-patch" and dumping the files into gnu/packages/patches, which probably has a default length that is a little too long in this case. Another option that would help a little would be to drop the .patch suffix, it's kind of redundant to have gnu/packages/patches/*.patch >> Is it worth adding an inexpensive check to etc/git/pre-push that also >> checks for file-length and fails to push due to this issue potentially >> breaking "make dist"? > > Could be. This basically mimics the check that guix-lint does: for p in $(find gnu/packages/patches -type f ) ; do if [ "$(echo guix-2.0.0rc3-10000-1234567890/${p} | wc -c)" -ge "99" ] then echo $p exit 1 fi done Would something like that be cheap enough to consider adding to etc/git/pre-push? Are "find" and "wc" reasonable dependencies to assume they are available? Obviously, have to wait until they are all fixed, some of which probably require going through core-updates... or start with a more conservative but still useful length-check. >> A different angle might be to actually use a different tar format: >> >> https://www.gnu.org/software/tar/manual/html_section/Formats.html >> >> I would guess "make dist" is using the tar "v7" format, based on the 99 >> character length limit for files. Most of the other formats have no file >> length limit or a longer limit. > > Yes, we could also do that. Struggling to figure out how to do that; seems automake is very inclined to use the old format... anyone with sufficient auto* skills to try and upgrade the "make dist" to pass one of the newer --format= arguments to tar? live well, vagrant [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 227 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2429 bytes --] On 2021-11-17, Vagrant Cascadian wrote: > On 2021-11-17, Ludovic Courtès wrote: >> Vagrant Cascadian <vagrant@debian.org> skribis: >> >>> Ideally, "guix lint" would be run and issues fixed before applying >>> patches ... ! ... >>> Is it worth adding an inexpensive check to etc/git/pre-push that also >>> checks for file-length and fails to push due to this issue potentially >>> breaking "make dist"? ... > This basically mimics the check that guix-lint does: > > for p in $(find gnu/packages/patches -type f ) ; do > if [ "$(echo guix-2.0.0rc3-10000-1234567890/${p} | wc -c)" -ge "99" ] > then > echo $p > exit 1 > fi > done > > Would something like that be cheap enough to consider adding to > etc/git/pre-push? Are "find" and "wc" reasonable dependencies to assume > they are available? > > Obviously, have to wait until they are all fixed, some of which probably > require going through core-updates... or start with a more conservative > but still useful length-check. Another strategy would be to reduce the overly cautious lint check: diff --git a/guix/lint.scm b/guix/lint.scm index ac2e7b3841..e795c466b1 100644 --- a/guix/lint.scm +++ b/guix/lint.scm @@ -957,7 +957,7 @@ (define (starts-with-package-name? file-name) ;; 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 (string-length "guix-2.0.0rc3-10000-12345678/")) (max 99)) (filter-map (match-lambda ((? string? patch) That leaves only two packages on master in violation of the lint check, and those are both updatable directly on master, and "make dist" still works with those two packages as they are, in my experience. I think that's fairly safe to do, actually, as "make dist" on core-updates-frozen currently produces a tarball prefixed with: guix-1.3.0.10380-fe257/ If guix keeps bumping it's version into the double-digits, an rc-version, and it surpasses 99999 commits, and an extra character for the git commit hash, this still leaves considerable wiggle-room: guix-10.0.0rc0-123456-abcde0/ vs. guix-2.0.0rc3-10000-12345678/ So, I guess I'm leaning towards making the guix lint check a little more lenient. Thoughts? live well, vagrant [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 227 bytes --]
Hello,
Vagrant Cascadian <vagrant@debian.org> skribis:
> Another strategy would be to reduce the overly cautious lint check:
>
> diff --git a/guix/lint.scm b/guix/lint.scm
> index ac2e7b3841..e795c466b1 100644
> --- a/guix/lint.scm
> +++ b/guix/lint.scm
> @@ -957,7 +957,7 @@ (define (starts-with-package-name? file-name)
>
> ;; 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 (string-length "guix-2.0.0rc3-10000-12345678/"))
> (max 99))
> (filter-map (match-lambda
> ((? string? patch)
>
>
> That leaves only two packages on master in violation of the lint check,
> and those are both updatable directly on master, and "make dist" still
> works with those two packages as they are, in my experience.
>
> I think that's fairly safe to do, actually, as "make dist" on
> core-updates-frozen currently produces a tarball prefixed with:
>
> guix-1.3.0.10380-fe257/
>
> If guix keeps bumping it's version into the double-digits, an
> rc-version, and it surpasses 99999 commits, and an extra character for
> the git commit hash, this still leaves considerable wiggle-room:
>
> guix-10.0.0rc0-123456-abcde0/
> vs.
> guix-2.0.0rc3-10000-12345678/
>
> 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)!
Ludo’.
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)!
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.
If instead `guix lint` is telling us about a hard limit that will break
things, I think it should say so clearly.
-Philip
[-- Attachment #1: Type: text/plain, Size: 2314 bytes --] 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 > 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 227 bytes --]
Hello,
[...]
>>> A different angle might be to actually use a different tar format:
>>>
>>> https://www.gnu.org/software/tar/manual/html_section/Formats.html
>>>
>>> I would guess "make dist" is using the tar "v7" format, based on the 99
>>> character length limit for files. Most of the other formats have no file
>>> length limit or a longer limit.
>>
>> Yes, we could also do that.
>
> Struggling to figure out how to do that; seems automake is very inclined
> to use the old format... anyone with sufficient auto* skills to try and
> upgrade the "make dist" to pass one of the newer --format= arguments to
> tar?
Reading the Automake manual (info (automake) List of Automake options) I
stumbled on this:
‘filename-length-max=99’
Abort if file names longer than 99 characters are found during
‘make dist’. Such long file names are generally considered not to
be portable in tarballs. See the ‘tar-v7’ and ‘tar-ustar’ options
below. This option should be used in the top-level ‘Makefile.am’
or as an argument of ‘AM_INIT_AUTOMAKE’ in ‘configure.ac’; it will
be ignored otherwise. It will also be ignored in sub-packages of
nested packages (*note Subpackages::).
This makes me think that Automake is simply configured out of the box to
keep the file names as portable as possible (it doesn't mean it uses
tar-v7 itself, IIUC, though I haven't checked).
Seems a good thing to be as portable as can be, especially since 200
chars patch file names wouldn't look good in the sources anyway ;-).
Thanks,
Maxim
[-- Attachment #1: Type: text/plain, Size: 1708 bytes --] Hi! Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > Reading the Automake manual (info (automake) List of Automake options) I > stumbled on this: > > ‘filename-length-max=99’ > Abort if file names longer than 99 characters are found during > ‘make dist’. Such long file names are generally considered not to > be portable in tarballs. See the ‘tar-v7’ and ‘tar-ustar’ options > below. This option should be used in the top-level ‘Makefile.am’ > or as an argument of ‘AM_INIT_AUTOMAKE’ in ‘configure.ac’; it will > be ignored otherwise. It will also be ignored in sub-packages of > nested packages (*note Subpackages::). > > This makes me think that Automake is simply configured out of the box to > keep the file names as portable as possible (it doesn't mean it uses > tar-v7 itself, IIUC, though I haven't checked). Oh, looks like we could add the ‘tar-ustar’ option and be done with it (info "(automake) List of Automake options"): ‘tar-ustar’ selects the ustar format defined by POSIX 1003.1-1988. This format is old enough to be portable: As of 2018, it is supported by the native ‘tar’ command on GNU, FreeBSD, NetBSD, OpenBSD, AIX, HP-UX, and Solaris, at least. It fully supports empty directories. It can store file names with up to 256 characters, provided that the file name can be split at directory separator in two parts, first of them being at most 155 bytes long. So, in most cases the maximum file name length will be shorter than 256 characters. Any objections against the patch below? [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Type: text/x-patch, Size: 448 bytes --] diff --git a/configure.ac b/configure.ac index 84592f6041..341cff8fbd 100644 --- a/configure.ac +++ b/configure.ac @@ -8,7 +8,7 @@ AC_INIT([GNU Guix], [https://www.gnu.org/software/guix/]) AC_CONFIG_AUX_DIR([build-aux]) -AM_INIT_AUTOMAKE([1.14 gnu silent-rules subdir-objects \ +AM_INIT_AUTOMAKE([1.14 gnu tar-ustar silent-rules subdir-objects \ color-tests parallel-tests -Woverride -Wno-portability]) # Enable silent rules by default. [-- Attachment #3: Type: text/plain, Size: 1735 bytes --] I tried it and there’s no tar warning and the file looks just fine, including the long file names: --8<---------------cut here---------------start------------->8--- $ make dist -j5 […] make[2]: Leaving directory '/home/ludo/src/guix' tardir=guix-1.3.0.8612-7cad44-dirty && tar --format=ustar -chf - "$tardir" | eval GZIP= gzip --best -c >guix-1.3.0.8612-7cad44-dirty.tar.gz make[1]: Leaving directory '/home/ludo/src/guix' if test -d "guix-1.3.0.8612-7cad44-dirty"; then find "guix-1.3.0.8612-7cad44-dirty" -type d ! -perm -200 -exec chmod u+w {} ';' && rm -rf "guix-1.3.0.8612-7cad44-dirty" || { sleep 5 && rm -rf "guix-1.3.0.8612-7cad44-dirty"; }; else :; fi $ tar tvf guix-1.3.0.8612-7cad44-dirty.tar.gz |grep patches/xplanet -rw-r--r-- ludo/users 1252 2020-04-06 00:14 guix-1.3.0.8612-7cad44-dirty/gnu/packages/patches/xplanet-1.3.1-libimage_gif.c.patch -rw-r--r-- ludo/users 765 2020-04-06 00:14 guix-1.3.0.8612-7cad44-dirty/gnu/packages/patches/xplanet-1.3.1-xpUtil-Add2017LeapSecond.cpp.patch -rw-r--r-- ludo/users 6990 2020-04-06 00:14 guix-1.3.0.8612-7cad44-dirty/gnu/packages/patches/xplanet-1.3.1-cxx11-eof.patch -rw-r--r-- ludo/users 783 2020-04-06 00:14 guix-1.3.0.8612-7cad44-dirty/gnu/packages/patches/xplanet-1.3.1-libdisplay_DisplayOutput.cpp.patch --8<---------------cut here---------------end--------------->8--- > Seems a good thing to be as portable as can be, especially since 200 > chars patch file names wouldn't look good in the sources anyway ;-). I don’t think we care about “portability” as understood in the context of Automake, which roughly translates to “support SunOS 2 and HP-UX’s 1991 ‘tar’ implementation”. :-) Thanks, Ludo’.
Hi Ludo,
Ludovic Courtès <ludo@gnu.org> writes:
[...]
> Any objections against the patch below?
>
> diff --git a/configure.ac b/configure.ac
> index 84592f6041..341cff8fbd 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -8,7 +8,7 @@ AC_INIT([GNU Guix],
> [https://www.gnu.org/software/guix/])
> AC_CONFIG_AUX_DIR([build-aux])
>
> -AM_INIT_AUTOMAKE([1.14 gnu silent-rules subdir-objects \
> +AM_INIT_AUTOMAKE([1.14 gnu tar-ustar silent-rules subdir-objects \
> color-tests parallel-tests -Woverride -Wno-portability])
>
> # Enable silent rules by default.
>
>
> I tried it and there’s no tar warning and the file looks just fine,
> including the long file names:
>
> $ make dist -j5
> […]
> make[2]: Leaving directory '/home/ludo/src/guix'
> tardir=guix-1.3.0.8612-7cad44-dirty && tar --format=ustar -chf - "$tardir" | eval GZIP= gzip --best -c >guix-1.3.0.8612-7cad44-dirty.tar.gz
> make[1]: Leaving directory '/home/ludo/src/guix'
> if test -d "guix-1.3.0.8612-7cad44-dirty"; then find "guix-1.3.0.8612-7cad44-dirty" -type d ! -perm -200 -exec chmod u+w {} ';' && rm -rf "guix-1.3.0.8612-7cad44-dirty" || { sleep 5 && rm -rf "guix-1.3.0.8612-7cad44-dirty"; }; else :; fi
> $ tar tvf guix-1.3.0.8612-7cad44-dirty.tar.gz |grep patches/xplanet
> -rw-r--r-- ludo/users 1252 2020-04-06 00:14 guix-1.3.0.8612-7cad44-dirty/gnu/packages/patches/xplanet-1.3.1-libimage_gif.c.patch
> -rw-r--r-- ludo/users 765 2020-04-06 00:14 guix-1.3.0.8612-7cad44-dirty/gnu/packages/patches/xplanet-1.3.1-xpUtil-Add2017LeapSecond.cpp.patch
> -rw-r--r-- ludo/users 6990 2020-04-06 00:14 guix-1.3.0.8612-7cad44-dirty/gnu/packages/patches/xplanet-1.3.1-cxx11-eof.patch
> -rw-r--r-- ludo/users 783 2020-04-06 00:14 guix-1.3.0.8612-7cad44-dirty/gnu/packages/patches/xplanet-1.3.1-libdisplay_DisplayOutput.cpp.patch
>
>> Seems a good thing to be as portable as can be, especially since 200
>> chars patch file names wouldn't look good in the sources anyway ;-).
>
> I don’t think we care about “portability” as understood in the context
> of Automake, which roughly translates to “support SunOS 2 and HP-UX’s
> 1991 ‘tar’ implementation”. :-)
Hehe.
The patch LGTM.
Thanks!
Maxim
[-- Attachment #1: Type: text/plain, Size: 2495 bytes --] On 2021-11-19, Vagrant Cascadian wrote: > 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 Er, I meant: https://issues.guix.gnu.org/51985 >> 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 227 bytes --]