Item630: Fix the code in extender.pl to use UNIVERSAL::require instead of eval use

The check_dep function of extender.pl does some eval use $module which isn't very elegant.

Best to use CPAN:UNIVERSAL::require, as suggested by Maddingue on #perlfr, and as he was so kind as to provide the patch and the unit tests, I'll commit it right away.

-- OlivierRaginel - 01 Jan 2009

The problem with this is that it adds a dependency on http://search.cpan.org/dist/UNIVERSAL-require/lib/UNIVERSAL/require.pm

It's a pretty old CPAN module, but it's not part of standard distributions...

Thus, I'd rather wait for other feedbacks before doing any code change.

-- OlivierRaginel - 01 Jan 2009

is it just a matter of elegance or are there real performance issues for the desired change? w.r.t. CPAN:UNIVERSAL::require, the code is rather small ( http://search.cpan.org/src/MSCHWERN/UNIVERSAL-require-0.11/lib/UNIVERSAL/require.pm ), so relevant bits could be copied (though i am loathe to do such a thing) or we can add it to our lib/CPAN/lib/ directory as it is pure perl. if it makes things better, i say to copy it to our CPAN directory. i also notice that it is taint clean.

-- WillNorris - 02 Jan 2009

I'd be very nervous of importing any module that redefines require - sounds like a recipe for trouble. I don't understand the problem here; what is improved by using the CPAN module? Surely this is an Enhancement, not Normal priority? I changed the priority accordingly.

-- CrawfordCurrie - 02 Jan 2009

(added CC's signature to keep track of who said what)

Ok, let me explain a bit more...

I thought the purpose of using CPAN modules was clear, but it seems it's not. Basically, why re-invent the wheel when you can use something that's been highly tested, written by a very well known and renowned perl guru (Michael Schwern, yes, TEH Michael Schwern), and that he will maintain, bugfix, etc...

The all benefit of the module is stated in its documentation: http://search.cpan.org/dist/UNIVERSAL-require/lib/UNIVERSAL/require.pm#DESCRIPTION In a word: readability. Check [Maddingue's code (below, as the post on rafb.net was gone) and compare it with our current one. If you don't see an improvement here, then reject this change and forget about it.

Also (sorry, was lost in my re-factoring), Crawford, is does NOT redefine the require method, it simply creates a require method in the UNIVERSAL scope, so it's universal, meaning you may invoke it on any object and non-object.

The drawback, of course, is that it adds one required CPAN module. But as WillNorris points out, we could add it to our local CPAN directory.

I've already created a local git branch with Maddingue's fix and unit test integration, so it's ready to be integrated. If you want to have a look at the code, I've posted his post below.

And honestly, if you wonder why I've been looking into this, it all started because of configure taint errors. When installing a module, it was failing (See Item597). I debugged this all the way to the "use CPAN", which taints @INC because it replaces '.' by Cwd. And trust me, the error message want's obvious at all. I was hinted that it might be @INC that was tainted by rgs. Using UNIVERSAL::require should really ease this kind of maintenance.

Also, please note that this module is available from most of the current linux distributions, but it is not included in ActiveState perl. And not in Strawberry perl either, but as it's a CPAN module, it should be a piece of cake to install with Strawberry.

We could also do worst case scenario, that if UNIVERSAL::require isn't installed, it degrades to the way we currently do it, otherwise, we use the require way.

-- OlivierRaginel - 02 Jan 2009

maddingue@Gwaihir:~ $ cat check_dep 
#!/usr/bin/perl
use strict;
use UNIVERSAL::require;


my @deps = (
    { type => "external", name => "libpcap",         version => "1.0.0" },
    { type => "perl",     name => "Carp" },
    { type => "cpan",     name => "Net::Pcap",       version => "0.15" },
    { type => "cpan",     name => "Net::Pcap::Easy", version => "1.31" },
    { type => "cpan",     name => "Net::Frame",      version => "1.12" },
);

for my $dep (@deps) {
    print join " ", check_dep($dep);
}


sub check_dep {
    my ($dep) = @_;
    my ($ok, $msg) = (1, "");

    # reject non-Perl dependencies
    if ($dep->{type} !~ /^(?:perl|cpan)$/i) {
        $ok  = 0;
        $msg = "Module is type $dep->{type}, and cannot be automatically checked.\n"
             . "Please check it manually and install if necessary.\n";
        return ($ok, $msg)
    }

    # try to load the module
    my $module = $dep->{name};
    if (not $module->require) {
        $ok = 0;
        ($msg = $@) =~ s/ in .*$/\n/s;
        return ($ok, $msg)
    }

    # check if the version satisfies the prerequisite
    if (defined $dep->{version}) {
        if (not eval { $module->VERSION($dep->{version}) }) {
            $ok = 0;
            ($msg = $@) =~ s/ at .*$//;
            return ($ok, $msg)
        }
    }

    $msg = "$module v" . $module->VERSION . " loaded\n";

    return ($ok, $msg)
}

maddingue@Gwaihir:~ $ perl check_dep 
0 Module is type external, and cannot be automatically checked.
Please check it manually and install if necessary.
1 Carp v1.03 loaded
1 Net::Pcap v0.16 loaded
0 Can't locate Net/Pcap/Easy.pm
0 Net::Frame version 1.12 required--this is only version 1.05

---

OK, I understand. What does it do to performance? If it is less than measurable, and the installation issues can be resolved, then it looks like a good idea. Note that it would also require really thorough developer doc, as it's total black magic to any new reader of the above code.

-- CrawfordCurrie - 06 Jan 2009

Thanks to Maddingue again:

#!/usr/bin/perl
use strict;
use Benchmark;
 
my $module = "POSIX";
 
timethese(1000 => {
    'require Module'        => sub { system perl => -e => "require $module" },
    'eval-require $module'  => sub { system perl => -e => "my \$module = '$module'; eval \"require \$module\"" },
    'Module->require'       => sub { system perl => -MUNIVERSAL::require => -e => "$module->require" },
    '$module->require'      => sub { system perl => -MUNIVERSAL::require => -e => "my \$module = '$module'; $\module->require" },
});
 
__END__
 
Benchmark: timing 1000 iterations of 1. require Module, 2. eval-require $module, 3. Module->require, 4. $module->require...
require Module: 11 wallclock secs ( 0.07 usr  0.38 sys +  7.18 cusr  2.71 csys = 10.34 CPU) @ 2222.22/s (n=1000)
eval-require $module: 11 wallclock secs ( 0.06 usr  0.43 sys +  7.31 cusr  2.63 csys = 10.43 CPU) @ 2040.82/s (n=1000)
Module->require: 18 wallclock secs ( 0.06 usr  0.36 sys + 12.80 cusr  3.32 csys = 16.54 CPU) @ 2380.95/s (n=1000)
$module->require: 12 wallclock secs ( 0.07 usr  0.35 sys +  8.27 cusr  2.89 csys = 11.58 CPU) @ 2380.95/s (n=1000)

It might be dependent on the module, but I doubt the overhead will be more significant.

-- OlivierRaginel - 23 Feb 2009

We are not going to add another CPAN dependency to the core for something that is not necessary.

It is so bloody difficult for the users that are on a shared host to get CPAN libs installed.

And I cannot see that this adds so much value that it absorbs the pain it creates. We have enough depands for additional CPAN modules as it is.

I can accept when an extension that does something advanced requires CPAN libs that does it all for you. But adding a CPAN lib just to do require just does not make sense. I spent hours trying to figure out how to install CPAN libs on a Windows installation. It is difficult because CPAN does not work without a lot of tweaking on Windows. We are going to loose a lot of people with this.

-- KennethLavrsen - 23 Feb 2009

Anyway, we've discussed this already, and we decided to include the module (which is one file, UNIVERSAL/require.pm) in our local core/lib/CPAN/lib, thus you may install the system one, or use the one provided with the disctribution.

I agree that we shouldn't bloat the dependencies for nothing, but here...

Also, may I note that the 1.0.1 root cause of the bug which triggered 1.0.2 was because of exactly this kind of code (eval "require $module")? When I said it helps the code, it also helps the reporting.

And last but not least, Windows installation come with a cpan shell nowadays.

-- OlivierRaginel - 23 Feb 2009

this needs to be backed out.

there is no actual benefit, and it adds at least the following issues:

1. it hides a built into the language feature behind a class method
2. it makes the code harder to read, analyse and detect
3. it adds heavy magic where its not needed
4. it adds more code execution to where its not needed - your analysis seems to suggest that it has a performance impact - and as we know from tmwiki history, the millions of 'it won't have an impact' are exactly why the codebase is slow.

this task appears to be for configure, but i get the feeling its used elsewhere too.

-- SvenDowideit - 24 Apr 2009

Whatever.

1. really? See 2.
2. so if( not $module->require ) is harder to read than if( eval "require $module" ). Maybe. What about: if( not $module->use ) vs if( eval "use $module; 1;" )
3. there is no heavy magic. The only thing it's using is that everything inherits from UNIVERSAL. But if you call that heavy magic...
4. My analysis seems to suggest it doesn't have any impact. The benchmark shows I'm more benchmarking the speed of my disks and cpu by forking perl binaries than anything else.

Anyway, rollback commit in the pipe. For the release branch, should arrive within minutes.

PS: Crawford, you started using it in Store.pm, I changed that too.

-- OlivierRaginel - 24 Apr 2009

Please do not reopen old bugs when there has been a released between. In this case 1.0.3-1.0.4

Otherwise it becomes impossible to follow what changed in which released versions.

If there is a new bug, open a new report.

And when we talk Release branch - if it works do not fix it.

I am re-closing this and please check in new code on trunk on new report so we can get the traceability and change histories correct

Both for developers looking for bugs and for users reading the release note this is of high value.

If same bug is used for multiple releases noone can see from the report what was fixed or rebroken when.

it takes 10 seconds to open a new bug

Note that my action here is in no way expressing disagreement with Sven's position. I am just enforcing a little configuration and change management.

-- KennethLavrsen - 24 Apr 2009

Note that my action to re-open this bug and commit it here is in no way expressing disagreement with anybody. In fact, I just wanted to commit it to the release branch with the same bug ID I put on trunk, but as Kenneth pointed out, it's not the right time to merge that into the release branch, so I rolled it back. Might put it back again if 1.0.6 is planned and not 1.1, but we'll have time to test it.

I agree I should never have pushed for that move. It seemed like the right thing to do back then, but it's too much of a hassle for a small gain. Anyway, it helped fix some other bugs that came to light because of that

-- OlivierRaginel - 24 Apr 2009