Merge #336

336: Fix handling spaces in the sysroot path r=RalfJung a=RalfJung

Fixes #206

Co-authored-by: Ralf Jung <post@ralfj.de>

.github/workflows/ci.yml

@@ -67,7 +67,7 @@ jobs:
           - nightly
           # Don't remove this target; test coverage in `smoke.rs` relies on us
           # running at least one pinned toolchain.
-          - nightly-2020-12-01
+          - nightly-2021-08-01
 
     runs-on: ubuntu-20.04
 
@@ -82,11 +82,6 @@ jobs:
           toolchain: ${{ matrix.toolchain }}
           override: true
 
-      # We test against Cargo versions that don't support 'default-run'
-      # As a workaround, we remove 'default-run' from the Cargo.toml on CI
-      - name: Patch Cargo.toml
-        run: sed -i /^default-run/d Cargo.toml
-
       - name: Test
         uses: actions-rs/cargo@v1
         with:

src/cargo.rs

@@ -12,6 +12,7 @@ use util;
 use sysroot::XargoMode;
 use xargo::Home;
 
+#[derive(Clone)]
 pub struct Rustflags {
     flags: Vec<String>,
 }
@@ -41,12 +42,16 @@ impl Rustflags {
         }
     }
 
-    /// Stringifies these flags for Xargo consumption
-    pub fn for_xargo(&self, home: &Home) -> String {
-        let mut flags = self.flags.clone();
-        flags.push("--sysroot".to_owned());
-        flags.push(home.display().to_string());
-        flags.join(" ")
+    pub fn push(&mut self, flags: &[&str]) {
+        self.flags.extend(flags.iter().map(|w| w.to_string()));
+    }
+
+    /// Stringifies the default flags for Xargo consumption
+    pub fn encode(mut self, home: &Home) -> String {
+        self.flags.push("--sysroot".to_owned());
+        self.flags.push(home.display().to_string()); // FIXME: we shouldn't use display, we should keep the OsString
+        // As per CARGO_ENCODED_RUSTFLAGS docs, the separator is `0x1f`.
+        self.flags.join("\x1f")
     }
 }
 
@@ -60,16 +65,18 @@ pub fn rustflags(config: Option<&Config>, target: &str) -> Result<Rustflags> {
     flags(config, target, "rustflags").map(|fs| Rustflags { flags: fs })
 }
 
+#[derive(Clone)]
 pub struct Rustdocflags {
     flags: Vec<String>,
 }
 
 impl Rustdocflags {
     /// Stringifies these flags for Xargo consumption
-    pub fn for_xargo(mut self, home: &Home) -> String {
+    pub fn encode(mut self, home: &Home) -> String {
         self.flags.push("--sysroot".to_owned());
-        self.flags.push(home.display().to_string());
-        self.flags.join(" ")
+        self.flags.push(home.display().to_string()); // FIXME: we shouldn't use display, we should keep the OsString
+        // As per CARGO_ENCODED_RUSTFLAGS docs, the separator is `0x1f`.
+        self.flags.join("\x1f")
     }
 }
 
@@ -82,6 +89,7 @@ pub fn rustdocflags(config: Option<&Config>, target: &str) -> Result<Rustdocflag
 ///
 /// This looks into the environment and into `.cargo/config`
 fn flags(config: Option<&Config>, target: &str, tool: &str) -> Result<Vec<String>> {
+    // TODO: would be nice to also support the CARGO_ENCODED_ env vars
     if let Some(t) = env::var_os(tool.to_uppercase()) {
         return Ok(
             t.to_string_lossy()

src/sysroot.rs

@@ -133,12 +133,12 @@ version = "0.0.0"
 
         let cargo = || {
             let mut cmd = cargo::command();
-            let mut flags = rustflags.for_xargo(home);
-            flags.push_str(" -Z force-unstable-if-unmarked");
+            let mut flags = rustflags.clone();
+            flags.push(&["-Z", "force-unstable-if-unmarked"]);
             if verbose {
-                writeln!(io::stderr(), "+ RUSTFLAGS={:?}", flags).ok();
+                writeln!(io::stderr(), "+ RUSTFLAGS={}", flags).ok();
             }
-            cmd.env("RUSTFLAGS", flags);
+            cmd.env("CARGO_ENCODED_RUSTFLAGS", flags.encode(home));
 
             // Since we currently don't want to respect `.cargo/config` or `CARGO_TARGET_DIR`,
             // we need to force the target directory to match the `cp_r` below.

src/xargo.rs

@@ -28,16 +28,15 @@ pub fn run(
 
     if args.subcommand() == Some(Subcommand::Doc) {
         cmd.env(
-            "RUSTDOCFLAGS",
-            cargo::rustdocflags(config, cmode.triple())?.for_xargo(home),
+            "CARGO_ENCODED_RUSTDOCFLAGS",
+            cargo::rustdocflags(config, cmode.triple())?.encode(home),
         );
     }
 
-    let flags = rustflags.for_xargo(home);
     if verbose {
-        writeln!(io::stderr(), "+ RUSTFLAGS={:?}", flags).ok();
+        writeln!(io::stderr(), "+ RUSTFLAGS={}", rustflags).ok();
     }
-    cmd.env("RUSTFLAGS", flags);
+    cmd.env("CARGO_ENCODED_RUSTFLAGS", rustflags.encode(home));
 
     let locks = (home.lock_ro(&meta.host), home.lock_ro(cmode.triple()));
 

tests/smoke.rs

@@ -322,7 +322,8 @@ impl HProject {
 
         let guard = ONCE.lock();
 
-        let td = TempDir::new("xargo").chain_err(|| "couldn't create a temporary directory")?;
+        // Put a space into the working directory name to also test that case.
+        let td = TempDir::new("xar go").chain_err(|| "couldn't create a temporary directory")?;
 
         xargo()?
             .args(&["init", "-q", "--lib", "--vcs", "none", "--name", "host"])
@@ -629,7 +630,7 @@ rustflags = ["--cfg", "xargo"]
     run!()
 }
 
-/// Check that RUSTFLAGS are passed to all `rustc`s
+/// Check that RUSTFLAGS are passed to all `rustc`s, and that they can contain spaces.
 #[test]
 fn rustflags() {
     fn run() -> Result<()> {
@@ -640,17 +641,14 @@ fn rustflags() {
         project.config(
             r#"
 [build]
-rustflags = ["--cfg", "xargo"]
+rustflags = ["--cfg", 'xargo="y e s"']
 "#,
         )?;
 
         let stderr = project.build_and_get_stderr(Some(TARGET))?;
 
         assert!(
-            stderr
-                .lines()
-                .filter(|l| !l.starts_with("+") && l.contains("rustc") && !l.contains("rustc-std-workspace"))
-                .all(|l| l.contains("--cfg") && l.contains("xargo")),
+            stderr.contains(r#"'xargo="y e s"'"#),
             "unexpected stderr:\n{}", stderr
         );