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

Bleadperl v5.25.3-22-g8901dde breaks FLORA/Class-C3-XS-0.13.tar.gz #15484

Open
p5pRT opened this issue Jul 29, 2016 · 32 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Jul 29, 2016

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

Searchable as RT128769$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 29, 2016

From @andk

bisect


commit 8901dde
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jun 23 14​:06​:40 2016 +1000

  dist/​: remove . from @​INC when loading optional modules

diagnostics


5.25.3 http​://www.cpantesters.org/cpan/report/74f15440-5329-11e6-9a64-21b9ed09dfea
5.24.1-RC2 http​://www.cpantesters.org/cpan/report/5780f794-545a-11e6-9930-56f0ed09dfea
5.22.3-RC2 http​://www.cpantesters.org/cpan/report/9de3b628-530f-11e6-9fdd-758fed09dfea

affected


FLORA/Class-C3-XS-0.13.tar.gz
DGL/deferred-0.01.tar.gz
JABRA/MetasploitExpress-Parser-0.02.tar.gz
YAPPO/Module-Setup-0.09.tar.gz
JABRA/NexposeSimpleXML-Parser-0.04.tar.gz
DOLMEN/POE-Component-Logger-1.10.tar.gz
FUJIWARA/Test-UNIXSock-0.1.tar.gz

perl -V


Summary of my perl5 (revision 5 version 25 subversion 4) configuration​:
  Commit id​: ef269bf
  Platform​:
  osname=linux
  osvers=4.3.0-1-amd64
  archname=x86_64-linux-thread-multi
  uname='linux k83 4.3.0-1-amd64 #1 smp debian 4.3.3-7 (2016-01-19) x86_64 gnulinux '
  config_args='-Dprefix=/home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-27-gef269bf/f7bf -Dmyhostname=k83 -Dinstallusrbinperl=n -Uversiononly -Dusedevel -des -Ui_db -Dlibswanted=cl pthread socket inet nsl gdbm dbm malloc dl ld sun m crypt sec util c cposix posix ucb BSD gdbm_compat -Duseithreads -Uuselongdouble -DDEBUGGING=-g'
  hint=recommended
  useposix=true
  d_sigaction=define
  useithreads=define
  usemultiplicity=define
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  bincompat5005=undef
  Compiler​:
  cc='cc'
  ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -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 -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
  ccversion=''
  gccversion='5.3.1 20160121'
  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/5/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 -lgdbm -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.21.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.21'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector-strong'

Characteristics of this binary (from libperl)​:
  Compile-time options​:
  HAS_TIMES
  MULTIPLICITY
  PERLIO_LAYERS
  PERL_COPY_ON_WRITE
  PERL_DONT_CREATE_GVSV
  PERL_HASH_FUNC_ONE_AT_A_TIME_HARD
  PERL_IMPLICIT_CONTEXT
  PERL_MALLOC_WRAP
  PERL_OP_PARENT
  PERL_PRESERVE_IVUV
  PERL_USE_DEVEL
  USE_64_BIT_ALL
  USE_64_BIT_INT
  USE_ITHREADS
  USE_LARGE_FILES
  USE_LOCALE
  USE_LOCALE_COLLATE
  USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC
  USE_LOCALE_TIME
  USE_PERLIO
  USE_PERL_ATOF
  USE_REENTRANT_API
  Built under linux
  Compiled at Jul 26 2016 09​:09​:16
  %ENV​:
  PERL5LIB=""
  PERL5OPT=""
  PERL5_CPANPLUS_IS_RUNNING="16182"
  PERL5_CPAN_IS_RUNNING="16182"
  PERL_CANARY_STABILITY_NOPROMPT="1"
  PERL_MM_USE_DEFAULT="1"
  @​INC​:
  /home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-27-gef269bf/f7bf/lib/site_perl/5.25.4/x86_64-linux-thread-multi
  /home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-27-gef269bf/f7bf/lib/site_perl/5.25.4
  /home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-27-gef269bf/f7bf/lib/5.25.4/x86_64-linux-thread-multi
  /home/sand/src/perl/repoperls/installed-perls/perl/v5.25.3-27-gef269bf/f7bf/lib/5.25.4
  .
 
--
andreas

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 29, 2016

From @cpansprout

On Thu Jul 28 21​:21​:06 2016, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

bisect
------
commit 8901dde
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jun 23 14​:06​:40 2016 +1000

dist/​: remove . from @​INC when loading optional modules

diagnostics
-----------
5.25.3 http​://www.cpantesters.org/cpan/report/74f15440-5329-11e6-
9a64-21b9ed09dfea
5.24.1-RC2 http​://www.cpantesters.org/cpan/report/5780f794-545a-11e6-
9930-56f0ed09dfea
5.22.3-RC2 http​://www.cpantesters.org/cpan/report/9de3b628-530f-11e6-
9fdd-758fed09dfea

I see something that really does need to be fixed before those maintenance releases are made. The error message from base.pm about making the module available in @​INC lists @​INC *with* . on the end, and mentions nothing about . having been stripped.

So we have an error message that effectively says that the module cannot be found in ./ which is a lie.

At least the diagnostics can be made more informative. Currently the message is​:

Base class package "t​::lib​::C" is empty.
  (Perhaps you need to 'use' the module which defines that package first,
  or make that module available in @​INC (@​INC contains​: blah blah blah).
at file line 42.

Maybe we need to add​:

  (Note that . was stripped from the end for security
  reasons, but "use lib '.'" can override that.)

Awful. Any better suggestions?

Should base.pm instead be made smarter and allow . if the calling package itself was loaded from the current directory? (That would fix the Class​::C3​::XS failure.)

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 29, 2016

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 29, 2016

From @cpansprout

On Thu Jul 28 21​:21​:06 2016, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

affected
--------
FLORA/Class-C3-XS-0.13.tar.gz
DGL/deferred-0.01.tar.gz
JABRA/MetasploitExpress-Parser-0.02.tar.gz

Those three are failing because of base.pm.

YAPPO/Module-Setup-0.09.tar.gz

That one depends on File​::HomeDir, so I cannot test it.

JABRA/NexposeSimpleXML-Parser-0.04.tar.gz
DOLMEN/POE-Component-Logger-1.10.tar.gz
FUJIWARA/Test-UNIXSock-0.1.tar.gz

Those three also fail because of base.pm.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 29, 2016

From @karenetheridge

On Thu Jul 28 22​:26​:27 2016, sprout wrote​:

On Thu Jul 28 21​:21​:06 2016, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

affected
--------
FLORA/Class-C3-XS-0.13.tar.gz
DGL/deferred-0.01.tar.gz
JABRA/MetasploitExpress-Parser-0.02.tar.gz

Those three are failing because of base.pm.

I'm going to do a release of Class​::C3​::XS this week, so don't worry about that one.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 31, 2016

From @cpansprout

On Thu Jul 28 21​:41​:09 2016, sprout wrote​:

I see something that really does need to be fixed before those
maintenance releases are made. The error message from base.pm about
making the module available in @​INC lists @​INC *with* . on the end,
and mentions nothing about . having been stripped.

So we have an error message that effectively says that the module
cannot be found in ./ which is a lie.

In order to get things moving, I have pushed this commit​:

commit bca5527
Author​: Father Chrysostomos <sprout@​cpan.org>
Date​: Sun Jul 31 13​:51​:36 2016 -0700

  [perl #128769] Improve base.pm @​INC '.' handling
 
  • Localise @​INC only if necessary.
  • Don’t mention '.' in the @​INC list in the error message, since it
  was not in the @​INC that was searched (this is accomplished by local-
  ising @​INC in the same scope as the error).
  • If a file exists that would have been loaded had '.' not been
  ignored, mention it and suggest ‘use lib’.
  • Use the same number of closing as opening parentheses in the
  error message.

I am hoping that it--or something like it--can be backported to the maint branches.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 1, 2016

From zefram@fysh.org

Father Chrysostomos via RT wrote​:

* Localise @​INC only if necessary.

There's obvious appeal in doing this, but on the whole I think it's
a bad idea. It means that a module loaded by base sees inconsistent
behaviour in whether @​INC is localised. If a module is going to have
a problem with localised @​INC, it is better that it consistently sees
it localised by base, so that the problem will quickly become apparent
and be addressed, rather than lurking to bite unsuspecting users later.

-zefram

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 1, 2016

From @xsawyerx

On 08/01/2016 08​:41 AM, Zefram wrote​:

Father Chrysostomos via RT wrote​:

* Localise @​INC only if necessary.
There's obvious appeal in doing this, but on the whole I think it's
a bad idea. It means that a module loaded by base sees inconsistent
behaviour in whether @​INC is localised. If a module is going to have
a problem with localised @​INC, it is better that it consistently sees
it localised by base, so that the problem will quickly become apparent
and be addressed, rather than lurking to bite unsuspecting users later.

We should determine this point in order to decide whether to include
this change or not in the changeset. The changeset will need to be sent
to vendors as it might be part of a local patch which will be applied on
vendor-maintained older versions of perl.

(I had to send to vendors the versioning fix of PathTools, for example.)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 1, 2016

From @cpansprout

On Mon Aug 01 08​:02​:24 2016, xsawyerx@​gmail.com wrote​:

On 08/01/2016 08​:41 AM, Zefram wrote​:

Father Chrysostomos via RT wrote​:

* Localise @​INC only if necessary.
There's obvious appeal in doing this, but on the whole I think it's
a bad idea. It means that a module loaded by base sees inconsistent
behaviour in whether @​INC is localised. If a module is going to have
a problem with localised @​INC, it is better that it consistently sees
it localised by base, so that the problem will quickly become apparent
and be addressed, rather than lurking to bite unsuspecting users later.

We should determine this point

Yes, but it would be nice if you would express an opinion one way on another. :-)

I think what Zefram says makes sense.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 1, 2016

From @cpansprout

On Mon Aug 01 08​:50​:17 2016, sprout wrote​:

On Mon Aug 01 08​:02​:24 2016, xsawyerx@​gmail.com wrote​:

On 08/01/2016 08​:41 AM, Zefram wrote​:

Father Chrysostomos via RT wrote​:

* Localise @​INC only if necessary.
There's obvious appeal in doing this, but on the whole I think it's
a bad idea. It means that a module loaded by base sees
inconsistent
behaviour in whether @​INC is localised. If a module is going to
have
a problem with localised @​INC, it is better that it consistently
sees
it localised by base, so that the problem will quickly become
apparent
and be addressed, rather than lurking to bite unsuspecting users
later.

We should determine this point

Yes, but it would be nice if you would express an opinion one way on
another. :-)

I think what Zefram says makes sense.

Then again, what Chris Travers writes in ticket #128800 also makes sense. I think this is the sort of situation in which the pumpking needs to decide, because nobody will ever agree.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 2, 2016

From @cpansprout

On Mon Aug 01 08​:50​:17 2016, sprout wrote​:

On Mon Aug 01 08​:02​:24 2016, xsawyerx@​gmail.com wrote​:

On 08/01/2016 08​:41 AM, Zefram wrote​:

Father Chrysostomos via RT wrote​:

* Localise @​INC only if necessary.
There's obvious appeal in doing this, but on the whole I think it's
a bad idea. It means that a module loaded by base sees
inconsistent
behaviour in whether @​INC is localised. If a module is going to
have
a problem with localised @​INC, it is better that it consistently
sees
it localised by base, so that the problem will quickly become
apparent
and be addressed, rather than lurking to bite unsuspecting users
later.

We should determine this point

Yes, but it would be nice if you would express an opinion one way on
another. :-)

I think what Zefram says makes sense.

And so I have made the change in commit 362f3f7.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 2, 2016

From @eserte

On Thu Jul 28 21​:21​:06 2016, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:
[...]

affected
--------
FLORA/Class-C3-XS-0.13.tar.gz
DGL/deferred-0.01.tar.gz
JABRA/MetasploitExpress-Parser-0.02.tar.gz
YAPPO/Module-Setup-0.09.tar.gz
JABRA/NexposeSimpleXML-Parser-0.04.tar.gz
DOLMEN/POE-Component-Logger-1.10.tar.gz
FUJIWARA/Test-UNIXSock-0.1.tar.gz

Also posibly affected​:

Tangence-0.21 (CPAN RT ticket 116579)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 2, 2016

From zefram@fysh.org

Father Chrysostomos via RT wrote​:

Then again, what Chris Travers writes in ticket #128800 also makes sense.

What Chris Travers writes is a bunch of good reasons to not perform any
@​INC twiddling for optional module loads. It pains me to argue against
it, because I made a related argument against modules twiddling @​INC early
in the closed discussion. We came to a consensus there that the ugliness
of this approach was justified in the light of the security situation.
I accept the need for this in order for the security fix to achieve
sufficient coverage.

Although base.pm incurs the ugliness especially much, it actually
provides the cases that are easiest to resolve. In almost all cases,
the optional module load can be converted to either a mandatory load or
a non-load, as appropriate, by changing the base.pm usage to parent.pm.
The arguments against changing optional module loads apply rather more
forcefully to the cases that really need to be optional.

-zefram

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 3, 2016

From @xsawyerx

On 08/01/2016 05​:50 PM, Father Chrysostomos via RT wrote​:

On Mon Aug 01 08​:02​:24 2016, xsawyerx@​gmail.com wrote​:

On 08/01/2016 08​:41 AM, Zefram wrote​:

Father Chrysostomos via RT wrote​:

* Localise @​INC only if necessary.
There's obvious appeal in doing this, but on the whole I think it's
a bad idea. It means that a module loaded by base sees inconsistent
behaviour in whether @​INC is localised. If a module is going to have
a problem with localised @​INC, it is better that it consistently sees
it localised by base, so that the problem will quickly become apparent
and be addressed, rather than lurking to bite unsuspecting users later.
We should determine this point
Yes, but it would be nice if you would express an opinion one way on another. :-)

My mail client decided to just not send my emails. Great.

I wrote that I usually wait with comments until I hear other considerations.

However, my position is that it should be consistent - the error and the
localization.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 3, 2016

From chris.travers@gmail.com

On Thu Jul 28 21​:41​:09 2016, sprout wrote​:

On Thu Jul 28 21​:21​:06 2016, andreas.koenig.7os6VVqR@​franz.ak.mind.de
wrote​:

bisect
------
commit 8901dde
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jun 23 14​:06​:40 2016 +1000

dist/​: remove . from @​INC when loading optional modules

diagnostics
-----------
5.25.3 http​://www.cpantesters.org/cpan/report/74f15440-5329-11e6-
9a64-21b9ed09dfea
5.24.1-RC2 http​://www.cpantesters.org/cpan/report/5780f794-545a-11e6-
9930-56f0ed09dfea
5.22.3-RC2 http​://www.cpantesters.org/cpan/report/9de3b628-530f-11e6-
9fdd-758fed09dfea

I see something that really does need to be fixed before those
maintenance releases are made. The error message from base.pm about
making the module available in @​INC lists @​INC *with* . on the end,
and mentions nothing about . having been stripped.

So we have an error message that effectively says that the module
cannot be found in ./ which is a lie.

At least the diagnostics can be made more informative. Currently the
message is​:

Base class package "t​::lib​::C" is empty.
(Perhaps you need to 'use' the module which defines that package
first,
or make that module available in @​INC (@​INC contains​: blah blah
blah).
at file line 42.

Maybe we need to add​:

(Note that . was stripped from the end for security
reasons, but "use lib '.'" can override that.)

Awful. Any better suggestions?

Recommending use lib '.' to get old behavior back is not what I would recommend other than as a last resort. Makes sense mostly when you have another module chdir'ing during module load or import (yikes!).

"Note that base.pm now ignores the current working directory when requiring if this was not explicitly selected. If your software worked on previous versions of Perl, the best solution is to use FindBin to detect the path properly. As a last resort, you can re-enable looking in the current working directory by adding use lib '.' to your code."

Should base.pm instead be made smarter and allow . if the calling
package itself was loaded from the current directory? (That would fix
the Class​::C3​::XS failure.)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 3, 2016

From [Unknown Contact. See original ticket]

On Thu Jul 28 21​:41​:09 2016, sprout wrote​:

On Thu Jul 28 21​:21​:06 2016, andreas.koenig.7os6VVqR@​franz.ak.mind.de
wrote​:

bisect
------
commit 8901dde
Author​: Tony Cook <tony@​develop-help.com>
Date​: Thu Jun 23 14​:06​:40 2016 +1000

dist/​: remove . from @​INC when loading optional modules

diagnostics
-----------
5.25.3 http​://www.cpantesters.org/cpan/report/74f15440-5329-11e6-
9a64-21b9ed09dfea
5.24.1-RC2 http​://www.cpantesters.org/cpan/report/5780f794-545a-11e6-
9930-56f0ed09dfea
5.22.3-RC2 http​://www.cpantesters.org/cpan/report/9de3b628-530f-11e6-
9fdd-758fed09dfea

I see something that really does need to be fixed before those
maintenance releases are made. The error message from base.pm about
making the module available in @​INC lists @​INC *with* . on the end,
and mentions nothing about . having been stripped.

So we have an error message that effectively says that the module
cannot be found in ./ which is a lie.

At least the diagnostics can be made more informative. Currently the
message is​:

Base class package "t​::lib​::C" is empty.
(Perhaps you need to 'use' the module which defines that package
first,
or make that module available in @​INC (@​INC contains​: blah blah
blah).
at file line 42.

Maybe we need to add​:

(Note that . was stripped from the end for security
reasons, but "use lib '.'" can override that.)

Awful. Any better suggestions?

Recommending use lib '.' to get old behavior back is not what I would recommend other than as a last resort. Makes sense mostly when you have another module chdir'ing during module load or import (yikes!).

"Note that base.pm now ignores the current working directory when requiring if this was not explicitly selected. If your software worked on previous versions of Perl, the best solution is to use FindBin to detect the path properly. As a last resort, you can re-enable looking in the current working directory by adding use lib '.' to your code."

Should base.pm instead be made smarter and allow . if the calling
package itself was loaded from the current directory? (That would fix
the Class​::C3​::XS failure.)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 5, 2016

From @cpansprout

On Wed Aug 03 03​:58​:11 2016, chris.travers@​gmail.com wrote​:

Recommending use lib '.' to get old behavior back is not what I would
recommend other than as a last resort. Makes sense mostly when you
have another module chdir'ing during module load or import (yikes!).

"Note that base.pm now ignores the current working directory when
requiring if this was not explicitly selected. If your software
worked on previous versions of Perl, the best solution is to use
FindBin to detect the path properly. As a last resort, you can re-
enable looking in the current working directory by adding use lib '.'
to your code."

Thank you for taking the time to write a much better message than I could come up with. :-)

What I actually ended up committing was this​:

  If you mean to load $fn from the current directory, you may
  want to try "use lib '.'".

But your message is clearer and more informative. Nevertheless, I think it could do with some tweaks, specifically to mention that it is @​INC we are talking about. (Also, I find the use of ‘when requiring’ a little arcane.) What do you think of this?

  The file t/lib/Foo.pm exists in the current directory. Note that
  base.pm, when loading a module, now ignores the current working
  directory if it the last entry in @​INC. If your software worked on
  previous versions of Perl, the best solution is to use FindBin to
  detect the path properly and to add that path to @​INC. As a last
  resort, you can re-enable looking in the current working directory by
  adding "use lib '.'" to your code.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 5, 2016

From chris.travers@gmail.com

On Thu Aug 04 22​:15​:37 2016, sprout wrote​:

On Wed Aug 03 03​:58​:11 2016, chris.travers@​gmail.com wrote​:

Recommending use lib '.' to get old behavior back is not what I would
recommend other than as a last resort. Makes sense mostly when you
have another module chdir'ing during module load or import (yikes!).

"Note that base.pm now ignores the current working directory when
requiring if this was not explicitly selected. If your software
worked on previous versions of Perl, the best solution is to use
FindBin to detect the path properly. As a last resort, you can re-
enable looking in the current working directory by adding use lib '.'
to your code."

Thank you for taking the time to write a much better message than I
could come up with. :-)

What I actually ended up committing was this​:

If you mean to load $fn from the current directory, you may
want to try "use lib '.'".

But your message is clearer and more informative. Nevertheless, I
think it could do with some tweaks, specifically to mention that it is
@​INC we are talking about. (Also, I find the use of ‘when requiring’
a little arcane.) What do you think of this?

The file t/lib/Foo.pm exists in the current directory. Note that
base.pm, when loading a module, now ignores the current working
directory if it the last entry in @​INC. If your software worked on
previous versions of Perl, the best solution is to use FindBin to
detect the path properly and to add that path to @​INC. As a last
resort, you can re-enable looking in the current working directory by
adding "use lib '.'" to your code.

Even better. I like it.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 5, 2016

From @cpansprout

On Thu Aug 04 22​:25​:28 2016, chris.travers@​gmail.com wrote​:

On Thu Aug 04 22​:15​:37 2016, sprout wrote​:

On Wed Aug 03 03​:58​:11 2016, chris.travers@​gmail.com wrote​:

Recommending use lib '.' to get old behavior back is not what I would
recommend other than as a last resort. Makes sense mostly when you
have another module chdir'ing during module load or import (yikes!).

"Note that base.pm now ignores the current working directory when
requiring if this was not explicitly selected. If your software
worked on previous versions of Perl, the best solution is to use
FindBin to detect the path properly. As a last resort, you can re-
enable looking in the current working directory by adding use lib '.'
to your code."

Thank you for taking the time to write a much better message than I
could come up with. :-)

What I actually ended up committing was this​:

If you mean to load $fn from the current directory, you may
want to try "use lib '.'".

But your message is clearer and more informative. Nevertheless, I
think it could do with some tweaks, specifically to mention that it is
@​INC we are talking about. (Also, I find the use of ‘when requiring’
a little arcane.) What do you think of this?

The file t/lib/Foo.pm exists in the current directory. Note that
base.pm, when loading a module, now ignores the current working
directory if it the last entry in @​INC. If your software worked on
previous versions of Perl, the best solution is to use FindBin to
detect the path properly and to add that path to @​INC. As a last
resort, you can re-enable looking in the current working directory by
adding "use lib '.'" to your code.

Even better. I like it.

Thank you. I have applied it as 458470f.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 7, 2016

From @cpansprout

How should we resolve this ticket now (in addition to patching the CPAN modules)?

Do my changes to base.pm need to be +1’ed by two other people so that we can backport them to the maint branches and release new RCs?

The patches in question are​:

bca5527
ec7784b
362f3f7
458470f
5e19631

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 7, 2016

From @xsawyerx

On 08/07/2016 07​:07 AM, Father Chrysostomos via RT wrote​:

How should we resolve this ticket now (in addition to patching the CPAN modules)?

Do my changes to base.pm need to be +1’ed by two other people so that we can backport them to the maint branches and release new RCs?

The patches in question are​:

bca5527
ec7784b
362f3f7
458470f
5e19631

+1

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 9, 2016

From @steve-m-hay

On 7 August 2016 at 11​:03, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 08/07/2016 07​:07 AM, Father Chrysostomos via RT wrote​:

How should we resolve this ticket now (in addition to patching the CPAN modules)?

Do my changes to base.pm need to be +1’ed by two other people so that we can backport them to the maint branches and release new RCs?

The patches in question are​:

bca5527
ec7784b
362f3f7
458470f
5e19631

+1

+1.

I'll go ahead and make new RCs with backports of these five patches
unless I hear of any more changes being required in the next day or
two. Thanks for your work on this.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 11, 2016

From @andk

Also affected


RPETTETT/ClearPress-v473.0.5.tar.gz (only with DBD​::SQLite installed,
test is otherwise skipped; already reported as
https://rt.cpan.org/Ticket/Display.html?id=116816)

--
andreas

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 6, 2016

From @jkeenan

On Tue, 09 Aug 2016 20​:37​:52 GMT, shay wrote​:

On 7 August 2016 at 11​:03, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 08/07/2016 07​:07 AM, Father Chrysostomos via RT wrote​:

How should we resolve this ticket now (in addition to patching the
CPAN modules)?

Do my changes to base.pm need to be +1’ed by two other people so
that we can backport them to the maint branches and release new RCs?

The patches in question are​:

bca5527
ec7784b
362f3f7
458470f
5e19631

+1

+1.

I'll go ahead and make new RCs with backports of these five patches
unless I hear of any more changes being required in the next day or
two. Thanks for your work on this.

Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 26, 2016

From @jkeenan

On Tue, 06 Dec 2016 22​:30​:13 GMT, jkeenan wrote​:

On Tue, 09 Aug 2016 20​:37​:52 GMT, shay wrote​:

On 7 August 2016 at 11​:03, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 08/07/2016 07​:07 AM, Father Chrysostomos via RT wrote​:

How should we resolve this ticket now (in addition to patching the
CPAN modules)?

Do my changes to base.pm need to be +1’ed by two other people so
that we can backport them to the maint branches and release new RCs?

The patches in question are​:

bca5527
ec7784b
362f3f7
458470f
5e19631

+1

+1.

I'll go ahead and make new RCs with backports of these five patches
unless I hear of any more changes being required in the next day or
two. Thanks for your work on this.

Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

Thank you very much.

Repeating the question​:

Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 26, 2016

From @xsawyerx

On 12/26/2016 03​:11 AM, James E Keenan via RT wrote​:

On Tue, 06 Dec 2016 22​:30​:13 GMT, jkeenan wrote​:

On Tue, 09 Aug 2016 20​:37​:52 GMT, shay wrote​:

On 7 August 2016 at 11​:03, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 08/07/2016 07​:07 AM, Father Chrysostomos via RT wrote​:

How should we resolve this ticket now (in addition to patching the
CPAN modules)?

Do my changes to base.pm need to be +1’ed by two other people so
that we can backport them to the maint branches and release new RCs?

The patches in question are​:

bca5527
ec7784b
362f3f7
458470f
5e19631
+1

+1.

I'll go ahead and make new RCs with backports of these five patches
unless I hear of any more changes being required in the next day or
two. Thanks for your work on this.
Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

Thank you very much.
Repeating the question​:

Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

(Apparently adding "shift" just removes the email I was about to send. Yay.)

These were all merged but I think we should remove them in favor of the
upcoming revised patch (based on Chris' comments) by Aristotle, work in
progress is on ap/baseincguard.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 20, 2017

From @iabyn

On Mon, Dec 26, 2016 at 06​:07​:55PM +0100, Sawyer X wrote​:

On 12/26/2016 03​:11 AM, James E Keenan via RT wrote​:

On Tue, 06 Dec 2016 22​:30​:13 GMT, jkeenan wrote​:

On Tue, 09 Aug 2016 20​:37​:52 GMT, shay wrote​:

On 7 August 2016 at 11​:03, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 08/07/2016 07​:07 AM, Father Chrysostomos via RT wrote​:

How should we resolve this ticket now (in addition to patching the
CPAN modules)?

Do my changes to base.pm need to be +1’ed by two other people so
that we can backport them to the maint branches and release new RCs?

The patches in question are​:

bca5527
ec7784b
362f3f7
458470f
5e19631
+1

+1.

I'll go ahead and make new RCs with backports of these five patches
unless I hear of any more changes being required in the next day or
two. Thanks for your work on this.
Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

Thank you very much.
Repeating the question​:

Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

(Apparently adding "shift" just removes the email I was about to send. Yay.)

These were all merged but I think we should remove them in favor of the
upcoming revised patch (based on Chris' comments) by Aristotle, work in
progress is on ap/baseincguard.

Are we still planning to do this for 5.26.0? If not, can this ticket
be removed from the blockers list?

--
No man treats a motor car as foolishly as he treats another human being.
When the car will not go, he does not attribute its annoying behaviour to
sin, he does not say, You are a wicked motorcar, and I shall not give you
any more petrol until you go. He attempts to find out what is wrong and
set it right.
  -- Bertrand Russell,
  Has Religion Made Useful Contributions to Civilization?

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 20, 2017

From @jkeenan

On Mon, 20 Mar 2017 12​:53​:42 GMT, davem wrote​:

On Mon, Dec 26, 2016 at 06​:07​:55PM +0100, Sawyer X wrote​:

On 12/26/2016 03​:11 AM, James E Keenan via RT wrote​:

On Tue, 06 Dec 2016 22​:30​:13 GMT, jkeenan wrote​:

On Tue, 09 Aug 2016 20​:37​:52 GMT, shay wrote​:

On 7 August 2016 at 11​:03, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 08/07/2016 07​:07 AM, Father Chrysostomos via RT wrote​:

How should we resolve this ticket now (in addition to patching
the
CPAN modules)?

Do my changes to base.pm need to be +1’ed by two other people
so
that we can backport them to the maint branches and release new
RCs?

The patches in question are​:

bca5527
ec7784b
362f3f7
458470f
5e19631
+1

+1.

I'll go ahead and make new RCs with backports of these five
patches
unless I hear of any more changes being required in the next day
or
two. Thanks for your work on this.
Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

Thank you very much.
Repeating the question​:

Has the work which P5P had to do for this RT been completed?

If so, do we have a plan for the BBC issues?

(Apparently adding "shift" just removes the email I was about to
send. Yay.)

These were all merged but I think we should remove them in favor of
the
upcoming revised patch (based on Chris' comments) by Aristotle, work
in
progress is on ap/baseincguard.

Are we still planning to do this for 5.26.0? If not, can this ticket
be removed from the blockers list?

My vote would be to remove it from the blockers list. Even if the work in the ap/baseincguard were to be completed, there's not sufficient time to test it against CPAN before full code freeze.

But this is a pumpking's call.

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 21, 2017

From @haarg

On Mon, Mar 20, 2017 at 4​:37 PM, James E Keenan via RT
<perlbug-followup@​perl.org> wrote​:

On Mon, 20 Mar 2017 12​:53​:42 GMT, davem wrote​:

On Mon, Dec 26, 2016 at 06​:07​:55PM +0100, Sawyer X wrote​:

These were all merged but I think we should remove them in favor of
the
upcoming revised patch (based on Chris' comments) by Aristotle, work
in
progress is on ap/baseincguard.

Are we still planning to do this for 5.26.0? If not, can this ticket
be removed from the blockers list?

My vote would be to remove it from the blockers list. Even if the work in the ap/baseincguard were to be completed, there's not sufficient time to test it against CPAN before full code freeze.

But this is a pumpking's call.

Thank you very much.

Can't we just revert the base.pm changes without putting anything
extra in place? We're already removing . from @​INC globally. There's
little need for base.pm to have its own protections, especially given
how broken the current implementation is.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 21, 2017

From @xsawyerx

On 03/21/2017 12​:51 PM, Graham Knop wrote​:

On Mon, Mar 20, 2017 at 4​:37 PM, James E Keenan via RT
<perlbug-followup@​perl.org> wrote​:

On Mon, 20 Mar 2017 12​:53​:42 GMT, davem wrote​:

On Mon, Dec 26, 2016 at 06​:07​:55PM +0100, Sawyer X wrote​:

These were all merged but I think we should remove them in favor of
the
upcoming revised patch (based on Chris' comments) by Aristotle, work
in
progress is on ap/baseincguard.
Are we still planning to do this for 5.26.0? If not, can this ticket
be removed from the blockers list?
My vote would be to remove it from the blockers list. Even if the work in the ap/baseincguard were to be completed, there's not sufficient time to test it against CPAN before full code freeze.

But this is a pumpking's call.

Thank you very much.
Can't we just revert the base.pm changes without putting anything
extra in place? We're already removing . from @​INC globally. There's
little need for base.pm to have its own protections, especially given
how broken the current implementation is.

This makes sense.

I spoke to Graham more about this on #p5p. There's no need to have an
altered base.pm with 5.26.0 (because safe @​INC is turned on by default
anyway). When base.pm is released separately, it will simply be a
different (and up-to-date) version.

So we should undo the changes to base.pm in blead. Post 5.26 we could
sync it with CPAN.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 24, 2017

From @iabyn

On Tue, Mar 21, 2017 at 09​:17​:17PM +0100, Sawyer X wrote​:

On 03/21/2017 12​:51 PM, Graham Knop wrote​:

On Mon, Mar 20, 2017 at 4​:37 PM, James E Keenan via RT
<perlbug-followup@​perl.org> wrote​:

On Mon, 20 Mar 2017 12​:53​:42 GMT, davem wrote​:

On Mon, Dec 26, 2016 at 06​:07​:55PM +0100, Sawyer X wrote​:

These were all merged but I think we should remove them in favor of
the
upcoming revised patch (based on Chris' comments) by Aristotle, work
in
progress is on ap/baseincguard.
Are we still planning to do this for 5.26.0? If not, can this ticket
be removed from the blockers list?
My vote would be to remove it from the blockers list. Even if the work in the ap/baseincguard were to be completed, there's not sufficient time to test it against CPAN before full code freeze.

But this is a pumpking's call.

Thank you very much.
Can't we just revert the base.pm changes without putting anything
extra in place? We're already removing . from @​INC globally. There's
little need for base.pm to have its own protections, especially given
how broken the current implementation is.

This makes sense.

I spoke to Graham more about this on #p5p. There's no need to have an
altered base.pm with 5.26.0 (because safe @​INC is turned on by default
anyway). When base.pm is released separately, it will simply be a
different (and up-to-date) version.

So we should undo the changes to base.pm in blead. Post 5.26 we could
sync it with CPAN.

I've just pushed the following 3 commits​:

commit 6ee05a9
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Fri Mar 24 08​:10​:12 2017 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Fri Mar 24 08​:34​:06 2017 +0000

  Revert base.pm's dot-in-INC changes.
 
  This reverts​:
  458470f
  362f3f7
  bca5527
  and the base.pm part of
  8901dde
 
  This commit removes all the recent stuff that made base.pm localise
  @​INC and remove a trailing '.'.
 
  This is because perl 5.26.0 will be released with '.' in @​INC disabled by
  default.
 
  See RT #128769.

commit 2250cd0
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Fri Mar 24 08​:30​:26 2017 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Fri Mar 24 08​:34​:12 2017 +0000

  bump base.pm $VERSION and un-CUSTOMISE
 
  follow-up to the previous commit's reverting of base.pm @​INC changes.

commit 939e7f2
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Fri Mar 24 08​:48​:32 2017 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Fri Mar 24 08​:48​:32 2017 +0000

  INSTALL​: add entry for -Ddefault_inc_excludes_dot

--
My Dad used to say 'always fight fire with fire', which is probably why
he got thrown out of the fire brigade.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 24, 2017

From @iabyn

On Fri, Mar 24, 2017 at 09​:00​:25AM +0000, Dave Mitchell wrote​:

I've just pushed the following 3 commits​:

.. and am removing this ticket from the 5.26.0 blockers list.

I'm not sure whether the ticket can be closed.

--
Dave's first rule of Opera​:
If something needs saying, say it​: don't warble it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.