-
-
Notifications
You must be signed in to change notification settings - Fork 229
fix: Fix path mismatches on Windows #2414
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
Conversation
// TODO: Enable the following test and make it pass | ||
// Linux allows having `\` in a file name but our path helpers, | ||
// specifically `join_path` and `clean_path` from `symbolic::common`, | ||
// do not use `std::path::MAIN_SEPARATOR` and instead tries to guess | ||
// the path style by the existence of a `\` in the path. This is | ||
// problematic because it can lead to incorrect path normalization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to explain this a bit: the functions in symbolic
work this way because they are not necessarily intended to be used on paths existing on the machine they're running on. Rather, they are used for paths stored in debug files and the like, which we want to combine and canonicalize. For this purpose they should work independently of the host system. For example, a path stored in a Windows debug file should be transformed and displayed the same regardless of whether you're using symbolic
on Windows or Unix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think we could also state that backslashes in Unix paths are not supported by sentry-cli
and leave it at that, it should be extremely uncommon for backslashes to be present anyway.
|
||
assert_eq!( | ||
normalize_sourcemap_url( | ||
&format!("foo{0}bar{0}baz.js", std::path::MAIN_SEPARATOR), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about this syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too. This is my first working Rust patch 😂
Attempts to fix #2413