Use "/" as path separator inside XDVDFSFilesystem #74
Conversation
… when creating default output path
There was a problem hiding this comment.
Thanks for the patch! Just have a couple comments on it, but the general approach seems good to me.
A refactor is probably in order to make the read/write interfaces use the same path interface at some point, but std::path::Path is useful for write due to native filesystems and it doesn't work for read because of no_std, so I think your fix is sufficient for now.
xdvdfs-cli/src/cmd_pack.rs
Outdated
| let source_file_name = source_path.file_name()?; | ||
| let output = PathBuf::from(source_file_name).with_extension("iso"); | ||
| fn get_default_image_path(source_path: &Path) -> PathBuf { | ||
| let output = PathBuf::from(source_path).with_extension("iso"); |
There was a problem hiding this comment.
I'd like to preserve the behavior that the default output path is relative to the current working directory. I have a candidate fix for this (just haven't gotten around to testing on Windows yet), so you can leave this part to me.
There was a problem hiding this comment.
Sure, I'll revert these for the time being. Nonetheless, if the desired behavior was to keep the work dir reference for the creation of the output, perhaps replacing if output.exists() && output == source_path with if output.exists() || output.same_file(source_path) would suffice here.
xdvdfs-core/src/write/img.rs
Outdated
| @@ -148,7 +149,7 @@ pub async fn create_xdvdfs_image<H: BlockDeviceWrite<E>, E>( | |||
| .await?; | |||
|
|
|||
| for entry in dirtab.file_listing { | |||
| let file_path = path.join(&entry.name); | |||
| let file_path = path.join_xdvdfs(&entry.name); | |||
There was a problem hiding this comment.
This may introduce issues when packing with a native filesystem source on platforms where paths don't support / separators or where string conversion is lossy (and won't raise any errors if the string conversion fails).
Since the underlying issue is specific to the xdvdfs Filesystem implementation, I think the path should be processed in copy_file_in and copy_file_buf instead when the path is initially converted to a string. Then, forcing / separators is an implementation detail of the XDVDFSFilesystem instead of the generic write routines.
There was a problem hiding this comment.
I'm thinking of three possible ways of doing this:
- The way you've suggested, by changing the signature of
copy_file_inandcopy_file_buf, allowing to join the path there. - By replacing the
\character with/at thecopy_file_inmethod fromXDVDFSFilesystem. - Using conditional compilation to call
join_xdvdfshere.
The first option would probably be the best way around this, but the FS module would lose some of its generic-ness, which seems to go against your initial design idea.
The second one would take advantage of the fact that filenames shouldn't contain / or \ characters within XDVDFS archives, as is with FATX volumes. But I'm not sure xdvdfs as a project is enforcing this rule, as Microsoft tools did.
And the last one is mostly a hacky way around it, by having Windows targets use the right thing at the right time.
Sorry for the long comment. Please let me know what you think. @antangelo
There was a problem hiding this comment.
I had option 2 in mind. I don't believe / or \ are valid characters in the filename so that tradeoff is fine (and if they were valid, the read functions wouldn't be able to handle them as they are now anyway).
|
Code changes look good to me, just need to run |
|
Thanks, lint and formatting changes have been applied now. |
|
Thanks! |
Closes #73. Also uses full path, when available, to create the default output path.
All modules, but desktop, tested working on Windows 11.
I don't have much experience with Rust, so the code is not ideal, so this is mostly a draft PR to base upon.