[hooks] Handle absolute paths in UserDefines.path() - #3526
Conversation
UserDefines.path() passed every value through basePath.resolve(). A Windows drive letter is a valid URI scheme, so an absolute Windows path was parsed as an already-absolute URI and toFilePath() on the result throws. Absolute paths (File(path).isAbsolute) are now converted with Uri.file without resolution; relative paths keep the existing behavior. Fixes dart-lang#3518
| expect(uri, Uri.parse('file://server/share/lib.dll')); | ||
| expect(uri.toFilePath(), r'\\server\share\lib.dll'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
We should probably also test that relative path with windows separators works? Or maybe it shouldn't work and we should document on the path methods that relative paths should use linux path separators.
There was a problem hiding this comment.
Thanks for the review!
It works. Dart's Uri parser treats backslashes as path separators, so some\dir\model.bin resolves against the base path as expected. Added a test.
| Uri.parse('file:///home/user/proj/example/some/dir/model.bin'), | ||
| ); | ||
| expect(uri.toFilePath(), '/home/user/proj/example/some/dir/model.bin'); | ||
| }); |
There was a problem hiding this comment.
We should probably also test what happens with relative paths with Windows path separators.
If we don't support it, then we should document this clearly on on the path method.
(What happens with File.isAbsolute and Uri.resolve if you pass in Windows relative paths on linux? If it does the right thing we could support it. If. Not we should clearly document that on the path method.)
There was a problem hiding this comment.
It does the right thing on Linux too: File.isAbsolute is false there, and the Uri parser normalizes backslashes to path separators independently of the platform, so it resolves the same as with forward slashes. Added a test as well.
Requested in review: Dart's Uri parser treats backslashes as path separators on all platforms, so relative paths with Windows separators resolve against basePath the same way forward-slash paths do. Adds the case to both the Windows and POSIX test groups.
|
@liamappelbe any chance the zizmor changes are preventing running the health check commenting? |
Definitely possible. I'm not a workflow expert. I probably removed an important permission. I'm about to head out for the weekend, so if you want it fixed before Monday you might need to take a look at it. |
3 participants
Description
UserDefines.path()passed every value throughbasePath.resolve(). A Windows drive letter is a valid URI scheme, so an absolute Windows path was parsed as an already-absolute URI:D:\a\b.dllbecame a URI with schemed, andtoFilePath()on the result throws.path()now returns absolute paths as-is and keeps the existingresolve()behavior for relative paths. Absolute paths are detected withFile(path).isAbsolute, which uses the platform's native path semantics, so no OS-specific branching is needed. The doc comment states this contract.Tests cover the two separator cases raised in the issue, both on Windows: backslashes with an absolute path, and forward slashes with a relative path. They also cover UNC paths, POSIX paths, and the
nullcases.Related Issues
Fixes #3518
PR Checklist
dart tool/ci.dart --alllocally and resolved all issues identified. This ensures the PR is formatted, has no lint errors, and ran all code generators. This applies to the packages part of the toplevelpubspec.yamlworkspace.CHANGELOG.mdfor the relevant packages. (Not needed for small changes such as doc typos).