Repository: oli-obk/cargo_metadata
Pull request #152 (2021-02-24 19:06:26)

[RFC] use camino::Utf8PathBuf for returned paths
[`camino`] is a library I just released which provides thin wrappers over Path
and PathBuf which guarantee that their contents are UTF-8. The readme at
https://crates.io/crates/camino explains the advantages of using this
library.

Given that:
* `cargo` only supports UTF-8 paths for cross-platform compatibility
  reasons
* `serde` only supports UTF-8 paths: https://docs.serde.rs/src/serde/ser/impls.rs.html#776-778
* JSON, as a format, only supports UTF-8 strings

all valid metadata instances are already forced to be UTF-8. This means
that `cargo_metadata` can switch over to using UTF-8 paths without any
loss of functionality.

What do you think?

[`camino`]: https://crates.io/crates/camino

sunshowers: Ah I guess one issue is that cargo_metadata supports Rust 1.32 and above while camino only supports Rust 1.39 or above (I picked Debian stable minus 2 versions). Looks like camino can support 1.34 without any changes -- I can release an update promising support for 1.34 if you think camino is useful and are willing to bump the MSRV to that.

Patch:
diff --git a/.travis.yml b/.travis.yml
index 09de03d..1717518 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -5,7 +5,7 @@ matrix:
     - rust: "nightly"
     - rust: "beta"
     - rust: "stable"
-    - rust: "1.32.0"
+    - rust: "1.34.0"
 
 script:
   - cargo build --verbose
diff --git a/Cargo.toml b/Cargo.toml
index b9d34c7..3fe7df4 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -9,6 +9,7 @@ readme = "README.md"
 edition = "2018"
 
 [dependencies]
+camino = { version = "1.0.1", features = ["serde1"] }
 cargo-platform = "0.1"
 semver = { version = "0.11.0", features = ["serde"] }
 semver-parser = "0.10.2"
diff --git a/src/dependency.rs b/src/dependency.rs
index ac57ff0..44c5956 100644
--- a/src/dependency.rs
+++ b/src/dependency.rs
@@ -1,8 +1,8 @@
 //! This module contains `Dependency` and the types/functions it uses for deserialization.
 
+use camino::Utf8PathBuf;
 use semver::VersionReq;
 use serde::{Deserialize, Deserializer, Serialize};
-use std::path::PathBuf;
 
 #[derive(Eq, PartialEq, Clone, Debug, Copy, Hash, Serialize, Deserialize)]
 /// Dependencies can come in three kinds
@@ -69,7 +69,7 @@ pub struct Dependency {
     /// The file system path for a local path dependency.
     ///
     /// Only produced on cargo 1.51+
-    pub path: Option<PathBuf>,
+    pub path: Option<Utf8PathBuf>,
     #[doc(hidden)]
     #[serde(skip)]
     __do_not_match_exhaustively: (),
diff --git a/src/lib.rs b/src/lib.rs
index de5cf76..aa531a2 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -78,6 +78,7 @@
 //! let output = command.wait().expect("Couldn't get cargo's exit status");
 //! ```
 
+use camino::Utf8PathBuf;
 use std::collections::HashMap;
 use std::env;
 use std::fmt;
@@ -85,6 +86,7 @@ use std::path::PathBuf;
 use std::process::Command;
 use std::str::from_utf8;
 
+pub use camino;
 pub use semver::Version;
 
 pub use dependency::{Dependency, DependencyKind};
@@ -138,9 +140,9 @@ pub struct Metadata {
     /// Dependencies graph
     pub resolve: Option<Resolve>,
     /// Workspace root
-    pub workspace_root: PathBuf,
+    pub workspace_root: Utf8PathBuf,
     /// Build directory
-    pub target_directory: PathBuf,
+    pub target_directory: Utf8PathBuf,
     /// The workspace-level metadata object. Null if non-existent.
     #[serde(rename = "metadata", default, skip_serializing_if = "is_null")]
     pub workspace_metadata: serde_json::Value,
@@ -272,13 +274,13 @@ pub struct Package {
     pub license: Option<String>,
     /// If the package is using a nonstandard license, this key may be specified instead of
     /// `license`, and must point to a file relative to the manifest.
-    pub license_file: Option<PathBuf>,
+    pub license_file: Option<Utf8PathBuf>,
     /// Targets provided by the crate (lib, bin, example, test, ...)
     pub targets: Vec<Target>,
     /// Features provided by the crate, mapped to the features required by that feature.
     pub features: HashMap<String, Vec<String>>,
     /// Path containing the `Cargo.toml`
-    pub manifest_path: PathBuf,
+    pub manifest_path: Utf8PathBuf,
     /// Categories as given in the `Cargo.toml`
     #[serde(default)]
     pub categories: Vec<String>,
@@ -286,7 +288,7 @@ pub struct Package {
     #[serde(default)]
     pub keywords: Vec<String>,
     /// Readme as given in the `Cargo.toml`
-    pub readme: Option<PathBuf>,
+    pub readme: Option<Utf8PathBuf>,
     /// Repository as given in the `Cargo.toml`
     // can't use `url::Url` because that requires a more recent stable compiler
     pub repository: Option<String>,
@@ -344,7 +346,7 @@ pub struct Package {
 
 impl Package {
     /// Full path to the license file if one is present in the manifest
-    pub fn license_file(&self) -> Option<PathBuf> {
+    pub fn license_file(&self) -> Option<Utf8PathBuf> {
         self.license_file.as_ref().map(|file| {
             self.manifest_path
                 .parent()
@@ -354,7 +356,7 @@ impl Package {
     }
 
     /// Full path to the readme file if one is present in the manifest
-    pub fn readme(&self) -> Option<PathBuf> {
+    pub fn readme(&self) -> Option<Utf8PathBuf> {
         self.readme
             .as_ref()
             .map(|file| self.manifest_path.join(file))
@@ -400,7 +402,7 @@ pub struct Target {
     /// It doesn't apply to `lib` targets.
     pub required_features: Vec<String>,
     /// Path to the main source file of the target
-    pub src_path: PathBuf,
+    pub src_path: Utf8PathBuf,
     /// Rust edition for this target
     #[serde(default = "edition_default")]
     pub edition: String,
diff --git a/src/messages.rs b/src/messages.rs
index 9b4fceb..1b5c641 100644
--- a/src/messages.rs
+++ b/src/messages.rs
@@ -1,8 +1,8 @@
 use super::{Diagnostic, PackageId, Target};
+use camino::Utf8PathBuf;
 use serde::{Deserialize, Serialize};
 use std::fmt;
 use std::io::{self, BufRead, Lines, Read};
-use std::path::PathBuf;
 
 /// Profile settings used to determine which compiler flags to use for a
 /// target.
@@ -37,9 +37,9 @@ pub struct Artifact {
     pub features: Vec<String>,
     /// The full paths to the generated artifacts
     /// (e.g. binary file and separate debug info)
-    pub filenames: Vec<PathBuf>,
+    pub filenames: Vec<Utf8PathBuf>,
     /// Path to the executable file
-    pub executable: Option<PathBuf>,
+    pub executable: Option<Utf8PathBuf>,
     /// If true, then the files were already generated
     pub fresh: bool,
     #[doc(hidden)]
@@ -68,9 +68,9 @@ pub struct BuildScript {
     /// The package this build script execution belongs to
     pub package_id: PackageId,
     /// The libs to link
-    pub linked_libs: Vec<PathBuf>,
+    pub linked_libs: Vec<Utf8PathBuf>,
     /// The paths to search when resolving libs
-    pub linked_paths: Vec<PathBuf>,
+    pub linked_paths: Vec<Utf8PathBuf>,
     /// Various `--cfg` flags to pass to the compiler
     pub cfgs: Vec<String>,
     /// The environment variables to add to the compilation
@@ -79,7 +79,7 @@ pub struct BuildScript {
     ///
     /// Added in Rust 1.41.
     #[serde(default)]
-    pub out_dir: PathBuf,
+    pub out_dir: Utf8PathBuf,
     #[doc(hidden)]
     #[serde(skip)]
     __do_not_match_exhaustively: (),


Test patch:
diff --git a/tests/test_samples.rs b/tests/test_samples.rs
index dfa0fb0..a0fd55f 100644
--- a/tests/test_samples.rs
+++ b/tests/test_samples.rs
@@ -3,8 +3,8 @@ extern crate semver;
 #[macro_use]
 extern crate serde_json;
 
+use camino::Utf8PathBuf;
 use cargo_metadata::{CargoOpt, DependencyKind, Metadata, MetadataCommand};
-use std::path::PathBuf;
 
 #[test]
 fn old_minimal() {
@@ -91,12 +91,12 @@ fn old_minimal() {
     assert_eq!(target.kind, vec!["bin"]);
     assert_eq!(target.crate_types, vec!["bin"]);
     assert_eq!(target.required_features.len(), 0);
-    assert_eq!(target.src_path, PathBuf::from("/foo/src/main.rs"));
+    assert_eq!(target.src_path, "/foo/src/main.rs");
     assert_eq!(target.edition, "2015");
     assert_eq!(target.doctest, true);
     assert_eq!(target.test, true);
     assert_eq!(pkg.features.len(), 0);
-    assert_eq!(pkg.manifest_path, PathBuf::from("/foo/Cargo.toml"));
+    assert_eq!(pkg.manifest_path, "/foo/Cargo.toml");
     assert_eq!(pkg.categories.len(), 0);
     assert_eq!(pkg.keywords.len(), 0);
     assert_eq!(pkg.readme, None);
@@ -113,9 +113,9 @@ fn old_minimal() {
         "foo 0.1.0 (path+file:///foo)"
     );
     assert!(meta.resolve.is_none());
-    assert_eq!(meta.workspace_root, PathBuf::from("/foo"));
+    assert_eq!(meta.workspace_root, "/foo");
     assert_eq!(meta.workspace_metadata, serde_json::Value::Null);
-    assert_eq!(meta.target_directory, PathBuf::from("/foo/target"));
+    assert_eq!(meta.target_directory, "/foo/target");
 }
 
 macro_rules! sorted {
@@ -177,10 +177,7 @@ fn all_the_fields() {
         .manifest_path("tests/all/Cargo.toml")
         .exec()
         .unwrap();
-    assert_eq!(
-        meta.workspace_root.file_name().unwrap(),
-        PathBuf::from("all")
-    );
+    assert_eq!(meta.workspace_root.file_name().unwrap(), "all");
     assert_eq!(
         serde_json::from_value::<WorkspaceMetadata>(meta.workspace_metadata).unwrap(),
         WorkspaceMetadata {
@@ -199,7 +196,7 @@ fn all_the_fields() {
     assert!(all.id.to_string().starts_with("all"));
     assert_eq!(all.description, Some("Package description.".to_string()));
     assert_eq!(all.license, Some("MIT/Apache-2.0".to_string()));
-    assert_eq!(all.license_file, Some(PathBuf::from("LICENSE")));
+    assert_eq!(all.license_file, Some(Utf8PathBuf::from("LICENSE")));
     assert!(all.license_file().unwrap().ends_with("tests/all/LICENSE"));
     assert_eq!(all.publish, Some(vec![]));
     assert_eq!(all.links, Some("foo".to_string()));
@@ -329,7 +326,7 @@ fn all_the_fields() {
     assert!(all.manifest_path.ends_with("all/Cargo.toml"));
     assert_eq!(all.categories, vec!["command-line-utilities"]);
     assert_eq!(all.keywords, vec!["cli"]);
-    assert_eq!(all.readme, Some(PathBuf::from("README.md")));
+    assert_eq!(all.readme, Some(Utf8PathBuf::from("README.md")));
     assert_eq!(
         all.repository,
         Some("https://github.com/oli-obk/cargo_metadata/".to_string())