Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] base.pm: failures loading modules are ignored when sub-package exists #13043

Closed
p5pRT opened this issue Jun 21, 2013 · 17 comments
Closed
Labels

Comments

@p5pRT
Copy link

p5pRT commented Jun 21, 2013

Migrated from rt.perl.org#118561 (status was 'resolved')

Searchable as RT118561$

@p5pRT
Copy link
Author

p5pRT commented Jun 21, 2013

From @haarg

Created by @haarg

This is a bug report for perl from haarg@​haarg.org,
generated with the help of perlbug 1.39 running under perl 5.18.0.

-----------------------------------------------------------------

When base tries to load a module and finds that it doesn't exist, it
ignores the error if anything the package's stash exists. This is by
design, but causes issues when sub-packages exist. Attached is a patch
that will ignore sub-packages, while still considering anything else
in the stash as marking the package's existence.

The patch is based on the base CPAN 2.18 release from CPAN.

Perl Info

Flags:
    category=library
    severity=low
    module=base

Site configuration information for perl 5.18.0:

Configured by gknop at Sat May 18 11:37:06 EDT 2013.

Summary of my perl5 (revision 5 version 18 subversion 0) configuration:
  Commit id: a9acda3b5f74585852a57b51b724804ac586cb0b
  Platform:
    osname=darwin, osvers=12.3.0, archname=darwin-2level
    uname='darwin cuneus 12.3.0 darwin kernel version 12.3.0: sun jan
6 22:37:10 pst 2013; root:xnu-2050.22.13~1release_x86_64 x86_64 '
    config_args='-des -Dusedevel -Uversiononly
-Dprefix=/Users/gknop/perl5/perls/v5.18.0'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fno-common -DPERL_DARWIN -fno-strict-aliasing
-pipe -fstack-protector -I/usr/local/include -I/opt/local/include',
    optimize='-O3',
    cppflags='-fno-common -DPERL_DARWIN -fno-strict-aliasing -pipe
-fstack-protector -I/usr/local/include -I/opt/local/include'
    ccversion='', gccversion='4.2.1 Compatible Apple LLVM 4.2
(clang-425.0.28)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='env MACOSX_DEPLOYMENT_TARGET=10.3 cc', ldflags ='
-fstack-protector -L/usr/local/lib -L/opt/local/lib'
    libpth=/usr/local/lib /opt/local/lib /usr/lib
    libs=-lgdbm -ldbm -ldb -ldl -lm -lutil -lc
    perllibs=-ldl -lm -lutil -lc
    libc=, so=dylib, useshrplib=false, libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=bundle, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags=' -bundle -undefined dynamic_lookup
-L/usr/local/lib -L/opt/local/lib -fstack-protector'

Locally applied patches:



@INC for perl 5.18.0:
    /Users/gknop/.local-lib/lib/perl5/darwin-2level
    /Users/gknop/.local-lib/lib/perl5
    /Users/gknop/perl5/libs/5.018-darwin-2level/lib/perl5/darwin-2level
    /Users/gknop/perl5/libs/5.018-darwin-2level/lib/perl5
    /Users/gknop/perl5/perls/v5.18.0/lib/site_perl/5.18.0/darwin-2level
    /Users/gknop/perl5/perls/v5.18.0/lib/site_perl/5.18.0
    /Users/gknop/perl5/perls/v5.18.0/lib/5.18.0/darwin-2level
    /Users/gknop/perl5/perls/v5.18.0/lib/5.18.0
    .


Environment for perl 5.18.0:
    DYLD_LIBRARY_PATH (unset)
    HOME=/Users/gknop
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/Users/gknop/bin:/usr/local/share/npm/bin:/Users/gknop/bin/mvim-ln:/Users/gknop/perl5/bin:/Users/gknop/.local-lib/bin:/Users/gknop/perl5/libs/5.018-darwin-2level/bin:/Users/gknop/perl5/perls/active/bin:/Users/gknop/Library/Python/2.7/bin:/Users/gknop/.gem/ruby/1.8/bin:/usr/local/MacGPG2/bin:/opt/local/bin:/opt/X11/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/usr/local/MacGPG2/bin:/usr/local/share/git-core/contrib/remote-helpers
    PERL5LIB=/Users/gknop/.local-lib/lib/perl5:/Users/gknop/perl5/libs/5.018-darwin-2level/lib/perl5:
    PERL_BADLANG (unset)
    PERL_CPANM_OPT=--mirror http://cpan.metacpan.org/
    PERL_LOCAL_LIB_ROOT=:/Users/gknop/perl5/libs/5.018-darwin-2level
    PERL_MB_OPT=--install_base /Users/gknop/perl5/libs/5.018-darwin-2level
    PERL_MM_OPT=INSTALL_BASE=/Users/gknop/perl5/libs/5.018-darwin-2level
    SHELL=/usr/local/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Jun 21, 2013

From @haarg

sub-package-fix.patch
diff --git i/lib/base.pm w/lib/base.pm
index 19fc845..1d3db87 100644
--- i/lib/base.pm
+++ w/lib/base.pm
@@ -82,7 +82,7 @@ sub import {
                 # Only ignore "Can't locate" errors from our eval require.
                 # Other fatal errors (syntax etc) must be reported.
                 die if $@ && $@ !~ /^Can't locate .*? at \(eval /;
-                unless (%{"$base\::"}) {
+                unless (grep { !/::$/ } keys %{"$base\::"}) {
                     require Carp;
                     local $" = " ";
                     Carp::croak(<<ERROR);
diff --git i/t/base.t w/t/base.t
old mode 100644
new mode 100755
index 6fb24ea..705ed8f
--- i/t/base.t
+++ w/t/base.t
@@ -1,7 +1,7 @@
 #!/usr/bin/perl -w
 
 use strict;
-use Test::More tests => 11;
+use Test::More tests => 12;
 
 use_ok('base');
 
@@ -55,6 +55,11 @@ like( $@, qr/^Base class package "reallyReAlLyNotexists" is empty\./,
 eval q{use base 'reallyReAlLyNotexists'};
 like( $@, qr/^Base class package "reallyReAlLyNotexists" is empty\./,
                                           '  still empty on 2nd load');
+eval 'sub reallyReAlLyNotexists::Sub::welp { }';
+eval q{use base 'reallyReAlLyNotexists'};
+like( $@, qr/^Base class package "reallyReAlLyNotexists" is empty\./,
+    '  empty even with sub-package existing');
+
 {
     my $warning;
     local $SIG{__WARN__} = sub { $warning = shift };

@p5pRT
Copy link
Author

p5pRT commented Jun 22, 2013

From @jkeenan

On Thu Jun 20 17​:26​:51 2013, haarg wrote​:

This is a bug report for perl from haarg@​haarg.org,
generated with the help of perlbug 1.39 running under perl 5.18.0.

-----------------------------------------------------------------

When base tries to load a module and finds that it doesn't exist, it
ignores the error if anything the package's stash exists. This is by
design, but causes issues when sub-packages exist.

Can you elaborate a bit about what those issues would be?

Attached is a patch
that will ignore sub-packages, while still considering anything else
in the stash as marking the package's existence.

The patch is based on the base CPAN 2.18 release from CPAN.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Jun 22, 2013

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jun 22, 2013

From @ikegami

Foo/Bar.pm exists and Foo.pm doesn't​:

perl -E"use Foo​::Bar; use base 'Foo'; say $INC{'Foo.pm'} || 0"
0

perl -E" use base 'Foo'; say $INC{'Foo.pm'} || 0"
Base class package "Foo" is empty.
  (Perhaps you need to 'use' the module which defines that package first,
  or make that module available in @​INC (@​INC contains​: ...).
at -e line 1.
BEGIN failed--compilation aborted at -e line 1.

@p5pRT
Copy link
Author

p5pRT commented Jun 25, 2013

From @tonycoz

On Thu Jun 20 17​:26​:51 2013, haarg wrote​:

When base tries to load a module and finds that it doesn't exist, it
ignores the error if anything the package's stash exists. This is by
design, but causes issues when sub-packages exist. Attached is a patch
that will ignore sub-packages, while still considering anything else
in the stash as marking the package's existence.

The patch is based on the base CPAN 2.18 release from CPAN.

Thanks, applied as c4f21d8.

Welcome to the AUTHORS file.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 25, 2013

@tonycoz - Status changed from 'open' to 'resolved'

@p5pRT
Copy link
Author

p5pRT commented Jun 28, 2013

From @tonycoz

On Mon Jun 24 19​:08​:05 2013, tonyc wrote​:

On Thu Jun 20 17​:26​:51 2013, haarg wrote​:

When base tries to load a module and finds that it doesn't exist, it
ignores the error if anything the package's stash exists. This is by
design, but causes issues when sub-packages exist. Attached is a patch
that will ignore sub-packages, while still considering anything else
in the stash as marking the package's existence.

The patch is based on the base CPAN 2.18 release from CPAN.

Thanks, applied as c4f21d8.

I may need to revert this, based on 118655.

The test code in Moose's instance_metaclass_incompat.t has code like​:

$@​ = undef;
eval {
  package Foo;
  metaclass->import('instance_metaclass' => 'Foo​::Meta​::Instance');
};
ok(!$@​, '... Foo.meta => Foo​::Meta is compatible') || diag $@​;

$@​ = undef;
eval {
  package Bar;
  metaclass->import('instance_metaclass' => 'Bar​::Meta​::Instance');
};
ok(!$@​, '... Bar.meta => Bar​::Meta is compatible') || diag $@​;

$@​ = undef;
eval {
  package Foo​::Foo;
  use base 'Foo';
  metaclass->import('instance_metaclass' => 'Bar​::Meta​::Instance');
};

That use base line is causing the test to fail.

I think what Moose is doing here is reasonable, so I think the change
needs to be reverted, but I welcome discussion.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 28, 2013

@tonycoz - Status changed from 'resolved' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jun 28, 2013

From @haarg

I don't really think what Moose is doing here is reasonable.

The only reason it worked without this change was the declaration of the
Foo​::Foo and Foo​::Meta​::Instance packages. That causes base to think
that the Foo package exists. The actual declaration of Foo, or it
calling metaclass->import, don't have any impact.

I think the test was only working accidentally, and the thing it was
testing doesn't really have anything to do with issue directly. It will
also be very simple to fix.

I also looked at the other two modules from #118655. One has a bug that
was reported in January that is now causing a test failure. The other
is due to a circular dependency between its modules.

@p5pRT
Copy link
Author

p5pRT commented Jul 3, 2013

From @tonycoz

On Thu Jun 27 23​:04​:34 2013, haarg wrote​:

I don't really think what Moose is doing here is reasonable.

The only reason it worked without this change was the declaration of the
Foo​::Foo and Foo​::Meta​::Instance packages. That causes base to think
that the Foo package exists. The actual declaration of Foo, or it
calling metaclass->import, don't have any impact.

I think the test was only working accidentally, and the thing it was
testing doesn't really have anything to do with issue directly. It will
also be very simple to fix.

I also looked at the other two modules from #118655. One has a bug that
was reported in January that is now causing a test failure. The other
is due to a circular dependency between its modules.

I agree about the other two modules, but I still think Moose is being
reasonable.

The problem with both the current check and the check your change
introduced is that they're both heuristics - they don't test with
authority that the module failed to load, just that it may have.

It's possible for a module to fail to load (due to a syntax error for
example) but still define names in its package.

This problem is one of the reasons parent.pm was created - with
parent.pm you specify whether you want the module required, and it uses
the normal require failure mechanism to report module load failure.

I've reverted this change and added a note to base.pm to warn anyone
else about changes to that code.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 3, 2013

@tonycoz - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Jul 3, 2013
@p5pRT
Copy link
Author

p5pRT commented Jul 3, 2013

From @haarg

Can you explain why you think what Moose is going is reasonable?

I know this is a heuristic. I prefer to always avoid base.pm in my own
code because of that. But some heuristics are better than others. The
only reason the code in Moose's test works (which I've already submitted
a pull request for) is due to an accident of the implementation.

"Mentioning a sub-package in the same compile-time context as a later
use base" doesn't sound like sane behavior to me.

@p5pRT
Copy link
Author

p5pRT commented Jul 3, 2013

From @karenetheridge

On Tue, Jul 02, 2013 at 06​:12​:59PM -0700, Tony Cook via RT wrote​:

On Thu Jun 27 23​:04​:34 2013, haarg wrote​:

I don't really think what Moose is doing here is reasonable.

The only reason it worked without this change was the declaration of the
Foo​::Foo and Foo​::Meta​::Instance packages. That causes base to think
that the Foo package exists. The actual declaration of Foo, or it
calling metaclass->import, don't have any impact.

I think the test was only working accidentally, and the thing it was
testing doesn't really have anything to do with issue directly. It will
also be very simple to fix.

I also looked at the other two modules from #118655. One has a bug that
was reported in January that is now causing a test failure. The other
is due to a circular dependency between its modules.

I agree about the other two modules, but I still think Moose is being
reasonable.

The code in question is​:

https://github.com/moose/moose/blob/master/t/cmop/instance_metaclass_incompat.t

where it's clear that the intent is that the Foo package *does* exist. Is
there actually a way to indicate this in an unambiguous way? If not, then
it's not a bad thing that Graham's changes happen to break these tests; we
simply need to change the code to meeting the new heuristic's requirements.

The problem with both the current check and the check your change
introduced is that they're both heuristics - they don't test with
authority that the module failed to load, just that it may have.

It's possible for a module to fail to load (due to a syntax error for
example) but still define names in its package.

This problem is one of the reasons parent.pm was created - with
parent.pm you specify whether you want the module required, and it uses
the normal require failure mechanism to report module load failure.

If it helps the discussion, Moose can quite easily switch from using base
to parent. There's no reason at all we should be using base, except that
it's in core in 5.8.x.

I've reverted this change and added a note to base.pm to warn anyone
else about changes to that code.

Why is base still in core, anyway? Isn't it past time that we throw it in
the bikeshed at the foot of the garden where the trolls live?

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2013

From @rgarcia

On 4 July 2013 00​:23, Karen Etheridge <perl@​froods.org> wrote​:

Why is base still in core, anyway? Isn't it past time that we throw it in
the bikeshed at the foot of the garden where the trolls live?

There are a few core modules that use base. It could be interesting to
switch them to parent.

@p5pRT
Copy link
Author

p5pRT commented Sep 8, 2013

From @dolmen

Le Mer. Jul. 03 15​:24​:35 2013, perl@​froods.org a �crit�​:

If it helps the discussion, Moose can quite easily switch from using
base
to parent. There's no reason at all we should be using base, except
that
it's in core in 5.8.x.

A good reason to switch Moose to parent now is that the patch is already
available.
I have submitted it to the Moose mailing list almost one year ago.
http​://www.nntp.perl.org/group/perl.moose/2012/09/msg2498.html
Unfortunately nobody applied it at the time.

That work is now avalable as a pull request.
moose/Moose#35

Olivier Mengué (DOLMEN).

@p5pRT
Copy link
Author

p5pRT commented Sep 9, 2013

From @haarg

Switching Moose or any core perl modules to use parent instead of base
is probably reasonable, but this isn't really the ticket for it.

Is there any chance of getting this ticket re-opened? I'd still like
to get the initial patch re-applied. While I agree that base.pm's
heuristic will never be perfect, that doesn't mean it can't be
improved. Any case that would be effected by this change is in my
mind very clearly already relying on broken behavior. Of the three
CPAN modules effected, this change caught real bugs in two.

The third, Moose, was accidentally relying on some very questionable
compile vs runtime ordering in its tests. These have since been
fixed, because Moose's authors agreed that this patch was an
appropriate change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant