Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Perl: porters

MM_Win32.t failures (caused by PathTools upgrade)

 

 

Perl porters RSS feed   Index | Next | Previous | View Threaded


SteveHay at planit

Jan 31, 2008, 3:19 AM

Post #1 of 11 (573 views)
Permalink
MM_Win32.t failures (caused by PathTools upgrade)

ExtUtils/t/MM_Win32.t has been failing two tests lately:

not ok 9 - catdir()
# Failed test 'catdir()'
# at ..\lib\ExtUtils\t\MM_Win32.t line 96.
# got: 'C:trick\dir\now_OK'
# expected: 'C:\trick\dir\now_OK'

not ok 11 - catfile()
# Failed test 'catfile()'
# at ..\lib\ExtUtils\t\MM_Win32.t line 105.
# got: 'C:trick\dir\now_OK\file.ext'
# expected: 'C:\trick\dir\now_OK\file.ext'

These are caused by the upgrade to PathTools-3.27 in #33042, which
brought in a major rewrite of Win32's catdir, catfile and canonpath from
PathTools-3.25_01.

In short, this program:

use File::Spec::Functions qw(catdir);
my @path_eg = qw( c: trick dir/now_OK );
print catdir(@path_eg);

used to output:

C:\trick\dir\now_OK

but now outputs:

C:trick\dir\now_OK

Is this new behaviour intended and correct (in which case
ExtUtils/t/MM_Win32.t needs a simple tweak to fix), or is PathTools
broken and needs fixing itself instead?

Either output is arguably correct (the notation "C:path" means the file
or directory called "path" relative to the current directory on the "C:"
drive, there being the notion of a current directory per drive), and the
File::Spec docs don't make it clear which output is to be expected.

I can imagine that most people would expect catdir(qw(C: trick)) to
produce "C:\trick". It currently doesn't, and it isn't clear what you
have to do to get "C:\trick": it turns out that this works:

my @path_eg = qw( c:/ trick dir/now_OK );
print catdir(@path_eg);

but this doesn't:

my @path_eg = qw( c: /trick dir/now_OK );
print catdir(@path_eg);

and neither does this:

my @path_eg = qw( c: / trick dir/now_OK );
print catdir(@path_eg);

which is all very confusing.

Given all that, I think I'd personally prefer not to have a change in
behaviour and fix PathTools so that the example above (and MM_Win32.t)
works as before. (That could be easier said than done, of course.)


schwern at pobox

Jan 31, 2008, 9:09 AM

Post #2 of 11 (540 views)
Permalink
Re: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

Steve Hay wrote:
> ExtUtils/t/MM_Win32.t has been failing two tests lately:
>
> not ok 9 - catdir()
> # Failed test 'catdir()'
> # at ..\lib\ExtUtils\t\MM_Win32.t line 96.
> # got: 'C:trick\dir\now_OK'
> # expected: 'C:\trick\dir\now_OK'
>
> not ok 11 - catfile()
> # Failed test 'catfile()'
> # at ..\lib\ExtUtils\t\MM_Win32.t line 105.
> # got: 'C:trick\dir\now_OK\file.ext'
> # expected: 'C:\trick\dir\now_OK\file.ext'
>
> These are caused by the upgrade to PathTools-3.27 in #33042, which
> brought in a major rewrite of Win32's catdir, catfile and canonpath from
> PathTools-3.25_01.
>
> In short, this program:
>
> use File::Spec::Functions qw(catdir);
> my @path_eg = qw( c: trick dir/now_OK );
> print catdir(@path_eg);
>
> used to output:
>
> C:\trick\dir\now_OK
>
> but now outputs:
>
> C:trick\dir\now_OK
>
> Is this new behaviour intended and correct (in which case
> ExtUtils/t/MM_Win32.t needs a simple tweak to fix), or is PathTools
> broken and needs fixing itself instead?

MakeMaker doesn't really care what the output is, as long as it's consistent.
The new behavior seems correct. I'd have to go back and see what issue that
test is actually testing.

MakeMaker runs catfile() through canonpath() to work around some old bugs.
It's possible that canonpath() is the one translating C:trick\dir\now_OK into
C:\trick\dir\now_OK. If so, that's a bug.

Can you check what File::Spec->canonpath("C:trick\dir\now_OK") does?


> Either output is arguably correct (the notation "C:path" means the file
> or directory called "path" relative to the current directory on the "C:"
> drive, there being the notion of a current directory per drive), and the
> File::Spec docs don't make it clear which output is to be expected.
>
> I can imagine that most people would expect catdir(qw(C: trick)) to
> produce "C:\trick". It currently doesn't, and it isn't clear what you
> have to do to get "C:\trick":

Probably use a blank directory to represent root. That's what Unix does. Try:

catdir( 'c:', '', 'trick', 'dir' );

Actually the correct answer is "none of the above" because catdir() uses
directories, not volumes. In reality the correct thing to use is catpath().


> and neither does this:
>
> my @path_eg = qw( c: / trick dir/now_OK );
> print catdir(@path_eg);

That doesn't work? Now that is odd. Have you tried using backwhacks?

When in doubt, run it through splitpath() and splitdir().


--
...they shared one last kiss that left a bitter yet sweet taste in her
mouth--kind of like throwing up after eating a junior mint.
-- Dishonorable Mention, 2005 Bulwer-Lytton Fiction Contest
by Tami Farmer


SteveHay at planit

Jan 31, 2008, 9:30 AM

Post #3 of 11 (540 views)
Permalink
RE: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

Michael G Schwern wrote:
> Steve Hay wrote:
>> In short, this program:
>>
>> use File::Spec::Functions qw(catdir);
>> my @path_eg = qw( c: trick dir/now_OK );
>> print catdir(@path_eg);
>>
>> used to output:
>>
>> C:\trick\dir\now_OK
>>
>> but now outputs:
>>
>> C:trick\dir\now_OK
>>
>
> MakeMaker runs catfile() through canonpath() to work around some old
> bugs. It's possible that canonpath() is the one translating
> C:trick\dir\now_OK into C:\trick\dir\now_OK. If so, that's a bug.
>
> Can you check what File::Spec->canonpath("C:trick\dir\now_OK") does?

Ick. You didn't mean that: you meant check what
File::Spec->canonpath('C:trick\dir\now_OK') does, and the answer is
nothing:

C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
File::Spec->canonpath('C:trick\dir\now_OK')"
C:trick\dir\now_OK


>
>
>> Either output is arguably correct (the notation "C:path" means the
>> file or directory called "path" relative to the current directory on
>> the "C:" drive, there being the notion of a current directory per
>> drive), and the File::Spec docs don't make it clear which output is
>> to be expected.
>>
>> I can imagine that most people would expect catdir(qw(C: trick)) to
>> produce "C:\trick". It currently doesn't, and it isn't clear what you
>> have to do to get "C:\trick":
>
> Probably use a blank directory to represent root. That's what Unix
> does. Try:
>
> catdir( 'c:', '', 'trick', 'dir' );

No good either:

C:\p5p\bleadperl>.\perl -MFile::Spec -e "print File::Spec->catdir('C:',
'', 'trick', 'dir')"
C:trick\dir


>
> Actually the correct answer is "none of the above" because catdir()
> uses directories, not volumes. In reality the correct thing to use
> is catpath().

It's still confusing:

C:\p5p\bleadperl>.\perl -MFile::Spec -e "print File::Spec->catpath('C:',
'trick', 'file')"
C:trick\file

C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
File::Spec->catpath('C:\\', 'trick', 'file')"
C:\trick\file

In other words catpath() expects the $volume to be something like C:\
rather than just C:, which is at odds with splitpath() which returns the
$volume as just C:

C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
+(File::Spec->splitpath('C:\trick\file'))[0]"
C:

(This catpath()/splitpath() confusion is not new in PathTools-3.25_01,
though.)


>
>
>> and neither does this:
>>
>> my @path_eg = qw( c: / trick dir/now_OK );
>> print catdir(@path_eg);
>
> That doesn't work? Now that is odd. Have you tried using backwhacks?

That definitely doesn't work with either forward- or back-slashes:

C:\p5p\bleadperl>.\perl -MFile::Spec -e "print File::Spec->catdir(qw(C:
/ trick dir/now_OK))
C:trick\dir\now_OK

C:\p5p\bleadperl>.\perl -MFile::Spec -e "print File::Spec->catdir(qw(C:
\ trick dir\now_OK))
C:trick\dir\now_OK


schwern at pobox

Jan 31, 2008, 10:57 AM

Post #4 of 11 (536 views)
Permalink
Re: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

Steve Hay wrote:
>> Can you check what File::Spec->canonpath("C:trick\dir\now_OK") does?
>
> Ick. You didn't mean that: you meant check what
> File::Spec->canonpath('C:trick\dir\now_OK') does, and the answer is
> nothing:
>
> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
> File::Spec->canonpath('C:trick\dir\now_OK')"
> C:trick\dir\now_OK

Oh, I was thinking wrong. MakeMaker is doing the right thing, meaning it's
just doing what File::Spec does. MakeMaker can be removed from consideration
as part of the bug. MakeMaker's test is wrong and should be using catpath().


>> Probably use a blank directory to represent root. That's what Unix
>> does. Try:
>>
>> catdir( 'c:', '', 'trick', 'dir' );
>
> No good either:
>
> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print File::Spec->catdir('C:',
> '', 'trick', 'dir')"
> C:trick\dir

Thinking more about this, it probably shouldn't work given that catdir() is
treating C: as a directory so you're asking for a root directory in the middle
of the path which doesn't make any sense.

catpath() is the correct thing to use.


>> Actually the correct answer is "none of the above" because catdir()
>> uses directories, not volumes. In reality the correct thing to use
>> is catpath().
>
> It's still confusing:
>
> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print File::Spec->catpath('C:',
> 'trick', 'file')"
> C:trick\file
>
> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
> File::Spec->catpath('C:\\', 'trick', 'file')"
> C:\trick\file
>
> In other words catpath() expects the $volume to be something like C:\
> rather than just C:, which is at odds with splitpath() which returns the
> $volume as just C:
>
> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
> +(File::Spec->splitpath('C:\trick\file'))[0]"
> C:

This is all correct. splitpath() returns ($volume, $directories, $file) so
the return value should be:

("C:", "\trick\", "file")

It's arguable whether the volume name should be "C" or "C:". Depends on
whether or not : is considered part of the volume name or just the volume
separator.

Try this:

print join "\n", File::Spec->splitpath('C:\trick\file');
print File::Spec->catpath("C:", File::Spec->catdir("", "trick"), "file");

It should say:
C:
\trick\
file
C:\trick\file

That invocation of catpath/catdir is how you're supposed to do it. I will
admit that getting File::Spec to work correctly with volumes and root dir is
confusing. Path::Class is supposed to make this easier, but it doesn't appear
to have any method for getting the volume at all.


>>> my @path_eg = qw( c: / trick dir/now_OK );
>>> print catdir(@path_eg);
>> That doesn't work? Now that is odd. Have you tried using backwhacks?
>
> That definitely doesn't work with either forward- or back-slashes:
>
> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print File::Spec->catdir(qw(C:
> / trick dir/now_OK))
> C:trick\dir\now_OK
>
> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print File::Spec->catdir(qw(C:
> \ trick dir\now_OK))
> C:trick\dir\now_OK

Ok, that all makes sense now once you realize that catdir() does not
understand the concept of a volume. If you think about it in terms of Unix
you have just asked for:

catdir(qw(a / b c));

which is like a//b/c and it just threw out the duplicate slash.


--
Ahh email, my old friend. Do you know that revenge is a dish that is best
served cold? And it is very cold on the Internet!


SteveHay at planit

Feb 1, 2008, 4:58 AM

Post #5 of 11 (535 views)
Permalink
RE: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

Michael G Schwern wrote:
> Steve Hay wrote:
>>> Can you check what File::Spec->canonpath("C:trick\dir\now_OK") does?
>>
>> Ick. You didn't mean that: you meant check what
>> File::Spec->canonpath('C:trick\dir\now_OK') does, and the answer is
>> nothing:
>>
>> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
>> File::Spec->canonpath('C:trick\dir\now_OK')"
>> C:trick\dir\now_OK
>
> Oh, I was thinking wrong. MakeMaker is doing the right thing,
> meaning it's just doing what File::Spec does. MakeMaker can be
> removed from consideration as part of the bug. MakeMaker's test is
> wrong and should be using catpath().

OK. Someone needs to change the test script, then. I'll look when I find
time if no-one else has done by then.


>
>
>>> Probably use a blank directory to represent root. That's what Unix
>>> does. Try:
>>>
>>> catdir( 'c:', '', 'trick', 'dir' );
>>
>> No good either:
>>
>> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
>> File::Spec->catdir('C:', '', 'trick', 'dir')" C:trick\dir
>
> Thinking more about this, it probably shouldn't work given that
> catdir() is treating C: as a directory so you're asking for a root
> directory in the middle of the path which doesn't make any sense.
>
> catpath() is the correct thing to use.

OK.


>
>
>>> Actually the correct answer is "none of the above" because catdir()
>>> uses directories, not volumes. In reality the correct thing to use
>>> is catpath().
>>
>> It's still confusing:
>>
>> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
>> File::Spec->catpath('C:', 'trick', 'file')" C:trick\file
>>
>> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
>> File::Spec->catpath('C:\\', 'trick', 'file')"
>> C:\trick\file
> >
>> In other words catpath() expects the $volume to be something like C:\
>> rather than just C:, which is at odds with splitpath() which returns
>> the $volume as just C:
>>
>> C:\p5p\bleadperl>.\perl -MFile::Spec -e "print
>> +(File::Spec->splitpath('C:\trick\file'))[0]"
>> C:
>
> This is all correct. splitpath() returns ($volume, $directories,
> $file) so the return value should be:
>
> ("C:", "\trick\", "file")

Ah, right. So going back to the original examples, we actually want
something like this:

my @path_eg = qw( c: /trick/ dir/now_OK );
print catpath(@path_eg);


>
> It's arguable whether the volume name should be "C" or "C:". Depends
> on whether or not : is considered part of the volume name or just the
> volume separator.
>
> Try this:
>
> print join "\n", File::Spec->splitpath('C:\trick\file');
> print File::Spec->catpath("C:", File::Spec->catdir("", "trick"),
> "file");
>
> It should say:
> C:
> \trick\
> file
> C:\trick\file

Yes (except that you missed a newline between your print()s).

Thanks for the explanations.


kenahoo at gmail

Feb 1, 2008, 5:08 AM

Post #6 of 11 (537 views)
Permalink
Re: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

On Jan 31, 2008 11:09 AM, Michael G Schwern <schwern [at] pobox> wrote:
>
> MakeMaker runs catfile() through canonpath() to work around some old bugs.
> It's possible that canonpath() is the one translating C:trick\dir\now_OK into
> C:\trick\dir\now_OK. If so, that's a bug.
>
> Can you check what File::Spec->canonpath("C:trick\dir\now_OK") does?

That has to be left alone, since it's a valid [relative] path.

> > I can imagine that most people would expect catdir(qw(C: trick)) to
> > produce "C:\trick". It currently doesn't, and it isn't clear what you
> > have to do to get "C:\trick":
>
> Probably use a blank directory to represent root. That's what Unix does. Try:
>
> catdir( 'c:', '', 'trick', 'dir' );

That won't work because it means a directory called 'c:' containing a
directory called '' (which for some reason gets collapsed away)
containing etc. etc.

Arguably it should just die() on those arguments, because AFAIK 'c:'
is an invalid directory name.


>
> Actually the correct answer is "none of the above" because catdir() uses
> directories, not volumes. In reality the correct thing to use is catpath().

Yes, that's right.

File::Spec is just hard to use correctly.

-Ken


jand at activestate

Feb 1, 2008, 1:00 PM

Post #7 of 11 (538 views)
Permalink
RE: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

On Fri, 01 Feb 2008, Ken Williams wrote:
> Arguably it should just die() on those arguments, because AFAIK 'c:'
> is an invalid directory name.

I don't think so. 'c:' is just a short form of 'c:.\\'.

Cheers,
-Jan


schwern at pobox

Feb 1, 2008, 4:18 PM

Post #8 of 11 (539 views)
Permalink
Re: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

Steve Hay wrote:
>> Oh, I was thinking wrong. MakeMaker is doing the right thing,
>> meaning it's just doing what File::Spec does. MakeMaker can be
>> removed from consideration as part of the bug. MakeMaker's test is
>> wrong and should be using catpath().
>
> OK. Someone needs to change the test script, then. I'll look when I find
> time if no-one else has done by then.

It's ok, I'll get it.


>> print join "\n", File::Spec->splitpath('C:\trick\file');
>> print File::Spec->catpath("C:", File::Spec->catdir("", "trick"),
>> "file");
>>
>> It should say:
>> C:
>> \trick\
>> file
>> C:\trick\file
>
> Yes (except that you missed a newline between your print()s).

Sorry, I tend to use -l


--
THIS I COMMAND!


kenahoo at gmail

Feb 1, 2008, 4:25 PM

Post #9 of 11 (540 views)
Permalink
Re: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

On Jan 31, 2008 12:57 PM, Michael G Schwern <schwern [at] pobox> wrote:
> That invocation of catpath/catdir is how you're supposed to do it. I will
> admit that getting File::Spec to work correctly with volumes and root dir is
> confusing. Path::Class is supposed to make this easier, but it doesn't appear
> to have any method for getting the volume at all.

$file->volume and $dir->volume both exist and are documented.

-Ken


schwern at pobox

Feb 1, 2008, 9:17 PM

Post #10 of 11 (538 views)
Permalink
Re: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

Ken Williams wrote:
> On Jan 31, 2008 12:57 PM, Michael G Schwern <schwern [at] pobox> wrote:
>> That invocation of catpath/catdir is how you're supposed to do it. I will
>> admit that getting File::Spec to work correctly with volumes and root dir is
>> confusing. Path::Class is supposed to make this easier, but it doesn't appear
>> to have any method for getting the volume at all.
>
> $file->volume and $dir->volume both exist and are documented.

I don't see them in Path::Class 0.16.

Oh, I see, it's in Path::Class::File.


--
I do have a cause though. It's obscenity. I'm for it.
- Tom Lehrer


kenahoo at gmail

Feb 2, 2008, 7:06 AM

Post #11 of 11 (539 views)
Permalink
Re: MM_Win32.t failures (caused by PathTools upgrade) [In reply to]

On Feb 1, 2008 11:17 PM, Michael G Schwern <schwern [at] pobox> wrote:
>
> Ken Williams wrote:
> > $file->volume and $dir->volume both exist and are documented.
>
> I don't see them in Path::Class 0.16.
>
> Oh, I see, it's in Path::Class::File.

Yeah, and Path::Class::Dir.

-Ken

Perl porters RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.