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

File-Find-1.39 (and later) breaks Module::Pluggable and Module::Find #19995

Closed
sisyphus opened this issue Jul 25, 2022 · 14 comments · Fixed by #20008
Closed

File-Find-1.39 (and later) breaks Module::Pluggable and Module::Find #19995

sisyphus opened this issue Jul 25, 2022 · 14 comments · Fixed by #20008
Labels
ext-File-Find issues in the blead-only File-Find distribution

Comments

@sisyphus
Copy link
Contributor

Module:
File::Find
Version 1.37 (which shipped with perl-5.32.x) is ok. I can't find version 1.38 in any stable perl release.
The problem is arising only with version 1.39 (which shipped with perl-5.34.0) and later.

Description

Module::Pluggable and Module::Find fail tests because of the new changes wrt Win32 symlinks support in File::Find.
Both MSVC-built and MinGW-built perls are affected in the same way.

Steps to Reproduce

cpan -i Module::Pluggable
cpan -i Module::Find

The question is:
Should Module::Pluggable and Module::Find be patched to accommodate these recent changes to File::Find, or should File::Find be patched such that it doesn't break these 2 cpan modules in the first place ?

The Module-Pluggable bug report is at simonwistow/Module-Pluggable#24.
This patch to Module-Pluggable-Object.pm allows Module::Pluggable to build successfully against these problematic versions of File::Find:

--- Object.pm_orig	2022-07-25 11:42:24 +1000
+++ Object.pm	2022-07-25 12:12:02 +1000
@@ -66,6 +66,7 @@
     $self->{'on_instantiate_error'} ||= sub { my ($plugin, $err) = @_; carp "Couldn't instantiate $plugin: $err"; return 0 };
 
     # default whether to follow symlinks
+    $self->{'follow_symlinks'} = 0 if ($^O eq 'MSWin32' && !exists $self->{'follow_symlinks'});
     $self->{'follow_symlinks'} = 1 unless exists $self->{'follow_symlinks'};
 
     # check to see if we're running under test

There's a bug report for Module::Find at crenz/Module-Find#9 .
No patch for that one, yet, AFAIK.

I'm on a Windows 7 machine - which might be relevant.
I'm not sure which Windows version is being used by the others who have hit this issue.
As the latest Strawberry Perl is still at 5.32.1 (File-Find-1.37), the number of these "others" is probably quite small.

Perl configuration

Summary of my perl5 (revision 5 version 36 subversion 0) configuration:

  Platform:
    osname=MSWin32
    osvers=6.1.7601
    archname=MSWin32-x64-multi-thread
    uname=''
    config_args='undef'
    hint=recommended
    useposix=true
    d_sigaction=undef
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=undef
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='gcc'
    ccflags =' -DWIN32 -DWIN64 -D_WIN32_WINNT=0x0601 -fdiagnostics-color=never -
DPERL_TEXTMODE_SCRIPTS -DMULTIPLICITY -DPERL_IMPLICIT_SYS -DUSE_PERLIO -D__USE_M
INGW_ANSI_STDIO -fwrapv -fno-strict-aliasing -mms-bitfields'
    optimize='-O2'
    cppflags='-DWIN32'
    ccversion=''
    gccversion='11.3.0'
    gccosandvers=''
    intsize=4
    longsize=4
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='long long'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='g++'
    ldflags ='-s -L"C:\perl-5.36.0\lib\MSWin32-x64-multi-thread\CORE" -L"C:\_64\
winlibs-gcc-1130\mingw64\lib" -L"C:\_64\winlibs-gcc-1130\mingw64\x86_64-w64-ming
w32\lib" -L"C:\_64\winlibs-gcc-1130\mingw64\lib\gcc\x86_64-w64-mingw32\11.3.0"'
    libpth=C:\_64\winlibs-gcc-1130\mingw64\lib C:\_64\winlibs-gcc-1130\mingw64\x
86_64-w64-mingw32\lib C:\_64\winlibs-gcc-1130\mingw64\lib\gcc\x86_64-w64-mingw32
\11.3.0 C:\_64\msys_1130\1.0\local\lib
    libs= -lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi3
2 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversio
n -lodbc32 -lodbccp32 -lcomctl32
    perllibs= -lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladv
api32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lve
rsion -lodbc32 -lodbccp32 -lcomctl32
    libc=
    so=dll
    useshrplib=true
    libperl=libperl536.a
    gnulibc_version='10.0.0'
  Dynamic Linking:
    dlsrc=dl_win32.xs
    dlext=dll
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags='-shared -s -L"C:\perl-5.36.0\lib\MSWin32-x64-multi-thread\CORE" -
L"C:\_64\winlibs-gcc-1130\mingw64\lib" -L"C:\_64\winlibs-gcc-1130\mingw64\x86_64
-w64-mingw32\lib" -L"C:\_64\winlibs-gcc-1130\mingw64\lib\gcc\x86_64-w64-mingw32\
11.3.0"'


Characteristics of this binary (from libperl):
  Compile-time options:
    HAS_TIMES
    HAVE_INTERP_INTERN
    MULTIPLICITY
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_IMPLICIT_SYS
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    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
  Built under MSWin32
  Compiled at May 28 2022 11:14:45
  @INC:
    C:/perl-5.36.0/site/lib/MSWin32-x64-multi-thread
    C:/perl-5.36.0/site/lib
    C:/perl-5.36.0/lib/MSWin32-x64-multi-thread
    C:/perl-5.36.0/lib
@jkeenan
Copy link
Contributor

jkeenan commented Jul 25, 2022

  1. Has this been observed on any operating system other than Windows?
  2. Would you be able to bisect this problem on Windows? With an invocation (from a git checkout of the core distribution) something like one of these two:
perl Porting/bisect.pl --module=Module::Pluggable \
--start=c1ec4bdd803f587dd2ae76548bca0ae59d0fe84b \
--end=1380c4f3b8014de5b3d8522690cff7ac5c6162b9

perl Porting/bisect.pl --module=Module::Pluggable \
--start=v5.32.0 \
--end=v5.34.0

(If Module::Find has fewer CPAN prerequisites that Module::Pluggable, use that instead.)

@sisyphus
Copy link
Contributor Author

If it helps, here's the diff between File/Find.pm versions 137 and 1.39.

--- \perl-5.32.0\lib\File\Find.pm	2020-06-15 09:01:25 +1000
+++ Find.pm	2021-02-21 22:01:31 +1100
@@ -3,7 +3,7 @@
 use strict;
 use warnings;
 use warnings::register;
-our $VERSION = '1.37';
+our $VERSION = '1.39';
 require Exporter;
 require Cwd;
 
@@ -161,9 +161,8 @@
     $pre_process       = $wanted->{preprocess};
     $post_process      = $wanted->{postprocess};
     $no_chdir          = $wanted->{no_chdir};
-    $full_check        = $Is_Win32 ? 0 : $wanted->{follow};
-    $follow            = $Is_Win32 ? 0 :
-                             $full_check || $wanted->{follow_fast};
+    $full_check        = $wanted->{follow};
+    $follow            = $full_check || $wanted->{follow_fast};
     $follow_skip       = $wanted->{follow_skip};
     $untaint           = $wanted->{untaint};
     $untaint_pat       = $wanted->{untaint_pattern};
@@ -840,6 +839,9 @@
 
 =back
 
+Despite the name of the C<finddepth()> function, both C<find()> and
+C<finddepth()> perform a depth-first search of the directory hierarchy.
+
 =head2 %options
 
 The first argument to C<find()> is either a code reference to your
@@ -849,7 +851,7 @@
 
 Here are the possible keys for the hash:
 
-=over 3
+=over 4
 
 =item C<wanted>
 
@@ -893,7 +895,7 @@
 directory tree. See L</follow_fast> and L</follow_skip> below.
 If either I<follow> or I<follow_fast> is in effect:
 
-=over 6
+=over 4
 
 =item *
 
@@ -1080,9 +1082,9 @@
 in the appropriate scope. See L<warnings> for more info about lexical
 warnings.
 
-=head1 CAVEAT
+=head1 BUGS AND CAVEATS
 
-=over 2
+=over 4
 
 =item $dont_use_nlink
 
@@ -1108,12 +1110,6 @@
 
 =back
 
-=head1 BUGS AND CAVEATS
-
-Despite the name of the C<finddepth()> function, both C<find()> and
-C<finddepth()> perform a depth-first search of the directory
-hierarchy.
-
 =head1 HISTORY
 
 File::Find used to produce incorrect results if called recursively.

Also, there's some previous discussion about this starting at StrawberryPerl/Perl-Dist-Strawberry#39 (comment)

@sisyphus
Copy link
Contributor Author

sisyphus commented Jul 25, 2022 via email

@jkeenan
Copy link
Contributor

jkeenan commented Jul 25, 2022 via email

@hvds
Copy link
Contributor

hvds commented Jul 25, 2022

That diff encompasses a range of commits. When we're diagnosing BBCs, we try whenever possible to identify the exact commit that is associated with the breakage. The first start/end pair is a small range which I suspect encompasses the range in your diff.

However since it appears to be an issue with symlinks, looking first at 0d00729 would seem like a reasonable thing to do.

Attempting this bisection would have the additional benefit that it would let us know whether Porting/bisect.pl works on Windows or not.

If you want someone to check if bisection works on Windows, it would be better to ask for that specifically - ideally from someone that is already familiar with how the bisect tool works.

@Leont
Copy link
Contributor

Leont commented Jul 26, 2022

However since it appears to be an issue with symlinks, looking first at 0d00729 would seem like a reasonable thing to do.

Agreed

@karenetheridge karenetheridge added the ext-File-Find issues in the blead-only File-Find distribution label Jul 26, 2022
@sisyphus
Copy link
Contributor Author

However since it appears to be an issue with symlinks, looking first at 0d00729 would seem like a reasonable thing to do.

I just want to point out that, for the purposes of understanding what has changed between perl-5.32.x and perl-5.34.0, one only has to look at the changes in that commit that were made to Find.pm.
AFAICT, all of those other changes were made for testing purposes - none of them have any affect on the perl that actually gets installed.

So, ignoring the Find.pm version bump, the only code changes were that:

$full_check        = $Is_Win32 ? 0 : $wanted->{follow};
became:
$full_check        = $wanted->{follow};

and

$follow            = $Is_Win32 ? 0 :  $full_check || $wanted->{follow_fast};
became:
$follow            = $full_check || $wanted->{follow_fast};

But this change apparently doesn't allow for some subtle difference between Windows and non-Windows systems.
And that subtle difference breaks both Module::Pluggable and Module::Find.

On the face of it, my hunch is that this should be addressed in File::Find ... but my understanding of what's happening here is poor, and maybe it's instead something that Module::Pluggable and Module::Find need to address.

Cheers,
Rob

@Leont
Copy link
Contributor

Leont commented Jul 27, 2022

Module::Pluggable and Module::Find fail tests because of the new changes wrt Win32 symlinks support in File::Find.
Both MSVC-built and MinGW-built perls are affected in the same way.

Can you actually describe the difference in results between 1.37 and 1.39. That would help in figuring out what is truly going on.

Just a wild hunch: what do the first two values of stat return? There's some deduplication going on in Follow_SymLink, if $DEV and $INO always have the same values, it would erroneously conclude certain files are the same.

@sisyphus
Copy link
Contributor Author

Can you actually describe the difference in results between 1.37 and 1.39. That would help in figuring out what is truly going on

I was thinking that the patch to Module::Pluggable (at #19995 (comment)) which works around the issue would be the key to understanding what's going on ... and completely overlooked the fact that the actual failures themselves had not been provided.
The test failures (with Module-Pluggable-5.2) are provided in the attached fail.txt below.
The original bug report at simonwistow/Module-Pluggable#24 is a little difficult to follow, but I think the gist of it is that File::Find should not be enforcing 'follow_symlinks' on Windows if the caller has not provided that option.
Failures are identical for both perl-5.34.0 (File-Find-1.39) and perl-5.36.0 (File-Find-1.40).
One way of getting these tests to pass is to apply the Module/Pluggable/Object.pm one-line patch that I just mentioned.
The other way to get them to pass is to revert the File::Find changes so that it specifies

$full_check        = $Is_Win32 ? 0 : $wanted->{follow};
$follow            = $Is_Win32 ? 0 : $full_check || $wanted->{follow_fast};

as it was in File-Find-1.37.
The Module::Find test failures are listed at crenz/Module-Find#9

Just a wild hunch: what do the first two values of stat return?

Sorry ... what do you want me to stat ?

Cheers,
Rob

fail.txt

@Leont
Copy link
Contributor

Leont commented Jul 27, 2022

Sorry ... what do you want me to stat ?

A symlink. Any symlink.

@xenu
Copy link
Member

xenu commented Jul 28, 2022

@nanis implemented a fix for this issue a few months ago, but it was never submitted as a PR. He wrote about it on his blog (part 1, part 2).

It's a hack, but I don't think we could do better without completely rewriting File::Find. That module is a mess. I'll clean-up his code and submit it as a PR soon.

@sisyphus
Copy link
Contributor Author

A symlink. Any symlink
@Leont, I can't create a symlink on either my Windows 7 system or my Windows 10 laptop (where the same issue exists), and I wondered if that in itself was at the root of the problem.
It would be interesting to know if the same issue was arising on Windows 7 & 10 systems where symlinks were creatable either with or without admin privileges.
According to google it's pretty simple to make the creation of symlinks do-able on both Windows 7 and Windows 10 but, after a couple of hours of trying, I'm damned if I've been able to attain the necessary permission level.
Before banging my head further against that particular brick wall, I might wait and see what effect @xenu's soon-to-be-proposed PR has.

Cheers,
Rob

xenu pushed a commit to xenu/perl5 that referenced this issue Jul 28, 2022
File::Find's code expects unix-style paths and it manipulates them using
basic string operations. That code is very fragile, and ideally we
should make it use File::Spec, but that would involve rewriting almost
the whole module.

Instead, we made it convert backslashes to slashes and handle drive
letters.

Note from xenu: this commit was adapted from the PR linked in this
blogpost[1]. I have squashed it, written the commit message and slightly
modified the code.

[1] - https://www.nu42.com/2021/09/canonical-paths-file-find-way-forward.html

Fixes Perl#19995
xenu pushed a commit to xenu/perl5 that referenced this issue Jul 28, 2022
File::Find's code expects unix-style paths and it manipulates them using
basic string operations. That code is very fragile, and ideally we
should make it use File::Spec, but that would involve rewriting almost
the whole module.

Instead, we made it convert backslashes to slashes and handle drive
letters.

Note from xenu: this commit was adapted from the PR linked in this
blogpost[1]. I have squashed it, written the commit message and slightly
modified the code.

[1] - https://www.nu42.com/2021/09/canonical-paths-file-find-way-forward.html

Fixes Perl#19995
@xenu
Copy link
Member

xenu commented Jul 28, 2022

And here it is: #20008

@tonycoz
Copy link
Contributor

tonycoz commented Aug 1, 2022

I can't create a symlink on either my Windows 7 system or my Windows 10 laptop (where the same issue exists), and I wondered if that in itself was at the root of the problem.

You should be able to create a symlink in an elevated shell on either1, using mklink or symlink in perl.

For a fully updated windows 10, you can enable developer mode and normal users will be able to create symbolic links.

Perl's symbolic link creation support depends on an API added in Vista.

Footnotes

  1. unless you tested on a non-NTFS filesystem.

xenu pushed a commit that referenced this issue Aug 2, 2022
File::Find's code expects unix-style paths and it manipulates them using
basic string operations. That code is very fragile, and ideally we
should make it use File::Spec, but that would involve rewriting almost
the whole module.

Instead, we made it convert backslashes to slashes and handle drive
letters.

Note from xenu: this commit was adapted from the PR linked in this
blogpost[1]. I have squashed it, written the commit message and slightly
modified the code.

[1] - https://www.nu42.com/2021/09/canonical-paths-file-find-way-forward.html

Fixes #19995
scottchiefbaker pushed a commit to scottchiefbaker/perl5 that referenced this issue Nov 3, 2022
File::Find's code expects unix-style paths and it manipulates them using
basic string operations. That code is very fragile, and ideally we
should make it use File::Spec, but that would involve rewriting almost
the whole module.

Instead, we made it convert backslashes to slashes and handle drive
letters.

Note from xenu: this commit was adapted from the PR linked in this
blogpost[1]. I have squashed it, written the commit message and slightly
modified the code.

[1] - https://www.nu42.com/2021/09/canonical-paths-file-find-way-forward.html

Fixes Perl#19995
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Dec 18, 2023
    0.16, 2022-08-01
            Fixes an issue where symlink tests failed on systems that do not
            support creation of symlinks. The issue appears on Windows
            systems due to changed behaviour in "File::Find" described in
            perl5/issue #19995 <Perl/perl5#19995>
            Symlink tests were previously skipped if symlink() is not
            available, and now also if creation of a symlink is not
            possible.

            Fixes issue #9 <crenz/Module-Find#9>.
            Note that on Windows system, the patch to "File::Find" from
            perl5/PR #20008 <Perl/perl5#20008> will
            be required for proper operation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext-File-Find issues in the blead-only File-Find distribution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants