BBC Module::Install broken by 0301e899536a22752f40481d8a1d141b7a7dda82

[p5pRT]

Migrated from rt.perl.org#132577 (status was 'open')

Searchable as RT132577$

[p5pRT]

From zefram@fysh.org

Created by zefram@fysh.org

Module​::Install fails one of its tests since core commit
0301e89 "properly define perl_parse()
return value". I'm surprised that anything noticed such small code
changes.

Perl Info

Flags:
    category=core
    severity=high

Site configuration information for perl 5.27.7:

Configured by zefram at Tue Dec 12 12:07:39 GMT 2017.

Summary of my perl5 (revision 5 version 27 subversion 7) configuration:
  Commit id: 0301e899536a22752f40481d8a1d141b7a7dda82
  Platform:
    osname=linux
    osvers=3.16.0-4-amd64
    archname=x86_64-linux-thread-multi
    uname='linux barba.rous.org 3.16.0-4-amd64 #1 smp debian 3.16.43-2+deb8u2 (2017-06-26) x86_64 gnulinux '
    config_args='-des -Dprefix=/home/zefram/usr/perl/perl_install/perl-git-blead-i64-f52 -Duselargefiles -Dusethreads -Uafs -Ud_csh -Uusesfio -Uusenm -Duseshrplib -Dusedevel -Uversiononly -Ui_db -DDEBUGGING'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
    bincompat5005=undef
  Compiler:
    cc='cc'
    ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2'
    optimize='-O2 -g'
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='4.9.2'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='cc'
    ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.9/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
    libs=-lpthread -lnsl -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.19.so
    so=so
    useshrplib=true
    libperl=libperl.so
    gnulibc_version='2.19'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E -Wl,-rpath,/home/zefram/usr/perl/perl_install/perl-git-blead-i64-f52/lib/5.27.7/x86_64-linux-thread-multi/CORE'
    cccdlflags='-fPIC'
    lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector-strong'



@INC for perl 5.27.7:
    /home/zefram/usr/perl/perl_install/perl-git-blead-i64-f52/lib/site_perl/5.27.7/x86_64-linux-thread-multi
    /home/zefram/usr/perl/perl_install/perl-git-blead-i64-f52/lib/site_perl/5.27.7
    /home/zefram/usr/perl/perl_install/perl-git-blead-i64-f52/lib/5.27.7/x86_64-linux-thread-multi
    /home/zefram/usr/perl/perl_install/perl-git-blead-i64-f52/lib/5.27.7


Environment for perl 5.27.7:
    HOME=/home/zefram
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/zefram/usr/perl/perl_install/perl-git-blead-i64-f52/bin:/home/zefram/pub/x86_64-unknown-linux-gnu/bin:/home/zefram/pub/common/bin:/usr/bin:/bin:/usr/local/bin:/usr/games
    PERL5LIB=
    PERL5OPT=
    PERL5_CPANPLUS_IS_RUNNING=13467
    PERL5_CPAN_IS_RUNNING=13467
    PERLDOC=-oman
    PERL_BADLANG (unset)
    SHELL=/usr/bin/zsh

[p5pRT]

From zefram@fysh.org

I wrote​:

Module​::Install fails one of its tests since core commit
0301e89 "properly define perl_parse()
return value".

Turns out that Module​::Install​::DSL has some rather convoluted code in
which an "import" method that runs from a Makefile.PL establishes an INIT
block that will do the real work of Makefile.PL and then performs exit(0).
The behaviour difference that's relevant boils down to​:

$ perl5.27.6 -lwe 'print "main program"; INIT { print "init block"; } BEGIN { print "begin block"; exit 0; }'
begin block
init block
$ perl-blead -lwe 'print "main program"; INIT { print "init block"; } BEGIN { print "begin block"; exit 0; }'
begin block
$

That is, after a BEGIN block performs an exit(0), perl used to run
INIT blocks and then exit, but now it exits immediately without running
INIT blocks. It always used to exit without running INIT blocks upon
an exit with non-zero exit code, or upon an exception.

This situation is not precisely the exit(0) from a CHECK block that was
called out in [perl #2754], and indeed on that ticket it was thought
that exit(0) from BEGIN blocks was operating correctly. Nevertheless,
for it to execute BEGIN blocks is clearly another form of the same bug
with which that ticket is concerned, and the commit has fixed it.

Module​::Install​::DSL does have a genuine, if not entirely respectable,
reason for its exit(0). The essence of this module is that a Makefile.PL
invokes the module and then stops containing Perl code, instead switching
to a DSL to specify attributes of its distro. The module opens up the
Makefile.PL as a file to read it in and parse this DSL, and uses exit(0)
to prevent Perl's parsing of the top-level Makefile.PL from proceeding
into the DSL code that would constitute syntax errors.

However, as far as I can see there's no need at all for the module to
delay execution of the code it generates by putting it in an INIT block.
If that code is instead executed immediately, by deleting the word "INIT"
to turn it into a bare block, everything works. With that alteration
to the module, the Module-Install distro passes its test suite.

-zefram

[p5pRT]

From zefram@fysh.org

I have opened [rt.cpan.org #123867 (https://rt.cpan.org/Public/Bug/Display.html?id=123867) ] on the Module-Install side.

-zefram

[p5pRT]

From @Leont

On Tue, Dec 12, 2017 at 8​:13 PM, Zefram <zefram@​fysh.org> wrote​:

However, as far as I can see there's no need at all for the module to
delay execution of the code it generates by putting it in an INIT block.
If that code is instead executed immediately, by deleting the word "INIT"
to turn it into a bare block, everything works. With that alteration
to the module, the Module-Install distro passes its test suite.

Given Module​::Install's rather unfortunate bundling nature, that would
require rereleasing all 119 distributions using it to be rereleased with
such a new Module​::Install.

Leon

[p5pRT]

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

[p5pRT]

From @Leont

On Tue, Dec 12, 2017 at 11​:03 PM, Leon Timmermans <fawaka@​gmail.com> wrote​:

On Tue, Dec 12, 2017 at 8​:13 PM, Zefram <zefram@​fysh.org> wrote​:

However, as far as I can see there's no need at all for the module to
delay execution of the code it generates by putting it in an INIT block.
If that code is instead executed immediately, by deleting the word "INIT"
to turn it into a bare block, everything works. With that alteration
to the module, the Module-Install distro passes its test suite.

Given Module​::Install's rather unfortunate bundling nature, that would
require rereleasing all 119 distributions using it to be rereleased with
such a new Module​::Install.

Well, all 119 modules using Module​::Install​::DSL, Module​::Install in
general has quite a few more users.

Leon

[p5pRT]

From @xsawyerx

On 12/13/2017 12​:04 AM, Leon Timmermans wrote​:

On Tue, Dec 12, 2017 at 11​:03 PM, Leon Timmermans <fawaka@​gmail.com
<mailto​:fawaka@​gmail.com>> wrote​:

On Tue\, Dec 12\, 2017 at 8&#8203;:13 PM\, Zefram \<zefram@&#8203;fysh\.org
\<mailto&#8203;:zefram@&#8203;fysh\.org>> wrote&#8203;:

    However\, as far as I can see there's no need at all for the
    module to
    delay execution of the code it generates by putting it in an
    INIT block\.
    If that code is instead executed immediately\, by deleting the
    word "INIT"
    to turn it into a bare block\, everything works\.  With that
    alteration
    to the module\, the Module\-Install distro passes its test suite\.


Given Module&#8203;::Install's rather unfortunate bundling nature\, that
would require rereleasing all 119 distributions using it to be
rereleased with such a new Module&#8203;::Install\.

Well, all 119 modules using Module​::Install​::DSL, Module​::Install in
general has quite a few more users.

That is indeed a pain.

What is the cost of reverting this commit instead?

[p5pRT]

From @cpansprout

On Sun, 17 Dec 2017 03​:06​:01 -0800, xsawyerx@​gmail.com wrote​:

On 12/13/2017 12​:04 AM, Leon Timmermans wrote​:

On Tue, Dec 12, 2017 at 11​:03 PM, Leon Timmermans <fawaka@​gmail.com
<mailto​:fawaka@​gmail.com>> wrote​:

On Tue\, Dec 12\, 2017 at 8&#8203;:13 PM\, Zefram \<zefram@&#8203;fysh\.org
\<mailto&#8203;:zefram@&#8203;fysh\.org>> wrote&#8203;:

    However\, as far as I can see there's no need at all for the
    module to
    delay execution of the code it generates by putting it in an
    INIT block\.
    If that code is instead executed immediately\, by deleting the
    word "INIT"
    to turn it into a bare block\, everything works\.  With that
    alteration
    to the module\, the Module\-Install distro passes its test suite\.


Given Module&#8203;::Install's rather unfortunate bundling nature\, that
would require rereleasing all 119 distributions using it to be
rereleased with such a new Module&#8203;::Install\.

Well, all 119 modules using Module​::Install​::DSL, Module​::Install in
general has quite a few more users.

That is indeed a pain.

I seem to remember an old blog or list post by Michael Schwern predicting this very problem with Module​::Install.

What is the cost of reverting this commit instead?

Buggy, unpredictable behaviour, unless you can memorise the list of complex rules for when exit does and does not prevent other blocks from running.

--

Father Chrysostomos

[p5pRT]

From @xsawyerx

On 12/17/2017 09​:17 PM, Father Chrysostomos via RT wrote​:

On Sun, 17 Dec 2017 03​:06​:01 -0800, xsawyerx@​gmail.com wrote​:

On 12/13/2017 12​:04 AM, Leon Timmermans wrote​:

On Tue, Dec 12, 2017 at 11​:03 PM, Leon Timmermans <fawaka@​gmail.com
<mailto​:fawaka@​gmail.com>> wrote​:

On Tue\, Dec 12\, 2017 at 8&#8203;:13 PM\, Zefram \<zefram@&#8203;fysh\.org
\<mailto&#8203;:zefram@&#8203;fysh\.org>> wrote&#8203;:

    However\, as far as I can see there's no need at all for the
    module to
    delay execution of the code it generates by putting it in an
    INIT block\.
    If that code is instead executed immediately\, by deleting the
    word "INIT"
    to turn it into a bare block\, everything works\.  With that
    alteration
    to the module\, the Module\-Install distro passes its test suite\.


Given Module&#8203;::Install's rather unfortunate bundling nature\, that
would require rereleasing all 119 distributions using it to be
rereleased with such a new Module&#8203;::Install\.

Well, all 119 modules using Module​::Install​::DSL, Module​::Install in
general has quite a few more users.
That is indeed a pain.
I seem to remember an old blog or list post by Michael Schwern predicting this very problem with Module​::Install.

What is the cost of reverting this commit instead?
Buggy, unpredictable behaviour, unless you can memorise the list of complex rules for when exit does and does not prevent other blocks from running.

Are you referring to long-term or immediate effect?

I'm wondering about temporary revert.

[p5pRT]

From @cpansprout

On Mon, 18 Dec 2017 07​:39​:32 -0800, xsawyerx@​gmail.com wrote​:

On 12/17/2017 09​:17 PM, Father Chrysostomos via RT wrote​:

On Sun, 17 Dec 2017 03​:06​:01 -0800, xsawyerx@​gmail.com wrote​:

On 12/13/2017 12​:04 AM, Leon Timmermans wrote​:

On Tue, Dec 12, 2017 at 11​:03 PM, Leon Timmermans <fawaka@​gmail.com
<mailto​:fawaka@​gmail.com>> wrote​:

On Tue, Dec 12, 2017 at 8​:13 PM, Zefram <zefram@​fysh.org
<mailto​:zefram@​fysh.org>> wrote​:

However, as far as I can see there's no need at all for the
module to
delay execution of the code it generates by putting it in an
INIT block.
If that code is instead executed immediately, by deleting the
word "INIT"
to turn it into a bare block, everything works.  With that
alteration
to the module, the Module-Install distro passes its test suite.

Given Module​::Install's rather unfortunate bundling nature, that
would require rereleasing all 119 distributions using it to be
rereleased with such a new Module​::Install.

Well, all 119 modules using Module​::Install​::DSL, Module​::Install
in
general has quite a few more users.
That is indeed a pain.
I seem to remember an old blog or list post by Michael Schwern
predicting this very problem with Module​::Install.

What is the cost of reverting this commit instead?
Buggy, unpredictable behaviour, unless you can memorise the list of
complex rules for when exit does and does not prevent other blocks
from running.

Are you referring to long-term or immediate effect?

I’m referring to the bugs that Zefram fixed, which went even deeper than Zefram realized when he was fixing them. So at present nobody but he can predict which blocks will trigger at what time if ‘exit’ is used in them.

I'm wondering about temporary revert.

That might be a good approach​: revert and then reinstate after 5.28. In the mean time, a bunch of modules need to be released.

--

Father Chrysostomos

[p5pRT]

From @xsawyerx

On 12/18/2017 07​:39 PM, Father Chrysostomos via RT wrote​:

On Mon, 18 Dec 2017 07​:39​:32 -0800, xsawyerx@​gmail.com wrote​:

On 12/17/2017 09​:17 PM, Father Chrysostomos via RT wrote​:

On Sun, 17 Dec 2017 03​:06​:01 -0800, xsawyerx@​gmail.com wrote​:

On 12/13/2017 12​:04 AM, Leon Timmermans wrote​:

On Tue, Dec 12, 2017 at 11​:03 PM, Leon Timmermans <fawaka@​gmail.com
<mailto​:fawaka@​gmail.com>> wrote​:

On Tue, Dec 12, 2017 at 8​:13 PM, Zefram <zefram@​fysh.org
<mailto​:zefram@​fysh.org>> wrote​:

However, as far as I can see there's no need at all for the
module to
delay execution of the code it generates by putting it in an
INIT block.
If that code is instead executed immediately, by deleting the
word "INIT"
to turn it into a bare block, everything works.  With that
alteration
to the module, the Module-Install distro passes its test suite.

Given Module​::Install's rather unfortunate bundling nature, that
would require rereleasing all 119 distributions using it to be
rereleased with such a new Module​::Install.

Well, all 119 modules using Module​::Install​::DSL, Module​::Install
in
general has quite a few more users.
That is indeed a pain.
I seem to remember an old blog or list post by Michael Schwern
predicting this very problem with Module​::Install.

What is the cost of reverting this commit instead?
Buggy, unpredictable behaviour, unless you can memorise the list of
complex rules for when exit does and does not prevent other blocks
from running.
Are you referring to long-term or immediate effect?
I’m referring to the bugs that Zefram fixed, which went even deeper than Zefram realized when he was fixing them. So at present nobody but he can predict which blocks will trigger at what time if ‘exit’ is used in them.

I'm wondering about temporary revert.
That might be a good approach​: revert and then reinstate after 5.28. In the mean time, a bunch of modules need to be released.

Exactly. A better phrasing for my question is "How problematic would it
be to postpone this fix to after 5.28 so we could fix the modules and
release?"

Zefram, can you please weigh in here?

[p5pRT]

From zefram@fysh.org

Sawyer X wrote​:

Exactly. A better phrasing for my question is "How problematic would it
be to postpone this fix to after 5.28 so we could fix the modules and
release?"

The impact of retaining the bug for another year should be low.
It's a long-standing bug, with fairly obscure triggering conditions.
There's little internally that depends on the bug being fixed; only the
documentation would have to change to match. If there's a significant
win to be made in CPAN readiness for the fix then it would be a reasonable
course of action.

-zefram

[p5pRT]

From @karenetheridge

On Thu, 21 Dec 2017 23​:03​:05 -0800, zefram@​fysh.org wrote​:

If there's a significant win to be made in CPAN readiness for the fix...

Is kicking the can down the road a significant win?

[p5pRT]

From @andk

Also affected (discovered by Slaven)​:

  MANWAR/Test-Strict-0.40.tar.gz

--
andreas

[p5pRT]

From @xsawyerx

On 12/22/2017 09​:02 AM, Zefram wrote​:

Sawyer X wrote​:

Exactly. A better phrasing for my question is "How problematic would it
be to postpone this fix to after 5.28 so we could fix the modules and
release?"
[...] If there's a significant
win to be made in CPAN readiness for the fix then it would be a reasonable
course of action.

I'm not sure there is such readiness. We are fairly behind.

Considering this fix is for such a long-standing low-priority bug,
perhaps we could provide temporary warning for now?