Repository: dtolnay/cxx (language: rust)

Issue:
Don't rely on the target directory being named 'target'
Currently, `paths::target_dir()` tries to find the `target` directory by looking at the name.
This has two problems:
1. The user might have specified a different target directory by using the Cargo config option `target-dir` (https://doc.rust-lang.org/cargo/reference/config.html)
2. The target directory might be called target, but actually be a symlink to something else that isn't called `target` - and `canonicalize` resolves symlinks.

In both cases, `paths::target_dir()` fails.

What is the rationale for not putting all the generated code etc. in the OUT_DIR or a subdirectory thereof directly? If the latter is acceptable, I'm happy to submit a pull request changing the behavior...


Discussion / hints:
I would appreciate a PR to improve this.

Fix patch:
diff --git a/demo/Cargo.toml b/demo/Cargo.toml
index d2147ab87..dc9486101 100644
--- a/demo/Cargo.toml
+++ b/demo/Cargo.toml
@@ -1,5 +1,5 @@
 [package]
-name = "cxxbridge-demo"
+name = "demo"
 version = "0.0.0"
 authors = ["David Tolnay <dtolnay@gmail.com>"]
 edition = "2018"
diff --git a/gen/build/src/cargo.rs b/gen/build/src/cargo.rs
index 3bddc6edd..b67c21b37 100644
--- a/gen/build/src/cargo.rs
+++ b/gen/build/src/cargo.rs
@@ -1,19 +1,19 @@
-use crate::error::TargetDirError;
 use crate::paths::TargetDir;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 use std::str;
 
-pub(crate) fn target_dir() -> Result<TargetDir, TargetDirError> {
-    let cargo = option_env!("CARGO").unwrap_or("cargo");
-    let output = Command::new(cargo)
-        .arg("metadata")
-        .arg("--no-deps")
-        .arg("--format-version=1")
-        .output()
-        .map_err(TargetDirError::Io)?;
-
+pub(crate) fn target_dir(out_dir: &Path) -> TargetDir {
     (|| {
+        let cargo = option_env!("CARGO").unwrap_or("cargo");
+        let output = Command::new(cargo)
+            .current_dir(out_dir)
+            .arg("metadata")
+            .arg("--no-deps")
+            .arg("--format-version=1")
+            .output()
+            .ok()?;
+
         // Cargo only outputs utf8 encoded JSON.
         let mut metadata = str::from_utf8(&output.stdout).ok()?;
 
@@ -25,7 +25,7 @@ pub(crate) fn target_dir() -> Result<TargetDir, TargetDirError> {
         let close_quote_index = metadata.find('"')?;
         let string = &metadata[..close_quote_index];
         let target_directory = string.replace("\\\\", "\\");
-        Some(TargetDir(PathBuf::from(target_directory)))
+        Some(TargetDir::Path(PathBuf::from(target_directory)))
     })()
-    .ok_or(TargetDirError::NotFound)
+    .unwrap_or(TargetDir::Unknown)
 }
diff --git a/gen/build/src/error.rs b/gen/build/src/error.rs
index ed4c6af65..0ed4d74d1 100644
--- a/gen/build/src/error.rs
+++ b/gen/build/src/error.rs
@@ -1,28 +1,19 @@
 use crate::gen::fs;
 use std::error::Error as StdError;
 use std::fmt::{self, Display};
-use std::io;
 
 pub(super) type Result<T, E = Error> = std::result::Result<T, E>;
 
 #[derive(Debug)]
 pub(super) enum Error {
     MissingOutDir,
-    TargetDir(TargetDirError),
     Fs(fs::Error),
 }
 
-#[derive(Debug)]
-pub(crate) enum TargetDirError {
-    Io(io::Error),
-    NotFound,
-}
-
 impl Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             Error::MissingOutDir => write!(f, "missing OUT_DIR environment variable"),
-            Error::TargetDir(_) => write!(f, "unable to identify target dir"),
             Error::Fs(err) => err.fmt(f),
         }
     }
@@ -31,10 +22,6 @@ impl Display for Error {
 impl StdError for Error {
     fn source(&self) -> Option<&(dyn StdError + 'static)> {
         match self {
-            Error::TargetDir(err) => match err {
-                TargetDirError::Io(err) => Some(err),
-                TargetDirError::NotFound => None,
-            },
             Error::Fs(err) => err.source(),
             _ => None,
         }
diff --git a/gen/build/src/lib.rs b/gen/build/src/lib.rs
index 22f53f65a..162851088 100644
--- a/gen/build/src/lib.rs
+++ b/gen/build/src/lib.rs
@@ -59,14 +59,14 @@ mod gen;
 mod paths;
 mod syntax;
 
-use crate::error::Result;
+use crate::error::{Error, Result};
 use crate::gen::error::report;
 use crate::gen::{fs, Opt};
-use crate::paths::TargetDir;
+use crate::paths::{PathExt, TargetDir};
 use cc::Build;
 use std::io::{self, Write};
 use std::iter;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process;
 
 /// This returns a [`cc::Build`] on which you should continue to set up any
@@ -98,45 +98,99 @@ pub fn bridges(rust_source_files: impl IntoIterator<Item = impl AsRef<Path>>) ->
     })
 }
 
+struct Project {
+    out_dir: PathBuf,
+    target_dir: TargetDir,
+}
+
+impl Project {
+    fn init() -> Result<Self> {
+        let out_dir = paths::out_dir()?;
+
+        let target_dir = match cargo::target_dir(&out_dir) {
+            target_dir @ TargetDir::Path(_) => target_dir,
+            // Fallback if Cargo did not work.
+            TargetDir::Unknown => paths::search_parents_for_target_dir(&out_dir),
+        };
+
+        Ok(Project {
+            out_dir,
+            target_dir,
+        })
+    }
+}
+
 fn build(rust_source_files: &mut dyn Iterator<Item = impl AsRef<Path>>) -> Result<Build> {
-    let ref target_dir = paths::target_dir()?;
-    let mut build = paths::cc_build(target_dir);
+    let ref prj = Project::init()?;
+    let mut build = Build::new();
     build.cpp(true);
     build.cpp_link_stdlib(None); // linked via link-cplusplus crate
-    write_header(target_dir);
+    build.include(paths::include_dir(prj));
+    write_header(prj);
+    symlink_crate(prj, &mut build);
 
     for path in rust_source_files {
-        generate_bridge(&mut build, path.as_ref(), target_dir)?;
+        generate_bridge(prj, &mut build, path.as_ref())?;
     }
 
     Ok(build)
 }
 
-fn write_header(target_dir: &TargetDir) {
-    let ref cxx_h = paths::include_dir(target_dir).join("rust").join("cxx.h");
+fn write_header(prj: &Project) {
+    let ref cxx_h = prj.out_dir.join("cxxbridge").join("rust").join("cxx.h");
     let _ = write(cxx_h, gen::include::HEADER.as_bytes());
+    if let TargetDir::Path(target_dir) = &prj.target_dir {
+        let ref header_dir = target_dir.join("cxxbridge").join("rust");
+        let _ = fs::create_dir_all(header_dir);
+        let ref cxx_h = header_dir.join("cxx.h");
+        let _ = write(cxx_h, gen::include::HEADER.as_bytes());
+    }
 }
 
-fn generate_bridge(
-    build: &mut Build,
-    rust_source_file: &Path,
-    target_dir: &TargetDir,
-) -> Result<()> {
+fn symlink_crate(prj: &Project, build: &mut Build) {
+    let manifest_dir = match paths::manifest_dir() {
+        Some(manifest_dir) => manifest_dir,
+        None => return,
+    };
+    let package_name = match paths::package_name() {
+        Some(package_name) => package_name,
+        None => return,
+    };
+
+    let mut link = paths::include_dir(prj);
+    link.push("CRATE");
+    let _ = fs::create_dir_all(&link);
+    let _ = paths::symlink_dir(manifest_dir, link.join(package_name));
+    build.include(link);
+}
+
+fn generate_bridge(prj: &Project, build: &mut Build, rust_source_file: &Path) -> Result<()> {
     let opt = Opt::default();
     let generated = gen::generate_from_path(rust_source_file, &opt);
+    let ref rel_path = paths::local_relative_path(rust_source_file);
+
+    let ref rel_path_h = rel_path.with_appended_extension(".h");
+    let ref header_path = paths::namespaced(&prj.out_dir, rel_path_h);
+    write(header_path, &generated.header)?;
 
-    let header_path = paths::out_with_extension(rust_source_file, ".h", target_dir)?;
-    fs::create_dir_all(header_path.parent().unwrap())?;
-    write(&header_path, &generated.header)?;
-    paths::symlink_header(&header_path, rust_source_file, target_dir);
+    let ref link_path = paths::namespaced(&prj.out_dir, rel_path);
+    let _ = paths::symlink_or_copy(header_path, link_path);
+    if let TargetDir::Path(target_dir) = &prj.target_dir {
+        let ref link_path = paths::namespaced(target_dir, rel_path);
+        let _ = fs::create_dir_all(link_path.parent().unwrap());
+        let _ = paths::symlink_or_copy(header_path, link_path);
+        let _ = paths::symlink_or_copy(header_path, link_path.with_appended_extension(".h"));
+    }
 
-    let implementation_path = paths::out_with_extension(rust_source_file, ".cc", target_dir)?;
-    write(&implementation_path, &generated.implementation)?;
-    build.file(&implementation_path);
+    let ref rel_path_cc = rel_path.with_appended_extension(".cc");
+    let ref implementation_path = paths::namespaced(&prj.out_dir, rel_path_cc);
+    write(implementation_path, &generated.implementation)?;
+    build.file(implementation_path);
     Ok(())
 }
 
 fn write(path: &Path, content: &[u8]) -> Result<()> {
+    let mut create_dir_error = None;
     if path.exists() {
         if let Ok(existing) = fs::read(path) {
             if existing == content {
@@ -146,8 +200,14 @@ fn write(path: &Path, content: &[u8]) -> Result<()> {
         }
         let _ = fs::remove_file(path);
     } else {
-        let _ = fs::create_dir_all(path.parent().unwrap());
+        let parent = path.parent().unwrap();
+        create_dir_error = fs::create_dir_all(parent).err();
+    }
+
+    match fs::write(path, content) {
+        // As long as write succeeded, ignore any create_dir_all error.
+        Ok(()) => Ok(()),
+        // If create_dir_all and write both failed, prefer the first error.
+        Err(err) => Err(Error::Fs(create_dir_error.unwrap_or(err))),
     }
-    fs::write(path, content)?;
-    Ok(())
 }
diff --git a/gen/build/src/paths.rs b/gen/build/src/paths.rs
index 870c49d69..96e370871 100644
--- a/gen/build/src/paths.rs
+++ b/gen/build/src/paths.rs
@@ -1,124 +1,109 @@
-use crate::cargo;
 use crate::error::{Error, Result};
 use crate::gen::fs;
+use crate::Project;
 use std::env;
-use std::ops::Deref;
-use std::path::{Path, PathBuf};
+use std::ffi::{OsStr, OsString};
+use std::path::{Component, Path, PathBuf};
 
-pub(crate) struct TargetDir(pub PathBuf);
-
-impl Deref for TargetDir {
-    type Target = Path;
-    fn deref(&self) -> &Self::Target {
-        &self.0
-    }
+pub(crate) enum TargetDir {
+    Path(PathBuf),
+    Unknown,
 }
 
-fn out_dir() -> Result<PathBuf> {
+pub(crate) fn out_dir() -> Result<PathBuf> {
     env::var_os("OUT_DIR")
         .map(PathBuf::from)
         .ok_or(Error::MissingOutDir)
 }
 
-pub(crate) fn cc_build(target_dir: &TargetDir) -> cc::Build {
-    let mut build = cc::Build::new();
-    build.include(include_dir(target_dir));
-    build.include(target_dir.parent().unwrap());
-    build
+// Given a path provided by the user, determines where generated files related
+// to that path should go in our out dir. In particular we don't want to
+// accidentally write generated code upward of our out dir, even if the user
+// passed a path containing lots of `..` or an absolute path.
+pub(crate) fn local_relative_path(path: &Path) -> PathBuf {
+    let mut rel_path = PathBuf::new();
+    for component in path.components() {
+        match component {
+            Component::Prefix(_) | Component::RootDir | Component::CurDir => {}
+            Component::ParentDir => drop(rel_path.pop()), // noop if empty
+            Component::Normal(name) => rel_path.push(name),
+        }
+    }
+    rel_path
 }
 
-// Symlink the header file into a predictable place. The header generated from
-// path/to/mod.rs gets linked to targets/cxxbridge/path/to/mod.rs.h.
-pub(crate) fn symlink_header(path: &Path, original: &Path, target_dir: &TargetDir) {
-    let _ = try_symlink_header(path, original, target_dir);
+pub(crate) fn namespaced(base: &Path, rel_path: &Path) -> PathBuf {
+    let mut path = base.to_owned();
+    path.push("cxxbridge");
+    path.extend(package_name());
+    path.push(rel_path);
+    path
 }
 
-fn try_symlink_header(path: &Path, original: &Path, target_dir: &TargetDir) -> Result<()> {
-    let suffix = relative_to_parent_of_target_dir(original, target_dir)?;
-    let ref dst = include_dir(target_dir).join(suffix);
-
-    fs::create_dir_all(dst.parent().unwrap())?;
-    let _ = fs::remove_file(dst);
-    symlink_or_copy(path, dst)?;
-
-    let mut file_name = dst.file_name().unwrap().to_os_string();
-    file_name.push(".h");
-    let ref dst2 = dst.with_file_name(file_name);
-    symlink_or_copy(path, dst2)?;
+pub(crate) trait PathExt {
+    fn with_appended_extension(&self, suffix: impl AsRef<OsStr>) -> PathBuf;
+}
 
-    Ok(())
+impl PathExt for Path {
+    fn with_appended_extension(&self, suffix: impl AsRef<OsStr>) -> PathBuf {
+        let mut file_name = self.file_name().unwrap().to_owned();
+        file_name.push(suffix);
+        self.with_file_name(file_name)
+    }
 }
 
-fn relative_to_parent_of_target_dir(original: &Path, target_dir: &TargetDir) -> Result<PathBuf> {
-    let mut outer = target_dir.parent().unwrap();
-    let original = canonicalize(original)?;
-    loop {
-        if let Ok(suffix) = original.strip_prefix(outer) {
-            return Ok(suffix.to_owned());
-        }
-        match outer.parent() {
-            Some(parent) => outer = parent,
-            None => return Ok(original.components().skip(1).collect()),
-        }
+pub(crate) fn include_dir(prj: &Project) -> PathBuf {
+    match &prj.target_dir {
+        TargetDir::Path(target_dir) => target_dir.join("cxxbridge"),
+        TargetDir::Unknown => prj.out_dir.join("cxxbridge"),
     }
 }
 
-pub(crate) fn out_with_extension(
-    path: &Path,
-    ext: &str,
-    target_dir: &TargetDir,
-) -> Result<PathBuf> {
-    let mut file_name = path.file_name().unwrap().to_owned();
-    file_name.push(ext);
-
-    let out_dir = out_dir()?;
-    let rel = relative_to_parent_of_target_dir(path, target_dir)?;
-    Ok(out_dir.join(rel).with_file_name(file_name))
+pub(crate) fn manifest_dir() -> Option<PathBuf> {
+    env::var_os("CARGO_MANIFEST_DIR").map(PathBuf::from)
 }
 
-pub(crate) fn include_dir(target_dir: &TargetDir) -> PathBuf {
-    target_dir.join("cxxbridge")
+pub(crate) fn package_name() -> Option<OsString> {
+    env::var_os("CARGO_PKG_NAME")
 }
 
-pub(crate) fn target_dir() -> Result<TargetDir> {
-    let fallback_err = match cargo::target_dir() {
-        Ok(target_dir) => return Ok(target_dir),
-        Err(err) => Error::TargetDir(err),
-    };
+pub(crate) fn search_parents_for_target_dir(out_dir: &Path) -> TargetDir {
+    // fs::canonicalize on Windows produces UNC paths which cl.exe is unable to
+    // handle in includes.
+    // https://github.com/rust-lang/rust/issues/42869
+    // https://github.com/alexcrichton/cc-rs/issues/169
+    let mut also_try_canonical = cfg!(not(windows));
 
-    // Fallback if Cargo did not work.
-    let mut dir = out_dir().and_then(canonicalize)?;
+    let mut dir = out_dir.to_owned();
     loop {
-        if dir.ends_with("target") {
-            return Ok(TargetDir(dir));
+        let is_target = dir.ends_with("target");
+        let parent_contains_cargo_toml = dir.with_file_name("Cargo.toml").exists();
+        if is_target && parent_contains_cargo_toml {
+            return TargetDir::Path(dir);
         }
-        if !dir.pop() {
-            return Err(fallback_err);
+        if dir.pop() {
+            continue;
         }
+        if also_try_canonical {
+            if let Ok(canonical_dir) = out_dir.canonicalize() {
+                dir = canonical_dir;
+                also_try_canonical = false;
+                continue;
+            }
+        }
+        return TargetDir::Unknown;
     }
 }
 
-#[cfg(not(windows))]
-fn canonicalize(path: impl AsRef<Path>) -> Result<PathBuf> {
-    Ok(fs::canonicalize(path)?)
-}
-
-#[cfg(windows)]
-fn canonicalize(path: impl AsRef<Path>) -> Result<PathBuf> {
-    // Real fs::canonicalize on Windows produces UNC paths which cl.exe is
-    // unable to handle in includes. Use a poor approximation instead.
-    // https://github.com/rust-lang/rust/issues/42869
-    // https://github.com/alexcrichton/cc-rs/issues/169
-    Ok(fs::current_dir()?.join(path))
-}
-
 #[cfg(unix)]
-use self::fs::symlink_file as symlink_or_copy;
+pub(crate) use self::fs::symlink_file as symlink_or_copy;
 
 #[cfg(windows)]
-fn symlink_or_copy(src: &Path, dst: &Path) -> Result<()> {
+pub(crate) fn symlink_or_copy(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> Result<()> {
     // Pre-Windows 10, symlinks require admin privileges. Since Windows 10, they
     // require Developer Mode. If it fails, fall back to copying the file.
+    let src = src.as_ref();
+    let dst = dst.as_ref();
     if fs::symlink_file(src, dst).is_err() {
         fs::copy(src, dst)?;
     }
@@ -126,4 +111,12 @@ fn symlink_or_copy(src: &Path, dst: &Path) -> Result<()> {
 }
 
 #[cfg(not(any(unix, windows)))]
-use self::fs::copy as symlink_or_copy;
+pub(crate) use self::fs::copy as symlink_or_copy;
+
+#[cfg(any(unix, windows))]
+pub(crate) use self::fs::symlink_dir;
+
+#[cfg(not(any(unix, windows)))]
+pub(crate) fn symlink_dir(_src: impl AsRef<Path>, _dst: impl AsRef<Path>) -> Result<()> {
+    Ok(())
+}
diff --git a/gen/src/fs.rs b/gen/src/fs.rs
index 77488f86b..fe15f868d 100644
--- a/gen/src/fs.rs
+++ b/gen/src/fs.rs
@@ -34,14 +34,6 @@ macro_rules! err {
     }
 }
 
-pub(crate) fn canonicalize(path: impl AsRef<Path>) -> Result<PathBuf> {
-    let path = path.as_ref();
-    match std::fs::canonicalize(path) {
-        Ok(string) => Ok(string),
-        Err(e) => err!(e, "Unable to canonicalize path: `{}`", path),
-    }
-}
-
 pub(crate) fn copy(from: impl AsRef<Path>, to: impl AsRef<Path>) -> Result<u64> {
     let from = from.as_ref();
     let to = to.as_ref();
diff --git a/third-party/Cargo.lock b/third-party/Cargo.lock
index 1f3e760c1..b335d31f3 100644
--- a/third-party/Cargo.lock
+++ b/third-party/Cargo.lock
@@ -114,14 +114,6 @@ dependencies = [
  "syn",
 ]
 
-[[package]]
-name = "cxxbridge-demo"
-version = "0.0.0"
-dependencies = [
- "cxx",
- "cxx-build",
-]
-
 [[package]]
 name = "cxxbridge-flags"
 version = "0.3.9"
@@ -136,6 +128,14 @@ dependencies = [
  "syn",
 ]
 
+[[package]]
+name = "demo"
+version = "0.0.0"
+dependencies = [
+ "cxx",
+ "cxx-build",
+]
+
 [[package]]
 name = "dissimilar"
 version = "1.0.2"


Test patch:
diff --git a/tests/BUCK b/tests/BUCK
index 4bfc9e8a8..a96bfb8d0 100644
--- a/tests/BUCK
+++ b/tests/BUCK
@@ -24,9 +24,10 @@ cxx_library(
         ":gen-lib-source",
         ":gen-module-source",
     ],
+    header_namespace = "cxx-test-suite",
     headers = {
-        "ffi/lib.rs.h": ":gen-lib-header",
-        "ffi/tests.h": "ffi/tests.h",
+        "lib.rs.h": ":gen-lib-header",
+        "tests.h": "ffi/tests.h",
     },
     deps = ["//:core"],
 )
diff --git a/tests/BUILD b/tests/BUILD
index e1f163718..29cc79ce5 100644
--- a/tests/BUILD
+++ b/tests/BUILD
@@ -26,6 +26,8 @@ cc_library(
         ":gen-module-source",
     ],
     hdrs = ["ffi/tests.h"],
+    include_prefix = "cxx-test-suite",
+    strip_include_prefix = "ffi",
     deps = [
         ":lib-include",
         "//:core",
@@ -51,7 +53,7 @@ genrule(
 cc_library(
     name = "lib-include",
     hdrs = [":gen-lib-header"],
-    include_prefix = "tests/ffi",
+    include_prefix = "cxx-test-suite",
 )
 
 genrule(
diff --git a/tests/ffi/lib.rs b/tests/ffi/lib.rs
index 8aaac75ba..d6724f569 100644
--- a/tests/ffi/lib.rs
+++ b/tests/ffi/lib.rs
@@ -23,7 +23,7 @@ pub mod ffi {
     }
 
     extern "C" {
-        include!("tests/ffi/tests.h");
+        include!("cxx-test-suite/tests.h");
 
         type C;
 
diff --git a/tests/ffi/module.rs b/tests/ffi/module.rs
index 77bae0664..8862dc161 100644
--- a/tests/ffi/module.rs
+++ b/tests/ffi/module.rs
@@ -4,7 +4,7 @@
 #[cxx::bridge(namespace = tests)]
 pub mod ffi {
     extern "C" {
-        include!("tests/ffi/tests.h");
+        include!("cxx-test-suite/tests.h");
 
         type C = crate::ffi::C;
 
diff --git a/tests/ffi/tests.cc b/tests/ffi/tests.cc
index d45d5d536..8dd16e8d9 100644
--- a/tests/ffi/tests.cc
+++ b/tests/ffi/tests.cc
@@ -1,5 +1,5 @@
-#include "tests/ffi/tests.h"
-#include "tests/ffi/lib.rs.h"
+#include "cxx-test-suite/tests.h"
+#include "cxx-test-suite/lib.rs.h"
 #include <cstring>
 #include <iterator>
 #include <numeric>