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: fix "follow => 1" on Windows #20008

Merged
merged 3 commits into from
Aug 2, 2022
Merged

Conversation

xenu
Copy link
Member

@xenu xenu commented 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 #19995

xenu and others added 3 commits July 28, 2022 10:21
This commit doesn't contain any functional changes. If you're
seeing it in "git blame" output, try using -w switch, it will
hide whitespace-only changes.
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 Author

xenu commented Jul 28, 2022

Note for reviewers: one of the commits converts tabs to spaces, make sure to hide whitespace changes.

@sisyphus
Copy link
Contributor

This PR fixes the issue with Module::Pluggable. Thanks @xenu, for digging through all of that.
However, it turns out that the issue with Module::Find is a different one - and not addressed by this PR..

The failing test script in Module::Find is t/07-symlinks.t.
That file tests to see whether symlinks are available or not by running:

SKIP: {
    eval { symlink($dirName, $linkName) };
    skip "Symlinks not supported on this system", 13 if $@;
.....

On my Windows systems, with perl-5.32 (File-Find-1.37), $@ is set and all 13 tests are skipped - as is the correct thing to do, IIUC.
But with perl-5.34, 5.36, and this PR, $@ is unset and it is therefore assumed (incorrectly) that symlinking is available.
Of course, that symlink() call has returned 0, but it seems that a return of 0 merely signifies that the call failed - not that symlinking is unavailable.
The symlink documentation says:

>perldoc -f symlink
    symlink OLDFILE,NEWFILE
            Creates a new filename symbolically linked to the old filename.
            Returns 1 for success, 0 otherwise. On systems that don't
            support symbolic links, raises an exception. To check for that,
            use eval:

                my $symlink_exists = eval { symlink("",""); 1 };

            Portability issues: "symlink" in perlport.

which is the behaviour that t/07-symlinks.t is checking for.
Unfortunately that documentation is, as of File-Find-1.39, no longer correct on Windows systems that don't support symlinks.

Cheers,
Rob

@Leont
Copy link
Contributor

Leont commented Jul 28, 2022

That check in Module::Find isn't quite what the docs say it should be, and should be changed IMO.

@sisyphus
Copy link
Contributor

That check in Module::Find isn't quite what the docs say it should be, and should be changed IMO

But even if it strictly adheres to the docs, I still get different results between 5.32 and this PR.

With perl-5.32:

C:\>perl -e "$x = symlink('',''); print $x;"
The symlink function is unimplemented at -e line 1.

C:\>

With this PR:

C:\>perl -e "$x = symlink('',''); print $x;"
0
C:\>

IIU the symlink() docs correctly, we should be getting an exception in both cases - given that symlinks are unsupported on this system of mine.

Cheers,
Rob

@Leont
Copy link
Contributor

Leont commented Jul 28, 2022

IIU the symlink() docs correctly, we should be getting an exception in both cases - given that symlinks are unsupported on this system of mine.

The OS supports it, they're just not enabled. Hence symlink returns 0.

@xenu
Copy link
Member Author

xenu commented Jul 28, 2022

The best ways to check if you have the required permissions to create a symlink on Windows are:

@blues1875
Copy link

I have never used symlinks on Windows before, but I find I can create them on my Windows 10 machine (as Administrator). I did a basic test of File::Find using my Strawberrry Perl 5.32 and 5.36 installations, with File::Find 1.37 and 1.40 respectively. This is what I found:

  • File::Find 1.37

    • follow unset: find follows the symlink
    • follow => 0: find follows the symlink
    • follow => 1: find follows the symlink
  • File::Find 1.40

    • follow unset: find does not follow the symlink
    • follow => 0: find does not follow the symlink
    • follow => 1: find follows the symlink

It looks to me like the functionality is correct for 1.40.
I see the documentation for 1.37 has the warning: "This is a no-op on Win32" for the 'follow' option. I'm not sure if that means that the module is not expected to follow symlinks or that the option doesn't work. It seems that the functionality to follow symlinks does work (on Windows 10 at least), but the option to disable it doesn't.
Interestingly, although the behaviour seems to have been fixed in 1.40, the caveat in the documentation is still there.

@xenu
Copy link
Member Author

xenu commented Jul 28, 2022

the caveat in the documentation is still there.

I noticed it too, which is why this PR removes it :)

@sisyphus
Copy link
Contributor

The OS supports it, they're just not enabled. Hence symlink returns 0.

Ok - If no-one beats me to it, I'll update crenz/Module-Find#9 accordingly.
Use of Win32::IsSymlinkCreationAllowed() is apparently only available beginning perl-5.34.0 but I think it's possible to make use of symlink() in a way that does what is needed here, anyway.

Cheers,
Rob

@xenu
Copy link
Member Author

xenu commented Jul 29, 2022

Use of Win32::IsSymlinkCreationAllowed() is apparently only available beginning perl-5.34.0

That's also the version where support for symlinks was introduced. Basically, when you're on Windows and $Config{d_symlink} is true, Win32::IsSymlinkCreationAllowed() is guaranteed to be present.

my $can_symlinks_be_used = sub {
  return 0 if !$Config{d_symlink};

  if ($^O eq 'MSWin32') {
    require Win32;
    return Win32::IsSymlinkCreationAllowed();
  }
  else {
    return 1;
  }
}->();

@xenu xenu merged commit 414f14d into Perl:blead Aug 2, 2022
@xenu xenu deleted the xenu/file-find-win32 branch August 2, 2022 00:20
@sisyphus
Copy link
Contributor

sisyphus commented Aug 2, 2022

Can/will this fix be backported to future 5.34.x and 5.36.x releases ?

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File-Find-1.39 (and later) breaks Module::Pluggable and Module::Find
5 participants