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

RFC && 3rd attempt: Fixed resolve of IO::Path for Windows #1249

Merged
merged 1 commit into from Dec 7, 2017

Conversation

ufobat
Copy link
Contributor

@ufobat ufobat commented Nov 13, 2017

Pathes with Windows volumes do no longer start with a seperator character.

> ./perl6 -e '$*SPEC=IO::Spec::Win32; "C:\\foo\\bar".IO.resolve.say'
"C:\foo\bar".IO

@ufobat ufobat force-pushed the path_3rd branch 2 times, most recently from 59d5c08 to b059e66 Compare November 14, 2017 10:11
@ufobat
Copy link
Contributor Author

ufobat commented Nov 14, 2017

  • moved the shift in oder to get $volume out of the while loop.
  • passes following small test case:
use v6;

use Test;

$*SPEC = IO::Spec::Win32;
my $sep = $*SPEC.dir-sep;

my @test-paths = <C:\ C:\Windows \User\martin>;

for @test-paths {
    is $_.IO.resolve.Str, $_, 'resolve of ' ~ $_;
}

my $relative-path = join($sep, 'something', 'relative');

my $current-directory = $*CWD.Str.trans(IO::Spec::Unix.dir-sep => $sep);

is $relative-path.IO.resolve.Str,
   $current-directory ~ $sep ~ $relative-path,
   'relative path ' ~ $relative-path;

done-testing;

@zoffixznet
Copy link
Contributor

What about the CWD attribute of the resultant path? Currently it's being set with this at the end of .resolve method:

    IO::Path!new-from-absolute-path($resolved,:$!SPEC,:CWD($sep));

So it'd end up as just a separator; can't tell off hand if that's going to cause issues down the road, but, say, "foo".IO.perl.say on Windows does have a volume in it.

It's possible the fix can simply be concatenating the $volume we already calculated earlier in the method; just need to ensure it's always set to something and handle the case when it isn't.

@zoffixznet
Copy link
Contributor

zoffixznet commented Nov 20, 2017

Sorry to see you make so many revisions, but I'm seeing three extra problems with this version.

My code suggestions below are mere guesses and were not tested.

1) Only absolute paths get resolved right

C:\rakudo-git>perl6 -e "IO::Path.new('C:\rakudo-git').resolve.say"
"C:\rakudo-git".IO

C:\rakudo-git>perl6 -e "IO::Path.new('.').resolve.say"
"\C:\rakudo-git".IO

This part will only set the volume for absolute paths:

my str $volume    = %.parts<volume>;
my $path          = join $sep, %!parts<dirname  basename>;
$path = $.is-absolute ?? $path !! join $sep, ($.CWD, $path);

For relative paths, it'll fallback to the old problematic method of having the volume in the path. Instead, you should absolutify the path first (also, I now realized my suggestion to just join $sep was wrong; need to use .catpath instead):

my %parts := $!SPEC.split: self.absolute;
my $path  := $!SPEC.catpath: '', |%parts<dirname basename>;

The volume is then accessible in %parts<volume>

2) Paths with volumes that differ from current volume get resolved on the wrong volume. This example shows the file exists when we print the size, but resolving that path fails:

C:\rakudo-git>perl6 -e "with IO::Path.new('X:\bug\bug3.jpg') { .s.say; .resolve( :completely).say }"
46252
Failed to completely resolve IO::Path.new("X:\\bug\\bug3.jpg", :SPEC(IO::Spec::Win32), CWD("C:\\rakudo-git"))
  in block <unit> at -e line 1

The code merely extracts the volume from the path and re-attaches it at the end. I think this can be fixed by moving my str $resolved = $empty; declaration to be after the volume extraction and initializing it with %parts<volume> instead of $empty will fix this issue. Just need to address this part at the end of the method:

    $resolved = $sep unless nqp::chars($resolved);
    $resolved = $volume ~ $resolved;

Untested, but probably just need to remove the second line and change the first one to:

    $resolved = $volume ~ $sep if $resolved eq $volume;

3) Slash after volume gets messed up

C:\rakudo-git>perl6 -e "IO::Path.new('C:\rakudo-git\nqp').resolve.say"
"C:\rakudo-git\nqp".IO

C:\rakudo-git>perl6 -e "IO::Path.new('C:/rakudo-git/nqp').resolve.say"
"C:\/rakudo-git\nqp".IO

The second version added an extraneous slash after the volume. I believe it's because of this line:

$resolved = $volume ~ $resolved;

It needs something more sophisticated, but the solution to (2) above removes it, so I guess that'll solve itself.

@ufobat
Copy link
Contributor Author

ufobat commented Nov 20, 2017

shoudn't we have all this in the spec tests as well?

@zoffixznet
Copy link
Contributor

shoudn't we have all this in the spec tests as well?

Yes, we should :)

Pathes with Windows volumes do no longer start with a seperator character.

IO::Path.resolve: Added $volume to CWD
@ufobat
Copy link
Contributor Author

ufobat commented Nov 20, 2017

what should be the result of "C:foo" with $*SPEC = IO::Spec::Win32 on linux?

I find it strange/wrong that "C:foo".absolute killes the volume. But this would be a bug in .absolute()?

┌─[martin@psyche] - [~/.workspace/p6/rakudo] - [Mo Nov 20, 22:55]
└─[$] <git:(path_3rd*)> ./perl6  -Ilib -e '$*SPEC = IO::Spec::Win32; "c:\\foo".IO.absolute.say; say "c:\\foo".IO.resolve'
C:\foo
"C:\foo".IO
┌─[martin@psyche] - [~/.workspace/p6/rakudo] - [Mo Nov 20, 22:56]
└─[$] <git:(path_3rd*)> ./perl6  -Ilib -e '$*SPEC = IO::Spec::Win32; "c:foo".IO.absolute.say; say "c:foo".IO.resolve'  
\home\martin\.workspace\p6\rakudo\foo
"\home\martin\.workspace\p6\rakudo\foo".IO

@zoffixznet
Copy link
Contributor

zoffixznet commented Nov 20, 2017

I find it strange/wrong that "C:foo".absolute killes the volume. But this would be a bug in .absolute()?

Makes sense to me. It's a relative path and your $.CWD is set to /home/martin/.workspace/p6/rakudo. It absolutifies to the $.CWD and if you set it to have proper volumed path, it gives right result:

$ perl6 -e 'with IO::Path::Win32.new: "c:foo", :CWD<C:/> { .absolute.say; }'                                                                                                                                                       
C:\foo

It has similar behaviour on Windows. Is it a bug? No idea, but the gut feeling is "no, it isn't":

F:\SteamLibrary>perl6 -e "'C:foo'.IO.absolute.say"
F:\SteamLibrary\foo

@ugexe
Copy link
Member

ugexe commented Nov 21, 2017

https://en.wikipedia.org/wiki/Path_(computing)#MS-DOS.2FMicrosoft_Windows_style

C:..\File.txt
This path refers to a file called File.txt located in the parent directory of the current directory on drive C:.


file: uri rfc also has some notes on this (ignore the leading /)
https://tools.ietf.org/html/rfc8089#page-14

A relative reference starting with a drive letter would be
interpreted by a generic URI parser as a URI with the drive letter as
its scheme. Instead, such a reference ought to be constructed with a
leading slash "/" character (e.g., "/c:/foo.txt").

Relative references with a drive letter followed by a character other
than a slash (e.g., "/c:bar/baz.txt" or "/c:../foo.txt") might not be
accepted as dereferenceable URIs in DOS- or Windows-like systems.

@ufobat
Copy link
Contributor Author

ufobat commented Dec 7, 2017

is there something missing or could this be merged?

@zoffixznet zoffixznet merged commit f70e20b into rakudo:master Dec 7, 2017
@zoffixznet
Copy link
Contributor

👍 Thanks!

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.

None yet

3 participants