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

Use fs-err crate instead of std::fs #56

Closed
marcospb19 opened this issue Oct 1, 2021 · 5 comments · Fixed by #94
Closed

Use fs-err crate instead of std::fs #56

marcospb19 opened this issue Oct 1, 2021 · 5 comments · Fixed by #94
Assignees
Labels
enhancement New feature or request high priority
Milestone

Comments

@marcospb19
Copy link
Member

marcospb19 commented Oct 1, 2021

That will give more context to errors when dealing with filesystem operations.

To replace it:

// Replace this
use std::fs;

// By this
use fs_err as fs;
@marcospb19 marcospb19 added enhancement New feature or request medium priority labels Oct 1, 2021
@marcospb19
Copy link
Member Author

We should however use it carefully, as it changes how our error messages are displayed, the new messages can become redundant and ugly.

So we shall test some errors after this change to see how common errors change.

@GabrielSimonetto
Copy link
Collaborator

GabrielSimonetto commented Oct 9, 2021

We have a problem, external libraries not necessarily will accept our fs type:

Here I have changed fs on src/archive/tar.rs

use std::{
    env,
    // fs,
    io::prelude::*,
    path::{Path, PathBuf},
};

use fs_err as fs;
...
error[E0308]: mismatched types
  --> src/archive/tar.rs:62:43
   |
62 |                 builder.append_file(path, &mut file)?;
   |                                           ^^^^^^^^^ expected struct `std::fs::File`, found struct `fs_err::File`
   |
   = note: expected mutable reference `&mut std::fs::File`
              found mutable reference `&mut fs_err::File`

For more information about this error, try `rustc --explain E0308`.

Which makes sense. Our external dependency tar tries to use specifically the std fs:

pub fn append_file<P: AsRef<Path>>(&mut self, path: P, file: &mut fs::File) -> io::Result<()> {...}

The same thing doesn't seem to be a problem with serde_json, because they are using generic types? idk that seems to be the case:

#[cfg(feature = "std")]
#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
pub fn from_reader<R, T>(rdr: R) -> Result<T>
where
    R: crate::io::Read,
    T: de::DeserializeOwned,
{
    from_trait(read::IoRead::new(rdr))
}

So, we have a few options/questions:
1- Is it possible to solve this on our end?
2- If it's not, should we vendor the tar lib? (probably not)
3- We could make a PR to them (the external tar lib) implementing generic stuff, but that will take some time.
4- If we aren't doing any of these, should we just not use fs_err when interacting with external libraries that have an issue with it?

A last point, do you guys think this is a valid issue to create on the fs_err github? Just asking how to deal with these kinds of issues? Because it seems to me there is nothing to be made, but maybe smarter people have other ideas to solve this on our end without involving the tar lib.

@GabrielSimonetto
Copy link
Collaborator

GabrielSimonetto commented Oct 9, 2021

It has been brought to my attention that there are wrappers on fs-err that deal with this 🍍

https://docs.rs/fs-err/2.6.0/fs_err/struct.File.html#method.file_mut

else {
    let mut file = fs::File::open(path)?;
    builder.append_file(path, &mut file.file_mut())?; // ugly line is ugly
}

Sadly we won't be able to enjoy the better error messages inside such operations, so maybe my decision points are still somewhat relevant (but not a barrier anymore)

@marcospb19
Copy link
Member Author

builder.append_file(path, &mut file.file_mut())?;

You can remove that &mut.

The append_file method can fail if fs::metadata fails, or if io::Read::read fails.

I read the code for of docs.rs/tar, and it's a bit complicated, I'll create an issue asking if it's possible to add anBuilder::append_file_with_metadata, that would probably be the only way to pass an &mut dyn Read value without having it to call fs::metadata for that file.

Probably the best solution is to .map_err this to an FinalError.

builder.append_file("main.rs", input_file).map_err(|err| {
    FinalError::with_title(f!("Could not create archive '{}'", to_utf(&output_path)))
        .detail(f!("Unexpected error while trying to read file '{}'", to_utf(&file)))
        .detail(f!("Error: {}.", err))
})?;

And this will be converted by our impl From<FinalError> for Error implementation in src/error.rs.

I think this is the best solution because we have almost all the information necessary about this error, that is:

  • Which file (we have the path)
  • What kind of error (some type of read error)

Specifically, by adding err in it, we will know if it was ErrorKind::{TimedOut, Interrupted or OutOfMemory}.


And this error is very unlikely, so we probably shouldn't put any more excessive thought in it, as we have already opened the file by this point and I have never seen sudden I/O failures in my life


This is the only error that comes up with fs_err? Or the zip archive also complains?

@GabrielSimonetto
Copy link
Collaborator

GabrielSimonetto commented Oct 10, 2021

Hey (: I'll follow up on your suggestions later when I can test them out, thanks for the input!

As for the zip it seems to be fine, the only problem we had is easily fixable by importing permissions directly from fs

error[E0433]: failed to resolve: could not find `Permissions` in `fs`
   --> src/archive/zip.rs:130:44
    |
130 |         fs::set_permissions(file_path, fs::Permissions::from_mode(mode))?;
    |                                            ^^^^^^^^^^^ not found in `fs`
    |
help: consider importing this struct
    |
3   | use std::fs::Permissions;
    |

P.s. actually, we had an internal import set up for this particular function, all it took was to remove the fs::

#[cfg(unix)]
fn __unix_set_permissions(file_path: &Path, file: &ZipFile) -> crate::Result<()> {
    use std::os::unix::fs::PermissionsExt;

    if let Some(mode) = file.unix_mode() {
        fs::set_permissions(file_path, Permissions::from_mode(mode))?;
    }

    Ok(())
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants