Implementation by Wishful Thinking

It is natural to want the world to be simpler. To wish legacy code away. To want to avoid dealing with platform specific differences.

It is natural. But, in most cases, it also not possible for the world to be simpler. Legacy code is always there. And, there are unavoidable platform specific differences.

I brought up the fact that that Windows has symlinks more than five years ago. At the time, creating them required admin privileges. Soon afterwards, the ability for non-admin users in developer mode to create symlinks was implemented. In the Perl world, Bayan Maxim wrote Win32::NTFS::Symlink.

Fast forward to me playing with Perl 5.34 built on Windows with MSVC recently. I wanted to install pwhich so I ran cpanm App::pwhich. After a while, the installation failed deep inside the chain of dependencies. I changed in to my .cpanm directory and noticed:

2021-09-13 10:39 AM <SYMLINK> build.log [C:\Users\user\.cpanm\work\$time.4012\build.log]

2021-09-13 10:39 AM <SYMLINK> latest-build [C:\Users\user/.cpanm/work/$time.4012]

Oh, cute … Let’s use those symlinks:

C:\Users\user\.cpanm> cd latest-build
The directory name is invalid.

and

C:\Users\user\.cpanm> more build.log
<<< output snipped >>>

Why does one symlink work and not the other? Simple: Internal APIs in DOS (I think since v.3) and Windows really do not care whether directories are separated using \ or /, but mixing the two styles within a single path and passing that to an external program is not good. In the above listing, you will notice that the symlink for build.log is using a canonicalized target whereas the symlink for latest-build is appending a Unix style subdirectory to a Windows style parent directory path.

As the synopsis for Module::CoreList shows, File::Spec was added to Perl core with version 5.005 in 1998. I’d say 23 years is enough to consistently handle file paths. In cpanm, the code for creating these symlinks looks like this:

if (CAN_SYMLINK){
    my $build_link = "$self->{home}/latest-build";
    unlink $build_link;
    symlink $self->{base}, $build_link;
    unlink $final_log;
    symlink $self->{log}, $final_log;
}

The difference between the two links is simple to see:

$self->{base} = "$self->{home}/work/" . time . ".$$";

# versus

$self->{log} = File::Spec->catfile($self->{base},"build.log")

What we see here is the fact that the developer embedded their then current knowledge of the world (“Perl on Windows does not support symlinks”) as a permanent assumption in the code. Manipulating all paths using File::Spec would not have been significantly more complicated at the time, but would have avoided the maintenance burden when the state of the world changed (and change it will).

This is not the most interesting thing. This bug is a mere inconvenience, one that doesn’t matter that much.

What caused cpanm App::pwhich to fail?

It turns out running the tests for App::pwhich requires Test2::V0. And, that depends on Module::Pluggable. The tests for Module::Pluggable failed. Module::Pluggable is in the Perl core.

Why did it fail?

#   Failed test at t\02alsoworks.t line 13.

#   Failed test 'is deeply'
#   at t\02alsoworks.t line 17.
#     Structures begin differing at:
#          $got->[0] = Does not exist
#     $expected->[0] = 'MyOtherTest::Plugin::Bar'
# etc etc etc

A tedious study of the module’s code and tests did not reveal any immediate hints. Then I tried single stepping, but that proved to be rather boring. So I resorted to printf debugging. … OMFG! Look at that path!

C:/Users/user/.cpanm/work/$time.1592/Module-Pluggable-5.2/C:\Users\user\.cpanm\work\$time.1592\Module-Pluggable-5.2\t\lib\MyOtherTest\Plugin

That’s when I went trawling for anything symlink related in Module::Pluggable. It wasn’t hard to find:

$self->{'follow_symlinks'} = 1 unless exists $self->{'follow_symlinks'};

So, if the thing that is instantiating the Module::Pluggable does not specify a value, Module::Pluggable defaults to setting the follow_symlink option when invoking find:

File::Find::find( { no_chdir => 1,
                    follow   => $self->{'follow_symlinks'},
                    wanted   => sub {
                     # Inlined from File::Find::Rule C< name => '*.pm' >
                     return unless $File::Find::name =~ /$file_regex/;
                     (my $path = $File::Find::name) =~ s#^\\./##;
                     push @files, $path;
                   }
              }, $search_path );

There is nothing fundamentally wrong with that. After all, find should know what to do.

Except … File::Find is as legacy as it gets with a whole bunch of assumptions from decades ago permeating every line. In the olden days, passing follow => 1 to find did not even matter: It categorically did not follow symlinks on Windows. What changed?

This commit titled File::Find support Win32 symlinks where the code change consists of replacing:

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

with

$full_check        = $wanted->{follow};
$follow            = $full_check || $wanted->{follow_fast};

and expecting everything will just work out. I can only call this “implementation by wishful thinking.”

The only substantial code change is in taint.t which consists of adding:

BEGIN {
    require File::Spec;
    if ($ENV{PERL_CORE}) {
        # May be doing dynamic loading while @INC is all relative
        @INC = map { $_ = File::Spec->rel2abs($_); /(.*)/; $1 } @INC;
    }

    if ($^O eq 'MSWin32' || $^O eq 'cygwin' || $^O eq 'VMS') {
        # This is a hack - at present File::Find does not produce native names
        # on Win32 or VMS, so force File::Spec to use Unix names.
        # must be set *before* importing File::Find
        require File::Spec::Unix;
        @File::Spec::ISA = 'File::Spec::Unix';
    }
    require File::Find;
    import File::Find;
}

So, tests fail with the new change in place and the instinct is to modify the tests to pass (treat this as a false positive) instead of finding the problem in the code under test and fixing the code. In case it is not obvious, this code is telling File::Spec to assume it is running on a Unix flavor disregarding the actual platform.

Allowing follow to be set causes code paths that used to be skipped on Windows to now be executed. That’s how we end up with a path like C:/Users/user/.cpanm/work/$time.1592/Module-Pluggable-5.2/C:\Users\user\.cpanm\work\$time.1592\Module-Pluggable-5.2\t\lib\MyOtherTest\Plugin.

File::Find, despite the fact that it loads File::Spec, relies on hard-coded / characters. That’s how we end up executing line 206:

$abs_dir = contract_name("$cwd/",$top_item);

and since contract_name looks like this:

sub contract_name {
    my ($cdir,$fn) = @_;

    return substr($cdir,0,rindex($cdir,'/')) if $fn eq $File::Find::current_dir;

    $cdir = substr($cdir,0,rindex($cdir,'/')+1);

    $fn =~ s|^\./||;

    my $abs_name= $cdir . $fn;

    if (substr($fn,0,3) eq '../') {
       1 while $abs_name =~ s!/[^/]*/\.\./+!/!;
    }

    return $abs_name;
}

we end up with the weird path. Of course, that path does not exist and Module::Pluggable fails to build & install.

Note:

#!/usr/bin/env perl

use v5.34;
use strict;
use warnings;

use File::Spec ();

my $up = 'C:/Users/user/.cpanm/work/$time.1592/Module-Pluggable-5.2/';
my $wp = 'C:\\Users\\user\\.cpanm\\work\\$time.1592\\Module-Pluggable-5.2\\t\\lib\\MyOtherTest\\Plugin';

say File::Spec->catfile($up, File::Spec->abs2rel($wp,$up));

Running this code correctly produces:

C:\Users\user\.cpanm\work\$time.1592\Module-Pluggable-5.2\t\lib\MyOtherTest\Plugin

It is unclear to me why File::Find after decades avoids another core library File::Spec.

What are the next steps?

It is reasonable for Module::Pluggable to default to supporting symlinks unless overridden except that it seems to have been written with the assumption that the calling module knows what works for the user. It cannot. So, Module::Pluggable should allow the functionality to be overridden using an environment variable. Something like PERL5_MODULE_PLUGGABLE_FOLLOW_SYMLINKS. After all, this module is used to locate other code to load and organizations may justfiable want to disallow the functionality instead of having to audit every library/module/script that uses Module::Pluggable.
It is reasonable for Test::V0 to use features provided through Module::Pluggable and probably appropriate for it to defer to Module::Pluggable for low level settings. Once again, however, since test suites might be running in interesting environments, it might reasonable to provide a mechanism for an organization to categorically override the ability to follow symlinks for loading code to be executed during the test run. I would recomment something like PERL5_TEST2_SUITE_FOLLOW_SYMLINKS.
It is not reasonable for a module that is as fundamental as File::Find to continue to pretend that the only computing environments in the world are just variations on a single flavor of Unix.
Above all, injecting fundamental changes in behavior in core modules with nary a new test in sight is not consistent with stability and backwards compatibility as priorities.

PS: I am no longer on Reddit, but you can discuss this post on r/perl

Implementation by Wishful Thinking

A. Sinan Unur

September 15, 2021

What are the next steps?