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

Segments that are just a drive letter are dropped on Windows #37

Open
DanTup opened this issue Apr 18, 2018 · 7 comments
Open

Segments that are just a drive letter are dropped on Windows #37

DanTup opened this issue Apr 18, 2018 · 7 comments
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@DanTup
Copy link
Contributor

DanTup commented Apr 18, 2018

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.

@nex3
Copy link
Member

nex3 commented Apr 24, 2018

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 nex3 added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Apr 24, 2018
@DanTup
Copy link
Contributor Author

DanTup commented Apr 24, 2018

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).

@munificent
Copy link
Member

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.

@DanTup
Copy link
Contributor Author

DanTup commented Apr 24, 2018

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! 😀

@nex3
Copy link
Member

nex3 commented Apr 24, 2018

@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.

@simonradev
Copy link

Hello, do you consider fixing this issue?

@simonradev
Copy link

Hello, do you consider fixing this issue?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants