Do a better job of cleaning up copied sysroot directories (e.g. includes)

Previously, the sysroot build copied include files from several
different directories into the final sysroot include dir, but did not
do anything to reset the state of the include directory. If the same
sysroot dir was used to build sysroots from multiple different
versions of hyperlight-libc, since the headers provided are different,
downstream code would get a confusing (and likely to fail to compile)
mix of the include files from the two libcs.  This changes the way
that the include directory (and other copied sysroot directories) are
constructed, to ensure that stale files are removed as well as new
files being copied.

In a somewhat-related issue, when building a C sysroot,
cargo-hyperlight needs to build hyperlight-libc (as part of
hyperlight-guest-capi), and it turns out that that build was itself
getting the output include directory, which meant that rebuidls could
also result in stale include files being added. This commit changes
the logic involved in building hyperlight-guest-capi to ensure that
the output sysroot include directory (which is not yet populated)
isn't on the include path.

Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>

Author: syntactically

src/command.rs

@@ -64,7 +64,7 @@ impl Debug for Command {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let args = self.build_args_infallible();
         let mut cmd = self.command();
-        cmd.populate_from_args(&args);
+        cmd.populate_from_args(&args, false);
 
         write!(f, "env ")?;
         if let Some(current_dir) = &self.current_dir {
@@ -659,7 +659,7 @@ impl Command {
             .context("Failed to prepare sysroot")?;
 
         self.command()
-            .populate_from_args(&args)
+            .populate_from_args(&args, false)
             .checked_status()
             .context("Failed to execute cargo")?;
         Ok(())

src/lib.rs

@@ -86,11 +86,11 @@ impl Args {
 }
 
 trait CargoCommandExt {
-    fn populate_from_args(&mut self, args: &Args) -> &mut Self;
+    fn populate_from_args(&mut self, args: &Args, bootstrap: bool) -> &mut Self;
 }
 
 impl CargoCommandExt for std::process::Command {
-    fn populate_from_args(&mut self, args: &Args) -> &mut Self {
+    fn populate_from_args(&mut self, args: &Args, bootstrap: bool) -> &mut Self {
         self.target(&args.target);
         self.sysroot(args.sysroot_dir());
         self.append_rustflags("--cfg=hyperlight");
@@ -109,7 +109,7 @@ impl CargoCommandExt for std::process::Command {
         } else {
             // do nothing, let cc-rs find ar itself
         }
-        self.append_cflags(&args.target, toolchain::cflags(args).joined());
+        self.append_cflags(&args.target, toolchain::cflags(args, bootstrap).joined());
 
         self
     }

src/main.rs

@@ -1,5 +1,5 @@
-use std::env;
 use std::ffi::{OsStr, OsString};
+use std::{env, iter};
 
 use anyhow::{Result, anyhow};
 use cargo_hyperlight::{Args, cargo, toolchain, toolchain_flags};
@@ -30,7 +30,7 @@ impl FlagKind {
     fn get_flags(&self, args: &Args) -> toolchain_flags::Flags {
         use FlagKind::*;
         match self {
-            C => toolchain::cflags(args),
+            C => toolchain::cflags(args, false),
             Ld => toolchain::ldflags(args),
             Libs => toolchain::libs(args),
         }
@@ -73,14 +73,18 @@ impl SpecialCommand {
                     .as_ref()
                     .ok_or(anyhow!("Usage: cargo-hyperlight build-c-sysroot [opts] --c-sysroot-dir </path/to/sysroot>"))?;
                 built_args.prepare_sysroot()?;
-                util::copy_glob(&built_args.wrapper_dir(), &sysroot_dir.join("bin"), "**/*")?;
-                util::copy_glob(
-                    &built_args.includes_dir(),
+                util::union_glob(
+                    iter::once(&built_args.wrapper_dir()),
+                    &sysroot_dir.join("bin"),
+                    "**/*",
+                )?;
+                util::union_glob(
+                    iter::once(&built_args.includes_dir()),
                     &sysroot_dir.join("include"),
                     "**/*.h",
                 )?;
-                util::copy_glob_with_predicate(
-                    &built_args.c_libs_dir(),
+                util::union_glob_with_predicate(
+                    iter::once(&built_args.c_libs_dir()),
                     &sysroot_dir.join("lib"),
                     "**/*",
                     |x| !x.starts_with("rustlib"),

src/toolchain.rs

@@ -87,8 +87,8 @@ fn find_package_dirs(args: &Args) -> Result<PackageDirectories> {
     })
 }
 
-fn copy_includes(src_dir: &Path, dst_dir: &Path) -> Result<()> {
-    util::copy_glob(src_dir, dst_dir, "**/*.h")
+fn copy_includes(src_dirs: impl Iterator<Item: AsRef<Path>>, dst_dir: &Path) -> Result<()> {
+    util::union_glob(src_dirs, dst_dir, "**/*.h")
 }
 
 fn build_guest_capi(args: &Args, capi_dir: &Path) -> Result<()> {
@@ -101,7 +101,7 @@ fn build_guest_capi(args: &Args, capi_dir: &Path) -> Result<()> {
         .target_dir(args.build_dir())
         .arg("--message-format=json")
         .env_remove("RUSTC_WORKSPACE_WRAPPER")
-        .populate_from_args(args)
+        .populate_from_args(args, true)
         .output()
         .context("Failed to build capi cargo project")?;
     ensure!(
@@ -265,14 +265,20 @@ pub fn prepare(args: &Args) -> Result<()> {
         include_dirs.push("third_party/musl/arch/x86_64");
     }
 
-    for dir in include_dirs {
-        copy_includes(&libc_dir.join(dir), &include_dst_dir)?;
-    }
-    if args.with_guest_capi {
-        let capi_dir = package_dirs.guest_capi()?;
-        build_guest_capi(args, &capi_dir)?;
-        copy_includes(&capi_dir.join("include"), &include_dst_dir)?;
-    }
+    let capi_dir = args
+        .with_guest_capi
+        .then(|| {
+            let d = package_dirs.guest_capi()?;
+            build_guest_capi(args, &d)?;
+            Ok::<_, anyhow::Error>(d)
+        })
+        .transpose()?;
+
+    let include_dirs = include_dirs
+        .into_iter()
+        .map(|dir| libc_dir.join(dir))
+        .chain(capi_dir.map(|x| x.join("include")).into_iter());
+    copy_includes(include_dirs, &include_dst_dir)?;
 
     build_wrappers(args)?;
 
@@ -291,8 +297,8 @@ impl From<&Args> for toolchain_flags::Args {
     }
 }
 
-pub fn cflags(args: &Args) -> toolchain_flags::Flags {
-    toolchain_flags::cflags(&args.into())
+pub fn cflags(args: &Args, bootstrap: bool) -> toolchain_flags::Flags {
+    toolchain_flags::cflags(&args.into(), bootstrap)
 }
 pub fn ldflags(args: &Args) -> toolchain_flags::Flags {
     toolchain_flags::ldflags(&args.into())

src/toolchain_flags.rs

@@ -45,7 +45,7 @@ impl Flags {
     }
 }
 
-pub(crate) fn cflags(args: &Args) -> Flags {
+pub(crate) fn cflags(args: &Args, bootstrap: bool) -> Flags {
     let common_flags: &[&str] = &[
         "-U__linux__",
         "-fPIC",
@@ -74,8 +74,10 @@ pub(crate) fn cflags(args: &Args) -> Flags {
         flags.push(os("-march=armv8.1-a+fp+simd"));
     }
 
-    flags.push(os("-isystem"));
-    flags.push(Cow::Owned(args.includes_dir.clone().into()));
+    if !bootstrap {
+        flags.push(os("-isystem"));
+        flags.push(Cow::Owned(args.includes_dir.clone().into()));
+    }
     Flags(flags)
 }
 

src/util.rs

@@ -1,31 +1,62 @@
+use std::collections::HashSet;
 use std::fs;
 use std::path::Path;
 
 use anyhow::{Context, Result};
 
-pub(crate) fn copy_glob_with_predicate(
-    src_dir: &Path,
+pub(crate) fn union_glob_with_predicate(
+    src_dirs: impl Iterator<Item: AsRef<Path>>,
     dst_dir: &Path,
     glob: &str,
     predicate: impl Fn(&Path) -> bool,
 ) -> Result<()> {
-    let files = glob::glob(&format!("{}/{}", src_dir.display(), glob))
-        .context("Failed to read include source directory")?;
-    for file in files {
-        let src = file.context("Failed to read include source file")?;
-        let dst = src.strip_prefix(src_dir).unwrap();
-        if !predicate(dst) {
-            continue;
+    let mut copied = HashSet::new();
+
+    for src_dir in src_dirs {
+        let src_dir = src_dir.as_ref();
+        let files = glob::glob(&format!("{}/{}", src_dir.display(), glob))
+            .context("Failed to read source directory")?;
+        for file in files {
+            let src = file.context("Failed to read source file")?;
+            if !src.is_file() {
+                // contents will also show up
+                continue;
+            }
+            let suffix = src.strip_prefix(src_dir)?;
+            if !predicate(suffix) {
+                continue;
+            }
+            let dst = dst_dir.join(suffix);
+
+            let parent_dir = dst.parent().ok_or(anyhow::anyhow!(
+                "Could not get parent directory of destination file"
+            ))?;
+            fs::create_dir_all(parent_dir).context("Failed to create subdirectory")?;
+            fs::copy(&src, &dst).context("Failed to copy file")?;
+            copied.insert(dst);
         }
-        let dst = dst_dir.join(dst);
+    }
 
-        fs::create_dir_all(dst.parent().unwrap())
-            .context("Failed to create include subdirectory")?;
-        fs::copy(&src, &dst).context("Failed to copy include file")?;
+    // This doesn't do a good job of cleaning up empty directories,
+    // but that shouldn't cause any problems right now, since this is
+    // only used for header files/libraries/binaries.
+    let dst_files = glob::glob(&format!("{}/**/*", dst_dir.display()))
+        .context("Failed to read destination directory")?;
+    for dst_file in dst_files {
+        let dst = dst_file.context("Failed to read destination file")?;
+        if !copied.contains(&dst) {
+            if dst.is_file() {
+                fs::remove_file(dst).context("Failed to remove stale destination file")?;
+            }
+        }
     }
     Ok(())
 }
 
-pub(crate) fn copy_glob(src_dir: &Path, dst_dir: &Path, glob: &str) -> Result<()> {
-    copy_glob_with_predicate(src_dir, dst_dir, glob, |_| true)
+pub(crate) fn union_glob(
+    src_dir: impl Iterator<Item: AsRef<Path>>,
+    dst_dir: &Path,
+    glob: &str,
+) -> Result<()> {
+    union_glob_with_predicate(src_dir, dst_dir, glob, |_| true)
 }

src/wrapper/_main.rs

@@ -21,7 +21,7 @@ fn usage() -> ! {
 fn hyperlight_config(args: &Args, mut cmd: env::ArgsOs) {
     while let Some(flag) = cmd.next() {
         if flag == "--cflags" {
-            println!("{}", toolchain_flags::cflags(&args).joined().to_str().unwrap());
+            println!("{}", toolchain_flags::cflags(&args, false).joined().to_str().unwrap());
         } else if flag == "--ldflags" {
             println!("{}", toolchain_flags::ldflags(&args).joined().to_str().unwrap());
         } else if flag == "--libs" {
@@ -93,7 +93,7 @@ fn clang(args: &Args, tool_name: &str, cmd: env::ArgsOs) {
         }
     }
     let mut proc = std::process::Command::new(find_next(args, tool_name));
-    proc.args(toolchain_flags::cflags(&args).0);
+    proc.args(toolchain_flags::cflags(&args, false).0);
     if !found_non_link_flag && found_non_flag {
         proc.args(toolchain_flags::ldflags(&args).0);
     }