vitiral/artifact

Windows: can't use subdirectories with '/' in settings

zilbuz opened this issue · 4 comments

I want to use the directory "docs/design" for the artifacts. I'm using artifacts_paths = ["docs/design"] in settings.toml because in general it seems that using '/' for subdirectories is standard for cross platform applications.

However this fails with the following error:

  ----- ERRORS -----:
[
  {
    "level": "Error",
    "path": "\\\\\\\\?\\\\C:\\\\my_art_project\\\\docs/design",
    "category": "LoadPaths",
    "msg": "Error during loading: La syntaxe du nom de fichier, de répertoire ou de volume est incorrecte. (os error 123) when getting metadata of \\\\?\\C:\\my_art_project\\docs/design"
  }
]

  ----- WARNINGS -----:
[]

The problem seems to be that path_abs canonicalize the project path before joining the artifact path. On windows this means that the path is converted to extended length path syntax and you can only use backslashes in the following calls to join (cf. stdfs::canonicalize()).

You apparently had a similar problem in v1 of artifact (#34).

I think the fix can be made in two places:

  • Either in artifact, where the paths joined to the project path can be converted with a similar function you wrote for #34;
  • Or directly in the path_abs crate, where the join() could do the conversion before calling the std::path::PathBuf::join() function.

An alternative could be to join all the paths in PathBuf before storing them in PathAbs.

What do you think?

(By the way, I would be happy to write you a PR to fix this once you have decided which action should be taken :) )

Some examples of the problem with `PathBuf` and `PathAbs`
    use std::path::PathBuf;
    use path_abs::PathAbs;

    // The path c:\a\b\c exists, c is also a directory
    
    // Using \ for join on a non-canonicalized path
    let path = PathBuf::from(r"c:\a");
    let path = path.join(r"b\c");
    assert!(path.metadata().is_ok()); //OK

    // Using / for join on a non-canonicalized path
    let path = PathBuf::from(r"c:\a");
    let path = path.join(r"b/c");
    assert!(path.metadata().is_ok()); //OK

    // Using \ for join on a canonicalized path
    let path = PathBuf::from(r"c:\a").canonicalize().unwrap();
    let path = path.join(r"b\c");
    assert!(path.metadata().is_ok()); //OK

    // Using / for join on a canonicalized path
    let path = PathBuf::from(r"c:\a").canonicalize().unwrap();
    let path = path.join(r"b/c");
    assert!(path.metadata().is_err()); //ERROR

    // Using \ for join on a path_abs
    let path = PathAbs::new(r"c:\a").unwrap();
    let path = path.join(r"b\c");
    assert!(path.metadata().is_ok()); //OK

    // Using / for join on a path_abs
    let path = PathAbs::new(r"c:\a").unwrap();
    let path = path.join(r"b/c");
    assert!(path.metadata().is_err()); //ERROR

Thanks for the bug! Yes, windows use-cases are very poorly tested at the moment, your feedback is extremely valuable.

I opened vitiral/path_abs#34 as I agree, it should be addressed there if possible.

#256 woot, windows now builds on travis and the tests tell the same story you do!

https://travis-ci.org/vitiral/artifact/jobs/478483421

phew, after basically rewriting path_abs for this purpose, this should now finally work.

I'm using a dev branch version of path_abs on master right now. I'm going to work to get that published and then will be uploading a new version. In the meantime, this should be fixed on master, and if you want to try it out then it would be much appreciated!

I've just tried it and it works! Thanks for the fix :)