Re: PATCH: silence Shellcheck warning
On Tue, Jan 18, 2022 at 09:50:04PM -0500, Mike Frysinger wrote: > shell does not lend itself to being written correctly. shellcheck > has proven its worth to me multiple times over, especially because i > can stop having to point out the same mistakes to people over & over > that it easily finds. we've integrated into CrOS & internally at > Google and people love it. I agree. In some cases, Shellcheck has corrected my longheld misunderstandings about various shell constructs. I put this down to the fact that many people learn Borne shell by reading the broken scripts of other people rather than a formal book or the man pages. Ben
Re: [PATCH] build: fix race in parallel builds
On 18 Jan 2022 04:53, Mike Frysinger wrote: > As reported by Hongxu Jia: > > The automake-$(APIVERSION) is a hardlink of automake, if it is > > created later than update_mans executing, there is a failure > > [snip] > > |: && mkdir -p doc && ./pre-inst-env /usr/bin/env perl > > ../automake-1.16.1/doc/help2man --output=doc/aclocal-1.16.1 aclocal-1.16 > > |help2man: can't get `--help' info from aclocal-1.16 > > |Try `--no-discard-stderr' if option outputs to stderr > > Makefile:3693: recipe for target 'doc/aclocal-1.16.1' failed > > [snip] > > > > The automake_script is required by update_mans and update_mans > > invokes automake-$(APIVERSION) rather than automake to generate > > doc, so we should assign `automake-$(APIVERSION)' to automake_script. > > > > The same reason to tweak aclocal_script. > > However, rather than update the _script variables to point to the > hardlinked copies of the programs, we can have the help2man steps > run the existing scripts directly. This makes the relationship a > bit more explicit and avoids implicit dependencies on names. > > * doc/local.mk: Pass $(aclocal_script) and $(automake_script) to > $(update_mans). i've pushed this now, and with updated THANKS: Hongxu Jia hongxu@windriver.com let me know if you prefer a different writing of your name, or a different e-mail address. it doesn't have to be pinyin/romanized if you don't want. -mike signature.asc Description: PGP signature
Re: PATCH: silence Shellcheck warning
On 18 Jan 2022 21:25, Ben Elliston wrote: > On Tue, Jan 18, 2022 at 04:18:52AM -0500, Mike Frysinger wrote: > > that said, automake targets lower than POSIX, so i agree that we > > should leave it be for automake specifically. feel like posting a > > patch to suppress this particular check in lib/missing instead ? > > If it's welcome, sure. Some people are opposed to littering pragmas > through source files to disable linters that they think do more harm > than good. tl;dr: using shellcheck directives in automate shell scripts sgtm. shell does not lend itself to being written correctly. shellcheck has proven its worth to me multiple times over, especially because i can stop having to point out the same mistakes to people over & over that it easily finds. we've integrated into CrOS & internally at Google and people love it. at least, people still (rightly) hate shell, but not much to be done about that other than use a better language :P. -mike signature.asc Description: PGP signature
Re: [PATCH] tests: fix py-compile-basedir.sh: add missing test call
On 18 Jan 2022 11:29, Jim Meyering wrote: > On Tue, Jan 18, 2022 at 10:29 AM Mike Frysinger wrote: > > On 18 Jan 2022 09:48, Jim Meyering wrote: > ... > > > But IMHO that's too much duplication/syntax. > > > How about this instead? > > > > > > case $(echo "$files" | wc -l) in 4|6) ;; *) false;; esac > > > > looks reasonable for POSIX shell. not a fan of the one-line, but that style > > seems to be SOP for test code, so i won't whine too loudly :p. > > > > i assume you'll take care of writing the actual patch at this point since it > > was your idea ? :) > > Sure. How about this? lgtm, thanks -mike signature.asc Description: PGP signature
Re: PATCH: silence Shellcheck warning
Ben Elliston wrote: On Tue, Jan 18, 2022 at 04:18:52AM -0500, Mike Frysinger wrote: that said, automake targets lower than POSIX, so i agree that we should leave it be for automake specifically. feel like posting a patch to suppress this particular check in lib/missing instead ? If it's welcome, sure. Some people are opposed to littering pragmas through source files to disable linters that they think do more harm than good. While I agree that Shellcheck's value is questionable, for the issues here there is no need to litter the file with pragmas. Shellcheck will consider a pragma before the first executable line in the file to apply to the entire file; I used this when reverting $() to backticks in config.guess and have included the relevant excerpt below; the"shellcheck disable" line would similarly apply to missing but the rationale would need to be revised to explain the different issues. 8<-- diff --git a/config.guess b/config.guess index d2bdee7..4bc8157 100755 --- a/config.guess +++ b/config.guess @@ -2,6 +2,8 @@ # Attempt to guess a canonical system name. # Copyright 1992-2021 Free Software Foundation, Inc. +# shellcheck disable=SC2006,SC2268 # see below for rationale + timestamp='2021-05-24' # This file is free software; you can redistribute it and/or modify it @@ -32,6 +34,14 @@ timestamp='2021-05-24' # Please send patches to . +# The "shellcheck disable" line above the timestamp inhibits complaints +# about features and limitations of the classic Bourne shell that were +# superseded or lifted in POSIX. However, this script identifies a wide +# variety of pre-POSIX systems that do not have POSIX shells at all, and +# even some reasonably current systems (Solaris 10 as case-in-point) still +# have a pre-POSIX /bin/sh. + + me=$(echo "$0" | sed -e 's,.*/,,') usage="\ 8<-- -- Jacob
Re: minor docs alteration
On Tue, Jan 18, 2022 at 10:01 AM Mike Frysinger wrote:
> On 31 May 2018 22:44, Jefferson Carpenter wrote:
> > Subject: [PATCH] automake.texi: clarify relationship between configure and
> > build dir
> >
> > I know you what this meant, but as a kid this would have confused me - the
> > word "in" seems to imply that the "configure" file is inside of the build
> > dir,
> > as if it should be copied there or something. The word "from" makes it more
> > clear that the intended action is to run "../configure" (or more generally
> > "$SOURCE_DIR/configure") from the build dir.
> >
> > --- a/doc/automake.texi
> > +++ b/doc/automake.texi
> > @@ -866,7 +866,7 @@ The source tree is rooted in the directory containing
> > @file{configure}. It contains all the sources files (those that are
> > distributed), and may be arranged using several subdirectories.
> >
> > -The build tree is rooted in the directory in which @file{configure}
> > +The build tree is rooted in the directory from which @file{configure}
> > was run, and is populated with all object files, programs, libraries,
> > and other derived files built from the sources (and hence not
> > distributed). The build tree usually has the same subdirectory layout
>
> thanks for the patch. it looks like Karl took inspiration from your
> suggestion
> and applied a fix to use "rooted in the current directory at the time
> configure"
> phrasing instead. i don't see a response in the group about it, so this is
> more
> of an FYI :).
> http://git.savannah.gnu.org/cgit/automake.git/commit/?h=8e05f006415d1811785ab5dab1ea4ae5a44c184e
Thanks for going through old patches.
In general, if one of the maintainers writes a diff and does not
attribute any other source, one can almost always assume they wrote
the diff independently. I.e., if Karl or I took inspiration from a bug
report, we would attribute it in the commit log. Giving credit where
due is important, so we try to be diligent about it. Unfortunately, I
have not read enough of the old submitted diffs.
Re: [PATCH] Use gender-neutral pronouns in HACKING and t/README
On Tue, Jan 18, 2022 at 10:03 AM Mike Frysinger wrote: > On 08 Apr 2018 16:07, Matthew Leeds wrote: > > --- > > HACKING | 4 ++-- > > t/README | 2 +- > > thanks for the patch. looks like Jim fixed HACKING: > http://git.savannah.gnu.org/cgit/automake.git/commit/?h=7665b8e209888c73ee4dc05256f4f09a703a01e5 > > but your change to t/README still applies cleanly. Thanks. I've adjusted the commit log and pushed.
Re: [PATCH] tests: fix py-compile-basedir.sh: add missing test call
On Tue, Jan 18, 2022 at 10:29 AM Mike Frysinger wrote:
> On 18 Jan 2022 09:48, Jim Meyering wrote:
...
> > But IMHO that's too much duplication/syntax.
> > How about this instead?
> >
> > case $(echo "$files" | wc -l) in 4|6) ;; *) false;; esac
>
> looks reasonable for POSIX shell. not a fan of the one-line, but that style
> seems to be SOP for test code, so i won't whine too loudly :p.
>
> i assume you'll take care of writing the actual patch at this point since it
> was your idea ? :)
Sure. How about this?
From: Jim Meyering
Date: Tue, 18 Jan 2022 02:00:22 -0800
Subject: [PATCH] tests: fix py-compile-basedir.sh: missing "test"
Prompted by a patch from Thomas Deutschmann ,
via https://lists.gnu.org/r/automake-patches/2022-01/msg1.html:
commit v1.16.1-26-gb279a0d46 ("tests: in python tests, do not
require .pyo files (for python3)") was missing a `test` call.
Reported to Gentoo at https://bugs.gentoo.org/715040.
* t/py-compile-basedir.sh: Rather than just adding the missing
"test", rewrite using a case statement, to avoid some duplication.
---
t/py-compile-basedir.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/py-compile-basedir.sh b/t/py-compile-basedir.sh
index 44b6b07c1..22e605f9d 100644
--- a/t/py-compile-basedir.sh
+++ b/t/py-compile-basedir.sh
@@ -43,7 +43,7 @@ for d in foo foo/bar "$(pwd)/foo" . .. ../foo ''; do
py_installed "$d2/sub/$f.pyc"
files=$(find "$d2" | grep '\.py[co]$')
# with new-enough Python3, there are six files.
- test $(echo "$files" | wc -l) -eq 4 || $(echo "$files" | wc -l) -eq 6
+ case $(echo "$files" | wc -l) in 4|6) ;; *) false;; esac
case $d2 in
.|..) rm -f $files;;
*) rm -rf "$d2";;
--
Re: PATCH: silence Shellcheck warning
On Tue, Jan 18, 2022 at 04:18:52AM -0500, Mike Frysinger wrote: > that said, automake targets lower than POSIX, so i agree that we > should leave it be for automake specifically. feel like posting a > patch to suppress this particular check in lib/missing instead ? If it's welcome, sure. Some people are opposed to littering pragmas through source files to disable linters that they think do more harm than good. Ben
[PATCH] build: fix race in parallel builds
As reported by Hongxu Jia: > The automake-$(APIVERSION) is a hardlink of automake, if it is > created later than update_mans executing, there is a failure > [snip] > |: && mkdir -p doc && ./pre-inst-env /usr/bin/env perl > ../automake-1.16.1/doc/help2man --output=doc/aclocal-1.16.1 aclocal-1.16 > |help2man: can't get `--help' info from aclocal-1.16 > |Try `--no-discard-stderr' if option outputs to stderr > Makefile:3693: recipe for target 'doc/aclocal-1.16.1' failed > [snip] > > The automake_script is required by update_mans and update_mans > invokes automake-$(APIVERSION) rather than automake to generate > doc, so we should assign `automake-$(APIVERSION)' to automake_script. > > The same reason to tweak aclocal_script. However, rather than update the _script variables to point to the hardlinked copies of the programs, we can have the help2man steps run the existing scripts directly. This makes the relationship a bit more explicit and avoids implicit dependencies on names. * doc/local.mk: Pass $(aclocal_script) and $(automake_script) to $(update_mans). --- doc/local.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/local.mk b/doc/local.mk index a29363d2d71b..06c78823a574 100644 --- a/doc/local.mk +++ b/doc/local.mk @@ -46,9 +46,9 @@ update_mans = \ && echo ".so man1/$$f-$(APIVERSION).1" > $@ %D%/aclocal-$(APIVERSION).1: $(aclocal_script) lib/Automake/Config.pm - $(update_mans) aclocal-$(APIVERSION) + $(update_mans) $(aclocal_script) %D%/automake-$(APIVERSION).1: $(automake_script) lib/Automake/Config.pm - $(update_mans) automake-$(APIVERSION) + $(update_mans) $(automake_script) ## This target is not invoked as a dependency of anything. It exists ## merely to make checking the links in automake.texi (that is, -- 2.33.0
Re: [PATCH] tests: fix py-compile-basedir.sh: add missing test call
On 18 Jan 2022 09:48, Jim Meyering wrote:
> On Tue, Jan 18, 2022 at 7:46 AM Mike Frysinger wrote:
> > From: Thomas Deutschmann
> >
> > Commit b279a0d46dfeca1ca40057c3c910ab1657d60be5 ("tests: in python
> > tests, do not require .pyo files (for python3)") had a slight logic
> > error in that it missed a `test` call.
> >
> > Reported to Gentoo at https://bugs.gentoo.org/715040.
> >
> > * t/py-compile-basedir.sh: Add test command.
> > ---
> > t/py-compile-basedir.sh | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/py-compile-basedir.sh b/t/py-compile-basedir.sh
> > index 44b6b07c1962..979f65710c0b 100644
> > --- a/t/py-compile-basedir.sh
> > +++ b/t/py-compile-basedir.sh
> > @@ -43,7 +43,7 @@ for d in foo foo/bar "$(pwd)/foo" . .. ../foo ''; do
> >py_installed "$d2/sub/$f.pyc"
> >files=$(find "$d2" | grep '\.py[co]$')
> ># with new-enough Python3, there are six files.
> > - test $(echo "$files" | wc -l) -eq 4 || $(echo "$files" | wc -l) -eq 6
> > + test $(echo "$files" | wc -l) -eq 4 || test $(echo "$files" | wc -l) -eq
> > 6
>
> Thanks. Good catch.
> If we were to use that, it's a little better to double-quote each
> $(...) result, in case somehow the result is not just precisely one
> token:
>
> test "$(echo "$files" | wc -l)" -eq 4 || test "$(echo "$files" | wc -l)"
> -eq 6
>
> But IMHO that's too much duplication/syntax.
> How about this instead?
>
> case $(echo "$files" | wc -l) in 4|6) ;; *) false;; esac
looks reasonable for POSIX shell. not a fan of the one-line, but that style
seems to be SOP for test code, so i won't whine too loudly :p.
i assume you'll take care of writing the actual patch at this point since it
was your idea ? :)
-mike
signature.asc
Description: PGP signature
Re: PATCH: silence Shellcheck warning
On 22 Jul 2018 14:41, Nick Bowler wrote: > On 7/21/18, Ben Elliston wrote: > > This patch silences a warning from Shellcheck about using old-style > > `...` command substitutions. > [...] > > commit 4d35c7aae97234bf055519075ef03cd4090a1dfc > > Author: Ben Elliston > > Date: Sun Jul 22 08:22:44 2018 +1000 > > > > * missing: Use $(..) command substitution syntax. > > Several shells do not support $(...) substitutions so portable scripts > like 'missing' must always use backticks. I would suggest that this > linter warning is spurious if not outright counterproductive. Shellcheck is doing the right thing. it targets POSIX, not ancient shells that the vast majority people should never waste their time on. that said, automake targets lower than POSIX, so i agree that we should leave it be for automake specifically. feel like posting a patch to suppress this particular check in lib/missing instead ? -mike signature.asc Description: PGP signature
Re: [PATCH] doc: Fix a typo
On 08 Apr 2018 16:06, Matthew Leeds wrote: > --- a/doc/automake.texi > +++ b/doc/automake.texi > All of these actions are performed in a temporary directory. Please > note that the exact location and the exact structure of such a directory > (where the read-only sources are placed, how the temporary build and > install directories are named and how deeply they are nested, etc.) is > to be considered an implementation detail, which can change at any time; > -so do not reply on it. > +so do not rely on it. thanks for the patch. this has been fixed in a larger cleanup: http://git.savannah.gnu.org/cgit/automake.git/commit/?h=6624f88b2ea685e5c44c7373d01df488d1dabd19 -mike signature.asc Description: PGP signature
Re: [PATCH] lib: drop unused shell variables
lgtm, and still applies cleanly -mike signature.asc Description: PGP signature
Re: [PATCH] Use gender-neutral pronouns in HACKING and t/README
On 08 Apr 2018 16:07, Matthew Leeds wrote: > --- > HACKING | 4 ++-- > t/README | 2 +- thanks for the patch. looks like Jim fixed HACKING: http://git.savannah.gnu.org/cgit/automake.git/commit/?h=7665b8e209888c73ee4dc05256f4f09a703a01e5 but your change to t/README still applies cleanly. -mike signature.asc Description: PGP signature
Re: minor docs alteration
On 31 May 2018 22:44, Jefferson Carpenter wrote:
> Subject: [PATCH] automake.texi: clarify relationship between configure and
> build dir
>
> I know you what this meant, but as a kid this would have confused me - the
> word "in" seems to imply that the "configure" file is inside of the build dir,
> as if it should be copied there or something. The word "from" makes it more
> clear that the intended action is to run "../configure" (or more generally
> "$SOURCE_DIR/configure") from the build dir.
>
> --- a/doc/automake.texi
> +++ b/doc/automake.texi
> @@ -866,7 +866,7 @@ The source tree is rooted in the directory containing
> @file{configure}. It contains all the sources files (those that are
> distributed), and may be arranged using several subdirectories.
>
> -The build tree is rooted in the directory in which @file{configure}
> +The build tree is rooted in the directory from which @file{configure}
> was run, and is populated with all object files, programs, libraries,
> and other derived files built from the sources (and hence not
> distributed). The build tree usually has the same subdirectory layout
thanks for the patch. it looks like Karl took inspiration from your suggestion
and applied a fix to use "rooted in the current directory at the time configure"
phrasing instead. i don't see a response in the group about it, so this is more
of an FYI :).
http://git.savannah.gnu.org/cgit/automake.git/commit/?h=8e05f006415d1811785ab5dab1ea4ae5a44c184e
-mike
signature.asc
Description: PGP signature
Re: [PATCH] tests: fix py-compile-basedir.sh: add missing test call
On Tue, Jan 18, 2022 at 7:46 AM Mike Frysinger wrote:
> From: Thomas Deutschmann
>
> Commit b279a0d46dfeca1ca40057c3c910ab1657d60be5 ("tests: in python
> tests, do not require .pyo files (for python3)") had a slight logic
> error in that it missed a `test` call.
>
> Reported to Gentoo at https://bugs.gentoo.org/715040.
>
> * t/py-compile-basedir.sh: Add test command.
> ---
> t/py-compile-basedir.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/py-compile-basedir.sh b/t/py-compile-basedir.sh
> index 44b6b07c1962..979f65710c0b 100644
> --- a/t/py-compile-basedir.sh
> +++ b/t/py-compile-basedir.sh
> @@ -43,7 +43,7 @@ for d in foo foo/bar "$(pwd)/foo" . .. ../foo ''; do
>py_installed "$d2/sub/$f.pyc"
>files=$(find "$d2" | grep '\.py[co]$')
># with new-enough Python3, there are six files.
> - test $(echo "$files" | wc -l) -eq 4 || $(echo "$files" | wc -l) -eq 6
> + test $(echo "$files" | wc -l) -eq 4 || test $(echo "$files" | wc -l) -eq 6
Thanks. Good catch.
If we were to use that, it's a little better to double-quote each
$(...) result, in case somehow the result is not just precisely one
token:
test "$(echo "$files" | wc -l)" -eq 4 || test "$(echo "$files" | wc -l)" -eq 6
But IMHO that's too much duplication/syntax.
How about this instead?
case $(echo "$files" | wc -l) in 4|6) ;; *) false;; esac
