[PATCH 1/2] copy-firmware: make script smarter about parameters

[PATCH 1/2] copy-firmware: make script smarter about parameters

[Timur Tabi]

Several improvements to copy-firmware.sh that make it more friendly
when passed unknown or not exactly correct command-line parameters.

1) Add a usage() function to show the command-line options.
2) Print that usage on all errors.
3) Don't fail with a weird error if there's a space between -j and
   the number.
4) Add support for the -h or --help options.
5) Ignore any command-line unsupported parameters that start with
   a dash.  This is necessary because otherwise the script will
   assume the option is actually a destination directory, and then
   the "test" command will get confused.  Drawback is that we don't
   support any more destination directories that start with a dash,
   but no one does that.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 copy-firmware.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index dd4b9b6f..cd5a6893 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -11,8 +11,13 @@ compext=
 destdir=
 num_jobs=1
 
+usage() {
+    echo "Usage: $0 [-v] [-jN] [--xz|--zstd] <destination directory>"
+}
+
 err() {
     printf "ERROR: %s\n" "$*"
+    usage
     exit 1
 }
 
@@ -39,6 +44,7 @@ while test $# -gt 0; do
 
         -j*)
             num_jobs=$(echo "$1" | sed 's/-j//')
+            num_jobs=${num_jobs:-1}
             if [ "$num_jobs" -gt 1 ] && ! has_gnu_parallel; then
                     err "the GNU parallel command is required to use -j"
             fi
@@ -66,6 +72,18 @@ while test $# -gt 0; do
             shift
             ;;
 
+        -h|--help)
+            usage
+            exit 1
+            ;;
+
+        -*)
+            # Ignore anything else that begins with - because that confuses
+            # the "test" command below
+            warn "ignoring option $1"
+            shift
+            ;;
+
         *)
             if test -n "$destdir"; then
                 err "unknown command-line options: $*"
-- 
2.43.0

[PATCH 2/2] copy-firmware: fail gracefully if moreutils parallel is installed

[Timur Tabi]

The copy-firmware.sh script can use the "parallel" command to
parallelize some operations, but it needs the GNU version of
parallel.  There is another, simpler version of parallel that is part
of the moreutils package, but that version confuses the
has_gnu_parallel() function.  So first test to make sure that
the --version parameter is even recognized before trying to use it.

If in the future, moreutils parallel adds support for --version, this
script should still work because that version should never report
"GNU parallel".

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 copy-firmware.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index cd5a6893..d8bfa2a3 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -27,6 +27,14 @@ warn() {
 
 has_gnu_parallel() {
     if command -v parallel > /dev/null; then
+        # The moreutils package comes with a simpler version of "parallel"
+        # that does not support the --version or -a options.  Check for
+        # that first.  In some distros, installing the "parallel" package
+        # will replace the moreutils version with the GNU version.
+        parallel --version >/dev/null 2>&1
+        if [ $? -ne 0 ]; then
+            return 1
+        fi
         if parallel --version | grep -Fqi 'gnu parallel'; then
            return 0
         fi
-- 
2.43.0

Re: [PATCH 1/2] copy-firmware: make script smarter about parameters

[Mario Limonciello]

On 3/17/2025 14:16, Timur Tabi wrote:
> Several improvements to copy-firmware.sh that make it more friendly
> when passed unknown or not exactly correct command-line parameters.
> 
> 1) Add a usage() function to show the command-line options.
> 2) Print that usage on all errors.
> 3) Don't fail with a weird error if there's a space between -j and
>     the number.
> 4) Add support for the -h or --help options.
> 5) Ignore any command-line unsupported parameters that start with
>     a dash.  This is necessary because otherwise the script will
>     assume the option is actually a destination directory, and then
>     the "test" command will get confused.  Drawback is that we don't
>     support any more destination directories that start with a dash,
>     but no one does that.
> 
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>   copy-firmware.sh | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)

/Conceptually/ I'm aligned on everything in this patch.

A few things.

1) One patch per change please.
2) It appears that the second patch in this this series fails CI.  You 
should install pre-commit or run `make check` to check before submitting 
in the future.

Here are the details of the failure.

https://gitlab.com/kernel-firmware/linux-firmware/-/merge_requests/486

BTW - In general it would be better to submit a MR so that you could 
just force push a fix.

> 
> diff --git a/copy-firmware.sh b/copy-firmware.sh
> index dd4b9b6f..cd5a6893 100755
> --- a/copy-firmware.sh
> +++ b/copy-firmware.sh
> @@ -11,8 +11,13 @@ compext=
>   destdir=
>   num_jobs=1
>   
> +usage() {
> +    echo "Usage: $0 [-v] [-jN] [--xz|--zstd] <destination directory>"
> +}
> +
>   err() {
>       printf "ERROR: %s\n" "$*"
> +    usage
>       exit 1
>   }
>   
> @@ -39,6 +44,7 @@ while test $# -gt 0; do
>   
>           -j*)
>               num_jobs=$(echo "$1" | sed 's/-j//')
> +            num_jobs=${num_jobs:-1}
>               if [ "$num_jobs" -gt 1 ] && ! has_gnu_parallel; then
>                       err "the GNU parallel command is required to use -j"
>               fi
> @@ -66,6 +72,18 @@ while test $# -gt 0; do
>               shift
>               ;;
>   
> +        -h|--help)
> +            usage
> +            exit 1
> +            ;;
> +
> +        -*)
> +            # Ignore anything else that begins with - because that confuses
> +            # the "test" command below
> +            warn "ignoring option $1"
> +            shift
> +            ;;
> +
>           *)
>               if test -n "$destdir"; then
>                   err "unknown command-line options: $*"

Re: [PATCH 1/2] copy-firmware: make script smarter about parameters

[Timur Tabi]

On Mon, 2025-03-17 at 15:27 -0500, Mario Limonciello wrote:
> On 3/17/2025 14:16, Timur Tabi wrote:
> > Several improvements to copy-firmware.sh that make it more friendly
> > when passed unknown or not exactly correct command-line parameters.
> > 
> > 1) Add a usage() function to show the command-line options.
> > 2) Print that usage on all errors.
> > 3) Don't fail with a weird error if there's a space between -j and
> >     the number.
> > 4) Add support for the -h or --help options.
> > 5) Ignore any command-line unsupported parameters that start with
> >     a dash.  This is necessary because otherwise the script will
> >     assume the option is actually a destination directory, and then
> >     the "test" command will get confused.  Drawback is that we don't
> >     support any more destination directories that start with a dash,
> >     but no one does that.
> > 
> > Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> > ---
> >   copy-firmware.sh | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> 
> /Conceptually/ I'm aligned on everything in this patch.
> 
> A few things.
> 
> 1) One patch per change please.

Do you want me to split patch [1/2] into five different patches?  That seems
excessive.  To me, this patch is already about "one change": improving 
the command line parser.

> 2) It appears that the second patch in this this series fails CI.  You 
> should install pre-commit or run `make check` to check before submitting 
> in the future.
> 
> Here are the details of the failure.
> 
> https://gitlab.com/kernel-firmware/linux-firmware/-/merge_requests/486
> 
> BTW - In general it would be better to submit a MR so that you could 
> just force push a fix.

Ok, I will fix the pipeline failure and submit an MR.

Re: [PATCH 1/2] copy-firmware: make script smarter about parameters

[Mario Limonciello]

On 3/17/2025 16:14, Timur Tabi wrote:
> On Mon, 2025-03-17 at 15:27 -0500, Mario Limonciello wrote:
>> On 3/17/2025 14:16, Timur Tabi wrote:
>>> Several improvements to copy-firmware.sh that make it more friendly
>>> when passed unknown or not exactly correct command-line parameters.
>>>
>>> 1) Add a usage() function to show the command-line options.
>>> 2) Print that usage on all errors.
>>> 3) Don't fail with a weird error if there's a space between -j and
>>>      the number.
>>> 4) Add support for the -h or --help options.
>>> 5) Ignore any command-line unsupported parameters that start with
>>>      a dash.  This is necessary because otherwise the script will
>>>      assume the option is actually a destination directory, and then
>>>      the "test" command will get confused.  Drawback is that we don't
>>>      support any more destination directories that start with a dash,
>>>      but no one does that.
>>>
>>> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
>>> ---
>>>    copy-firmware.sh | 18 ++++++++++++++++++
>>>    1 file changed, 18 insertions(+)
>>
>> /Conceptually/ I'm aligned on everything in this patch.
>>
>> A few things.
>>
>> 1) One patch per change please.
> 
> Do you want me to split patch [1/2] into five different patches?  That seems
> excessive.  To me, this patch is already about "one change": improving
> the command line parser.

Maybe 2 or 3 patches?  It seems like it's at least one patch for 
usage/--help and one patch for ignoring parameters with a dash.

> 
>> 2) It appears that the second patch in this this series fails CI.  You
>> should install pre-commit or run `make check` to check before submitting
>> in the future.
>>
>> Here are the details of the failure.
>>
>> https://gitlab.com/kernel-firmware/linux-firmware/-/merge_requests/486
>>
>> BTW - In general it would be better to submit a MR so that you could
>> just force push a fix.
> 
> Ok, I will fix the pipeline failure and submit an MR.
> 

OK.

Re: [PATCH 1/2] copy-firmware: make script smarter about parameters

[Timur Tabi]

On Mon, 2025-03-17 at 15:27 -0500, Mario Limonciello wrote:
> 2) It appears that the second patch in this this series fails CI.  You 
> should install pre-commit or run `make check` to check before submitting 
> in the future.

I forgot to mention that "make check" doesn't actually work for me.  I'm
running Ubuntu 24.04.01:

$ make check
[INFO] Installing environment for
https://github.com/igorshubovych/markdownlint-cli.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command:
('/usr/bin/python3', '-mnodeenv', '--prebuilt', '--clean-src',
'/home/ttabi/.cache/pre-commit/repo0ijef8bg/node_env-default')
return code: 1
stdout: (none)
stderr:
    /usr/lib/python3/dist-packages/nodeenv.py:24: DeprecationWarning:
'pipes' is deprecated and slated for removal in Python 3.13
      import pipes
    /usr/lib/python3/dist-packages/nodeenv.py:37: DeprecationWarning:
pkg_resources is deprecated as an API. See
https://setuptools.pypa.io/en/latest/pkg_resources.html
      from pkg_resources import parse_version
    Traceback (most recent call last):
      File "<frozen runpy>", line 198, in _run_module_as_main
      File "<frozen runpy>", line 88, in _run_code
      File "/usr/lib/python3/dist-packages/nodeenv.py", line 1042, in
<module>
        main()
      File "/usr/lib/python3/dist-packages/nodeenv.py", line 881, in main
        opt.node = get_last_stable_node_version()
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3/dist-packages/nodeenv.py", line 835, in
get_last_stable_node_version
        return links[-1][0]
               ~~~~~^^^^
    IndexError: list index out of range
Check the log at /home/ttabi/.cache/pre-commit/pre-commit.log
make: *** [Makefile:9: check] Error 3

Re: [PATCH 2/2] copy-firmware: fail gracefully if moreutils parallel is installed

[Maxim Cournoyer]

Hi Timur,

Timur Tabi <ttabi@nvidia.com> writes:

> The copy-firmware.sh script can use the "parallel" command to
> parallelize some operations, but it needs the GNU version of
> parallel.  There is another, simpler version of parallel that is part
> of the moreutils package, but that version confuses the
> has_gnu_parallel() function.  So first test to make sure that
> the --version parameter is even recognized before trying to use it.
>
> If in the future, moreutils parallel adds support for --version, this
> script should still work because that version should never report
> "GNU parallel".

Did you test that it works with moreutils' parallel?  When I authored
this change I went with GNU parallel because it could ingest a list of
commands from a file, and I couldn't see moreutils' parallel supporting
that.  Maybe I had missed something?

-- 
Thanks,
Maxim

Re: [PATCH 2/2] copy-firmware: fail gracefully if moreutils parallel is installed

[Maxim Cournoyer]

Hi again,

Timur Tabi <ttabi@nvidia.com> writes:

[...]

>  has_gnu_parallel() {
>      if command -v parallel > /dev/null; then
> +        # The moreutils package comes with a simpler version of "parallel"
> +        # that does not support the --version or -a options.  Check for
> +        # that first.  In some distros, installing the "parallel" package
> +        # will replace the moreutils version with the GNU version.
> +        parallel --version >/dev/null 2>&1
> +        if [ $? -ne 0 ]; then
> +            return 1
> +        fi

I just rechecked; I don't think this currently work, it'd require
complicating the script to accommodate this less featureful parallel
variant, which is why I had opted to use GNU Parallel in the first place
:-).

If you inspect how parallel is used, it is passed a list of commands
from a file, which GNU parallel accepts via its '-a' option:

--8<---------------cut here---------------start------------->8---
    parallel -j"$num_jobs" -a "$parallel_args_file"
--8<---------------cut here---------------end--------------->8---

Moreutils' parallel lacks the '-a' option, and doesn't appear to offer
something equivalent.  So instead of simply adding appending your
command lines to a file, you'd have to build a space-separated list of
quoted commands... which makes quoting annoying and complicates
debugging (in the former case you can just inspect/execute the commands
file).

-- 
Thanks,
Maxim

Re: [PATCH 2/2] copy-firmware: fail gracefully if moreutils parallel is installed

[Timur Tabi]

On Tue, 2025-03-18 at 15:20 +0900, Maxim Cournoyer wrote:
> > If in the future, moreutils parallel adds support for --version, this
> > script should still work because that version should never report
> > "GNU parallel".
> 
> Did you test that it works with moreutils' parallel? 

Several times.  I installed and uninstalled GNU parallel to make sure it
worked.


>  When I authored
> this change I went with GNU parallel because it could ingest a list of
> commands from a file, and I couldn't see moreutils' parallel supporting
> that.  Maybe I had missed something?

I think you missed the point of my patch.  moreutils' parallel does not
support -a, but it also does not support --version.  Your code does correctly
return 1, but it also displays a bogus "unsupported option" error message. 
All my change does is clean up that function so that it doesn't try to grep
the output of --version.

Re: [PATCH 2/2] copy-firmware: fail gracefully if moreutils parallel is installed

[Timur Tabi]

On Tue, 2025-03-18 at 15:42 +0900, Maxim Cournoyer wrote:
> I just rechecked; I don't think this currently work, it'd require
> complicating the script to accommodate this less featureful parallel
> variant, which is why I had opted to use GNU Parallel in the first place
> :-).

Like I said in my other reply, I think you misunderstood the point of my
patch.  moreutils' parallel is still not supported.  I'm not trying to make it
work.  All my patch does is make the function run more cleanly.  That's why I
said "fail gracefully".

Re: [PATCH 2/2] copy-firmware: fail gracefully if moreutils parallel is installed

[Maxim Cournoyer]

Hi Timur,

Timur Tabi <ttabi@nvidia.com> writes:

> On Tue, 2025-03-18 at 15:42 +0900, Maxim Cournoyer wrote:
>> I just rechecked; I don't think this currently work, it'd require
>> complicating the script to accommodate this less featureful parallel
>> variant, which is why I had opted to use GNU Parallel in the first place
>> :-).
>
> Like I said in my other reply, I think you misunderstood the point of my
> patch.  moreutils' parallel is still not supported.  I'm not trying to make it
> work.  All my patch does is make the function run more cleanly.  That's why I
> said "fail gracefully".

I see; I had indeed missed the point of your patch.  I understand the
problem this attempts to solve now, thanks for the clarifications.

+        # The moreutils package comes with a simpler version of "parallel"
+        # that does not support the --version or -a options.  Check for
+        # that first.  In some distros, installing the "parallel" package
+        # will replace the moreutils version with the GNU version.
+        parallel --version >/dev/null 2>&1
+        if [ $? -ne 0 ]; then
+            return 1
+        fi

Instead of checking $?, it's better style to check the command directly
like:

--8<---------------cut here---------------start------------->8---
@@ -22,8 +22,11 @@ warn() {
 
 has_gnu_parallel() {
     if command -v parallel > /dev/null; then
-        if parallel --version | grep -Fqi 'gnu parallel'; then
-           return 0
+        # Moreutils's parallel doesn't support '--version'.
+        if ! parallel --version >/dev/null 2>&1; then
+            return 1
+        elif parallel --version | grep -Fqi 'gnu parallel'; then
+            return 0
         fi
     fi
     return 1
--8<---------------cut here---------------end--------------->8---

-- 
Thanks,
Maxim

Re: [PATCH 2/2] copy-firmware: fail gracefully if moreutils parallel is installed

[Timur Tabi]

On Tue, 2025-03-18 at 23:58 +0900, Maxim Cournoyer wrote:
> Instead of checking $?, it's better style to check the command directly
> like:
> 
> --8<---------------cut here---------------start------------->8---
> @@ -22,8 +22,11 @@ warn() {
>  
>  has_gnu_parallel() {
>      if command -v parallel > /dev/null; then
> -        if parallel --version | grep -Fqi 'gnu parallel'; then
> -           return 0
> +        # Moreutils's parallel doesn't support '--version'.
> +        if ! parallel --version >/dev/null 2>&1; then
> +            return 1
> +        elif parallel --version | grep -Fqi 'gnu parallel'; then
> +            return 0

Sorry, I guess you missed the MR I submitted that already has this fix:

https://gitlab.com/kernel-firmware/linux-firmware/-/merge_requests/487