path icon indicating copy to clipboard operation
path copied to clipboard

Segments that are just a drive letter are dropped on Windows

Open DanTup opened this issue 6 years ago • 7 comments

On Windows, the following code:

final String homeDrive = platform.environment['HOMEDRIVE'];
final String homePath = platform.environment['HOMEPATH'];

print(homeDrive);
print(homePath);
print(fs.path.join(homeDrive, homePath));

Outputs the following:

C:
\Users\danny
\Users\danny

The value of HOMEDRIVE is something that's provided by Windows (without a trailing slash) and is often joined with HOMEPATH so maybe it makes sense to support it here.

DanTup avatar Apr 18 '18 15:04 DanTup

Windows paths like C: are a weird case that this package currently doesn't attempt to handle at all, and it's not clear what the correct behavior should be. According to this MSDN article, a path of the form C:foo\bar refers to foo\bar relative to "the current directory on drive C". That's all well and good if the current drive is C, but what does it mean when the current drive is D:?

According to this Raymond Chen article, cmd.exe uses environment variables to track a notion of the "current directory" for each drive. We have to figure out a policy for interacting with these separate drive directories in order to provide a correct view of paths on a Windows system.

I see three possibilities:

  1. Shell out to cmd to check the current directory of a drive if doing so ever becomes necessary. This would require accessing dart:io from path, which we've avoided so far, but which is now feasible due to dart:io being available anywhere. p.absolute("D:foo") would produce "D:\whatever\dir\foo".

  2. Treat foreign drives' working directories as unknowable in the same way we treat the main working directory as unknowable for p.windows and friends. p.absolute("D:foo") would produce "D:foo".

  3. Treat foreign drives' working directories as the root directory. This is the default behavior for a cmd.exe that's spawned outside the context of any other command prompt, and it will be correct a high percentage of the time since presumably most users won't be flipping between drives.

I lean towards option 2. I don't like the performance characteristics of option 1 (or the fact that the subprocess spawn could hypothetically flake), and I don't like even the marginal possibility of incorrectness in option 3.

@munificent I'd appreciate your thoughts on this.

nex3 avatar Apr 24 '18 01:04 nex3

I decided to try this out on dotnetfiddle, figuring .NET might be a good thing to copy since it came from MS... However:

Console.WriteLine(Path.Combine("C:", "\\test\\test"));
Console.WriteLine(Path.Combine("C:", "test\\test"));
Console.WriteLine(Path.Combine("X:", "\\test\\test"));
Console.WriteLine(Path.Combine("X:", "test\\test"));

Outputs:

\test\test
C:test\test
\test\test
X:test\test

So actually it's not doing what I'd want anyway. In my original example I had C: and \Users\danny which in .NET would've returned me \Users\danny regardless of my current drive, which would be incorrect if my current drive wasn't C!

So, probably the fix isn't going to be usable with HOMEDRIVE/HOMEPATH directly anyway, so I'll have to do just concat the strings myself (as I am currently).

DanTup avatar Apr 24 '18 06:04 DanTup

I think the right interpretation is that HOMEDRIVE is not a path on its own, it's a string you can use to build a path.

munificent avatar Apr 24 '18 16:04 munificent

Yeah, I think that makes sense. I (incorrectly) assumed they would both be in a nice format to use in path.join's etc., but these things are probably as old as me! 😀

DanTup avatar Apr 24 '18 17:04 DanTup

@munificent In practice, it seems that listing a string like C: as a directory does list the working directory, so I think there's a case to be made for considering it a path. But whether you do or not, C:foo is definitely a path, and we need to decide how to handle it.

nex3 avatar Apr 24 '18 20:04 nex3

Hello, do you consider fixing this issue?

simonradev avatar Oct 10 '20 12:10 simonradev

Hello, do you consider fixing this issue?

Please, disregard. I just saw that it is working as expected in 1.8.0-nullsafety.1.

simonradev avatar Oct 11 '20 18:10 simonradev