The needs() function should propagate run-time task arguments (%params and @args) from the calling task down to the "needed" tasks

Edits:

As it turns out: this bug is really easy to fix, as it appears to stem from a typo, introduced initially by PR #1157 (the fix for #1066 ) with 48c737b (ultimately merged into master with 7cf0ca4). Some other sharp edges introduced by PR #1157 seem to have already been handled by #1188, but not this one.

I am preparing a PR with accompanying tests.

Describe the bug

The needs() function has a bug which prevents it from implicitely propagating the calling task's params/args down to the "needed" tasks when it runs them.

How to reproduce it

Steps to reproduce the behavior:

1. In a brief Rexfile, define two simple tasks (task_a & task_b) that dump their run-time arguments (@_) on STDERR
2. Make sure that task_b calls needs("task_a")
3. From the shell, launch rex task_b (with appropriate host & auth info) and observe the results
   The dump from task_a is empty, whereas task_b normally displays its params.
4. Also try rex task_b --greetings=Hello, and observe that the result is similar to the previous trial.

Shortest code example that demonstrates the bug:

Rexfile

# Rexfile
### ex: set ft=perl :

use Rex -feature => [qw(1.13 exec_autodie)];
use DDP; # alias for `Data::Printer`, used just for pretty display of dumps.

desc "Display host name";
task hostname => sub {
  say STDERR "hostname: \@_ : "; DDP::p(@_);
  say STDERR ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>";   
  say ''. run('hostname');
};

desc "Display host info";
task hostinfo => sub {
  say STDERR "hostinfo: \@_ : "; DDP::p(@_);
  say STDERR ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>";   
  needs "hostname";
};

1;

Run the example from the shell

Execute rex within the directory where the above Rexfile resides (with appropriate host and authentication switches).

$ rex hostinfo --greetings=Hello
[2021-09-26 18:26:45] INFO - Running task hostinfo on medusa
hostinfo: @_ : 
[
    [0] {
            greetings => "Hello",
            hostinfo  => 1
        },
    [1] []
]
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
hostname: @_ : 
[]
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
medusa

Expected behavior

The needs() function should implicitly propagate the calling task's params/args down to the "needed" tasks when it runs them, as documented and seemingly intended by the current code base (see notes below).

Circumstances

• Rex version: (R)?ex 1.13.4 (also on latest branch master)
• Perl version: v5.34.0
• OS running rex: ~~~ (irrelevant)
• OS managed by rex: ~~~ (irrelevant)
• How rex was installed: git clone repo

Debug log

Notes

It turns out: this is really easy to fix, as it appears to stem from a typo.

The culprit

Here's the offending portion of code in Rex::Commands.

# lib/Rex/Commands.pm 
...
sub needs {
    ...
    if ( @args && grep ( /^\Q$task_name\E$/, @args ) ) {
      Rex::Logger::debug( "Calling " . $task_o->name );
      $task_o->run( "<func>", params => \@task_args, args => \%task_opts );
    }
    elsif ( !@args ) {
      Rex::Logger::debug( "Calling " . $task_o->name );
      $task_o->run( "<func>", params => \@task_args, args => \%task_opts );
    }
    ...
}

Notice how params and args are mixed up (interchanged) when being passed to run!! This certainly looks like a typo.

The current test suite does not catch the issue. The tests in needs.t do not cover propagation of parameters/args.

Quick fix

For a fix with minimal changes, just patch the two occurrences of the $task_o->run call, within the body of the needs() subroutine (in Rex::Commands), as below:

- $task_o->run( "<func>", params => \@task_args, args => \%task_opts );
+ $task_o->run( "<func>", params => \%task_opts, args => \@task_args );

Alternative fix (slightly better, imho)

There does not appear to be any particular reason for having two identical invocations of the ->run method in the above code snippet.

So, the offending code can be replaced by the following -logically equivalent- snippet. I will put that in a separate commit; in case there are other (historical?) reasons to keep the two branches of the if statement around.

In any case, all tests pass either way.

# lib/Rex/Commands.pm

...

sub needs {
    ...
    # edit: further simplified the logic.
    if ( !@args || grep ( /^\Q$task_name\E$/, @args ) ) {
      Rex::Logger::debug( "Calling " . $task_o->name );
      $task_o->run( "<func>", params => \%task_opts, args => \@task_args );
    }
    ...
}

Edit: Digging in repo history, it seems that the if ... elsif ... form existed since the very initial introduction of needs() by commit 95d3e91, but even at that time (and probably ever since), those two arms of the if statement always did exactly the same thing... So I can't think of any valid reason to keep them around.