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.

Sign up for GitHub

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

fix: Windows specific path problem #1649

Closed
wants to merge 1 commit into from
Closed

fix: Windows specific path problem #1649

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 93 additions & 1 deletion components/clarinet-files/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ impl FileLocation {
}

pub fn from_url_string(url_string: &str) -> Result<FileLocation, String> {
// Handle Windows-style paths specially
if url_string.contains('\\') || url_string.contains(':') {
let path = PathBuf::from(url_string);
if path.exists() {
return Ok(FileLocation::FileSystem { path });
}
}
Comment on lines +104 to +110
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's an issue with this code.
from_url_string kind of assume that we are passing a valid url and that we should return a FileLocation:Url.
If not, from_path_string is called.
And Url should never be "windows style"

Is there an issue in how we handle it here?

One other approach could be to have a generic From<String> that just sort of handle it.

impl From<String> for FileLocation {
    fn from(location_string: String) -> Self {
        if let Ok(location) = FileLocation::from_url_string(&location_string) {
            return location;
        }
        if let Ok(location) = FileLocation::from_path_string(&location_string) {
            return location;
        }
        panic!("Invalid location string: {}", location_string);
    }
}

Could be a good opportunity to implement From<PathBuf> and From<Url> and get a bit more idomatic (instead of the custom functions that we have now)

The point is that Path adn PathBuf should be platform agnostic and we shouldn't have to deal with it, looks like it will get even more error prone.

let url = Url::from_str(url_string)
.map_err(|e| format!("unable to parse {} as a url\n{:?}", url_string, e))?;

Expand Down Expand Up @@ -129,15 +136,26 @@ impl FileLocation {
path.extend(&path_to_append);
}
FileLocation::Url { url } => {
// Handle Windows paths first
if url.scheme() == "file" && url.path().starts_with("/C:") {
let new_path = url.path().trim_start_matches('/').to_string();
url.set_path(&new_path);
}
let mut paths_segments = url
.path_segments_mut()
.map_err(|_| "unable to mutate url".to_string())?;

// Clear existing empty segments at the end
paths_segments.pop_if_empty();

for component in path_to_append.components() {
let segment = component
.as_os_str()
.to_str()
.ok_or(format!("unable to format component {:?}", component))?;
paths_segments.push(segment);
if !segment.is_empty() {
paths_segments.push(segment);
}
}
}
}
Expand Down Expand Up @@ -411,3 +429,77 @@ pub fn get_manifest_location(path: Option<String>) -> Option<FileLocation> {
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use std::path::PathBuf;
use url::Url;

#[test]
fn test_windows_url_mutation() {
// Windows file URLs typically start with a drive letter
let windows_url = Url::parse("file:///C:/Users/test/project/")
.expect("Failed to parse Windows-style URL");
let mut location = FileLocation::from_url(windows_url);

let result = location.append_path("subdir\\file.txt");
assert!(result.is_ok());
}
#[test]
fn test_valid_url_mutation() {
// Create a regular URL that should work
let valid_url =
Url::parse("file:///home/user/project/").expect("Failed to parse Unix-style URL");

let mut location = FileLocation::from_url(valid_url);

// This should succeed
let result = location.append_path("subdir/file.txt");
assert!(result.is_ok());

if let FileLocation::Url { url } = location {
assert_eq!(url.path(), "/home/user/project/subdir/file.txt");
} else {
panic!("Expected URL variant");
}
}

#[test]
fn test_windows_drive_letter_parsing() {
// Test parsing a Windows-style file URL
let result = FileLocation::from_url_string("file:///C:/Users/test/project");
assert!(result.is_ok(), "Windows file URL should parse");
}

#[test]
fn test_try_parse_windows_paths() {
// Test relative Windows path
let result = FileLocation::try_parse("folder\\file.txt", None);
assert!(
result.is_none(),
"Relative Windows path without hint should return None"
);

// Test absolute Windows path
let result = FileLocation::try_parse("C:\\Users\\test\\file.txt", None);
assert!(result.is_some(), "Absolute Windows path should parse");
}

#[test]
fn test_append_with_windows_separators() {
let mut location = FileLocation::from_path(PathBuf::from("/base/path"));

// Test appending Windows-style path
let result = location.append_path("subdir\\file.txt");
assert!(result.is_ok(), "Should handle Windows separators");

if let FileLocation::FileSystem { path } = &location {
let path_str = path.to_string_lossy();
assert!(
path_str.contains("subdir") && path_str.contains("file.txt"),
"Path should contain appended components"
);
}
}
}
Loading