Skip to content

[hermes] Stop copying shadow crate; run in-place #3035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joshlf wants to merge 1 commit into
base: G4otukodorqa35fh3ms527yqmldc7g7ft
Choose a base branch
from
+10 −122
Open

[hermes] Stop copying shadow crate; run in-place #3035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tools/hermes/src/charon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub fn run_charon(args: &Args, roots: &Roots, packages: &[HermesArtifact]) -> Re
// Ensure cargo emits json msgs which charon-driver natively generates
cmd.arg("--message-format=json");

cmd.arg("--manifest-path").arg(&artifact.shadow_manifest_path);
cmd.arg("--manifest-path").arg(&artifact.manifest_path);

match artifact.target_kind {
HermesTargetKind::Lib
Expand Down Expand Up @@ -117,7 +117,7 @@ pub fn run_charon(args: &Args, roots: &Roots, packages: &[HermesArtifact]) -> Re
let reader = BufReader::new(stdout);

let mut mapper = crate::diagnostics::DiagnosticMapper::new(
artifact.shadow_manifest_path.parent().unwrap().to_path_buf(),
roots.workspace.clone(),
roots.workspace.clone(),
);

Expand Down
97 changes: 7 additions & 90 deletions tools/hermes/src/shadow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use std::{
};

use anyhow::{Context, Result};
use cargo_metadata::PackageName;
use dashmap::DashSet;
use walkdir::WalkDir;

Expand All @@ -19,8 +18,8 @@ use crate::{
pub struct HermesArtifact {
pub name: HermesTargetName,
pub target_kind: HermesTargetKind,
/// The path to the shadow crate's `Cargo.toml`.
pub shadow_manifest_path: PathBuf,
/// The path to the crate's `Cargo.toml`.
pub manifest_path: PathBuf,
pub start_from: Vec<String>,
}

Expand All @@ -37,10 +36,9 @@ impl HermesArtifact {
}

// Double-hash to make sure we can distinguish between e.g.
// (shadow_manifest_path, target_name) = ("abc", "def") and ("ab",
// "cdef"), which would hash identically if we just hashed their
// concatenation.
let h0 = hash(&self.shadow_manifest_path);
// (manifest_path, target_name) = ("abc", "def") and ("ab", "cdef"),
// which would hash identically if we just hashed their concatenation.
let h0 = hash(&self.manifest_path);
let h1 = hash(&self.name.target_name);
let h = hash(&[h0, h1]);
format!("{}-{}-{:08x}.llbc", self.name.package_name, self.name.target_name, h)
Expand All @@ -54,29 +52,12 @@ impl HermesArtifact {
/// 2. Creates symlinks for the remaining skeleton.
pub fn build_shadow_crate(roots: &Roots) -> Result<Vec<HermesArtifact>> {
log::trace!("build_shadow_crate({:?})", roots);
let shadow_root = roots.shadow_root();
if shadow_root.exists() {
fs::remove_dir_all(&shadow_root).context("Failed to clear shadow root")?;
}
fs::create_dir_all(&shadow_root).context("Failed to create shadow root")?;

let visited_paths = DashSet::new();
let (err_tx, err_rx) = mpsc::channel();
// Items are `((PackageName, TargetName), ItemPath)`.
let (path_tx, path_rx) = mpsc::channel::<(HermesTargetName, String)>();

let mut shadow_manifest_paths: HashMap<PackageName, PathBuf> = HashMap::new();
for target in &roots.roots {
if !shadow_manifest_paths.contains_key(&target.name.package_name) {
let relative_manifest_path = find_package_root(&target.src_path)?
.strip_prefix(&roots.workspace)
.context("Package root outside workspace")?
.join("Cargo.toml");
let shadow_manifest_path = shadow_root.join(&relative_manifest_path);
shadow_manifest_paths.insert(target.name.package_name.clone(), shadow_manifest_path);
}
}

let monitor_handle = std::thread::spawn(move || {
let mut error_count = 0;
for err in err_rx {
Expand Down Expand Up @@ -137,23 +118,17 @@ pub fn build_shadow_crate(roots: &Roots) -> Result<Vec<HermesArtifact>> {
return Err(anyhow::anyhow!("Aborting due to {} previous errors.", count));
}

let skip_paths: HashSet<PathBuf> = visited_paths.into_iter().collect();
create_symlink_skeleton(&roots.workspace, &shadow_root, &roots.cargo_target_dir, &skip_paths)?;

Ok(roots
.roots
.iter()
.filter_map(|target| {
// We know that every root has a shadow manifest path, because we
// inserted one for each package that has a root.
let shadow_manifest_path =
shadow_manifest_paths.get(&target.name.package_name).unwrap();
let manifest_path = find_package_root(&target.src_path).ok()?.join("Cargo.toml");
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since this function no longer builds a shadow crate, its name build_shadow_crate has become misleading. For better clarity and maintainability, consider renaming it to something like find_hermes_artifacts. This would also require updating its documentation and any call sites.

let start_from = entry_points.remove(&target.name)?;

Some(HermesArtifact {
name: target.name.clone(),
target_kind: target.kind,
shadow_manifest_path: shadow_manifest_path.clone(),
manifest_path,
start_from,
})
})
Expand Down Expand Up @@ -324,61 +299,3 @@ fn resolve_module_path(

None
}

fn create_symlink_skeleton(
source_root: &Path,
dest_root: &Path,
target_dir: &Path,
skip_paths: &HashSet<PathBuf>,
) -> Result<()> {
log::trace!("create_symlink_skeleton(source_root: {:?}, dest_root: {:?}, target_dir: {:?}, skip_paths_count: {})", source_root, dest_root, target_dir, skip_paths.len());
let walker = WalkDir::new(source_root)
.follow_links(false) // Security: don't follow symlinks out of the root.
.into_iter();

let filter = |e: &walkdir::DirEntry| {
let p = e.path();
// Normally, we expect the `dest_root` to be inside of `target_dir`,
// so checking both is redundant. However, if we ever add the option
// for the user to manually specify a `dest_root` that is outside of
// `target_dir`, this check will prevent us from infinitely recursing
// into the source root.
p != target_dir && p != dest_root && e.file_name() != ".git"
};

for entry in walker.filter_entry(filter) {
let entry = entry.context("Failed to read directory entry")?;
let src_path = entry.path();

let relative_path =
src_path.strip_prefix(source_root).context("File is not inside source root")?;
let dest_path = dest_root.join(relative_path);

if entry.file_type().is_dir() {
fs::create_dir_all(&dest_path)
.with_context(|| format!("Failed to create shadow directory: {:?}", dest_path))?;
continue;
}

if entry.file_type().is_file() && !skip_paths.contains(src_path) {
make_symlink(src_path, &dest_path)?;
}
}

Ok(())
}

#[cfg(unix)]
fn make_symlink(original: &Path, link: &Path) -> Result<()> {
std::os::unix::fs::symlink(original, link)
.with_context(|| format!("Failed to symlink {:?} -> {:?}", original, link))
}

#[cfg(windows)]
fn make_symlink(original: &Path, link: &Path) -> Result<()> {
// Windows treats file and directory symlinks differently.
// Since we only call this on files (checking is_file above),
// we use symlink_file.
std::os::windows::fs::symlink_file(original, link)
.with_context(|| format!("Failed to symlink {:?} -> {:?}", original, link))
}
31 changes: 1 addition & 30 deletions tools/hermes/tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,9 @@ exec "{1}" "$@"
let mock_json_file = test_case_root.join("mock_charon_output.json");
if mock_json_file.exists() {
// Instead of writing the mock json to the shadow root (which gets cleared by build_shadow_crate), write it to the test workspace root!
let shadow_root =
sandbox_root.join("target").join("hermes").join("hermes_test_target").join("shadow");
// We still need the path for mapping `[SHADOW_ROOT]` correctly!
// But we construct it manually since it might not be created yet:
let abs_shadow_root = std::env::current_dir().unwrap().join(&shadow_root);
let abs_shadow_root = std::env::current_dir().unwrap().join(&sandbox_root);
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable abs_shadow_root is now misleading since shadow crates are no longer used. It actually holds the absolute path to the sandbox root. Renaming it to abs_sandbox_root would make the code easier to understand. You'll need to update its usage below as well.

Suggested change
let abs_shadow_root = std::env::current_dir().unwrap().join(&sandbox_root);
let abs_sandbox_root = std::env::current_dir().unwrap().join(&sandbox_root);

let abs_test_case_root =
test_case_root.canonicalize().unwrap_or_else(|_| test_case_root.to_path_buf());

Expand Down Expand Up @@ -187,13 +185,6 @@ exec "{1}" "$@"
}
}

// Tests can specify the expected shadow crate content.
let actual_shadow = sandbox_root.join("target/hermes/hermes_test_target/shadow");
let expected_shadow = test_case_root.join("expected");
if expected_shadow.exists() {
assert_directories_match(&expected_shadow, &actual_shadow)?;
}

// Load Config
let mut config = TestConfig { artifact: vec![], command: vec![] };
let config_file = test_case_root.join("expected_config.toml");
Expand Down Expand Up @@ -324,26 +315,6 @@ fn assert_artifacts_match(
Ok(())
}

fn assert_directories_match(expected: &Path, actual: &Path) -> std::io::Result<()> {
for entry in WalkDir::new(expected) {
let entry = entry?;
if !entry.file_type().is_file() {
continue;
}
let rel = entry.path().strip_prefix(expected).unwrap();
let act = actual.join(rel);
if !act.exists() {
panic!("Missing file {:?}", rel);
}
let e_txt = fs::read_to_string(entry.path())?.replace("\r\n", "\n");
let a_txt = fs::read_to_string(&act)?.replace("\r\n", "\n");
if e_txt != a_txt {
panic!("Mismatch in {:?}", rel);
}
}
Ok(())
}

fn copy_dir_contents(src: &Path, dst: &Path) -> std::io::Result<()> {
fs::create_dir_all(dst)?;
for entry in fs::read_dir(src)? {
Expand Down
Loading