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

[tabulon]

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.

[tabulon]

Hi @ferki,

PR (#1509) which fixes this issue (#1508) is ready, but maintainer approval is required to launch CI workflow (for first time contributors to the project, as is the case here).

Otherwise, all tests pass locally with dzil test --all.

Should I just mark th PR as "Ready for Review" so as to get out of the chicken-and-egg situation ?

[ferki]

Thanks for the report, @tabulon! Kudos for the amazing amount of details 👍

[...] 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.

Yeah, unfortunately that commit had somehow caused quite some trouble. Hopefully the affected behavior is nicely covered by tests after this one.

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

Hmm, I don't think parameter and argument passing is explicitly documented for needs (yet). The docs only mention "same server configuration as the calling task" which refers to "execute the needed task on the same connection that is currently active". Though I can confirm that the previous behavior has changed (seemingly inadvertently) on the aforementioned commit, so it's a bug to fix.

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.

Yes, I agree that it seems to be a typo, and this aspect of the behavior is untested. Well spotted!

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.

That's an amazing depth of analysis, thanks for going the extra mile by looking at the history! ❤️

I also don't think there's any reason to keep duplicate code. As an aside, I wonder if there's a good way to detect similar duplications? 🤔 Perhaps that's something for the scope of Perl::Critic::TooMuchCode.

I'll also re-read the history again, but I think we should be fine as long as we:

• define the behavior better (in docs)
• define the behavior with tests
• restore the original behavior as a bugfix

[ferki]

[...] maintainer approval is required to launch CI workflow (for first time contributors to the project, as is the case here).

I've modified the repo settings to be more relaxed about triggering workflows.