[hermes] Resolve Cargo and Hermes target dirs #3007
Conversation
Summary of ChangesHello @joshlf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors how Hermes resolves and manages critical build directories. It introduces explicit tracking for both the Cargo target directory and a uniquely generated shadow root, enhancing build isolation and preventing potential conflicts across different workspaces. Furthermore, it ensures that the shadow directory is always created in a clean state, improving build reliability and consistency. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to resolve Cargo's target directory and create a unique, hashed shadow directory for Hermes. This is a good approach to avoid collisions between different workspaces. The change to clean the shadow directory before creating it is also a solid improvement for ensuring a clean state. My primary feedback relates to the choice of hashing algorithm, which could be improved for better cross-platform and cross-version stability.
| let workspace_root_hash = { | ||
| let mut hasher = std::hash::DefaultHasher::new(); | ||
| hasher.write(b"hermes_shadow_salt"); | ||
| metadata.workspace_root.hash(&mut hasher); | ||
| hasher.finish() | ||
| }; |
There was a problem hiding this comment.
std::hash::DefaultHasher is not guaranteed to be stable across different Rust versions, or even between 32-bit and 64-bit platforms. This means the generated shadow path could change unexpectedly, which could cause problems with build caching and reproducibility.
For a build tool where deterministic output is important, it's better to use a stable hashing algorithm. A simple and fast option is FnvHasher from the fnv crate.
Here is an example of how you could use it:
- Add
fnvtotools/hermes/Cargo.toml:
[dependencies]
# ...
fnv = "1.0"- Update the code to use
FnvHasher:
// Add to imports: use fnv::FnvHasher;
let workspace_root_hash = {
let mut hasher = FnvHasher::default();
hasher.write(b"hermes_shadow_salt");
metadata.workspace_root.hash(&mut hasher);
hasher.finish()
};
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef #3007 +/- ##
============================================================================
Coverage ? 91.87%
============================================================================
Files ? 20
Lines ? 6057
Branches ? 0
============================================================================
Hits ? 5565
Misses ? 492
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces logic to resolve paths for Cargo's target directory and a new "shadow" directory for Hermes. The changes are generally in the right direction, but I've identified a couple of issues. In resolve.rs, the use of DefaultHasher could lead to path instability across different Rust versions, which might clutter the target directory over time. More critically, the directory cleanup logic in shadow.rs is not robust and can cause the tool to fail under certain filesystem conditions, such as when the destination path is a file or a dangling symlink. I've provided detailed feedback and a suggested fix for this critical issue.
| if dest_root.exists() { | ||
| std::fs::remove_dir_all(&dest_root)?; | ||
| } | ||
| std::fs::create_dir_all(&dest_root)?; |
There was a problem hiding this comment.
The current logic for cleaning the destination directory has a couple of race conditions and bugs that can cause the tool to fail unexpectedly:
- If
dest_rootis a dangling symlink,dest_root.exists()returnsfalse. The code then proceeds tostd::fs::create_dir_all, which will fail because the path is already occupied by the symlink. - If
dest_rootis a file (not a directory),dest_root.exists()returnstrue, butstd::fs::remove_dir_allwill fail on Unix-like systems, causing an unhandled error.
A more robust approach is needed to handle all these filesystem states correctly.
| if dest_root.exists() { | |
| std::fs::remove_dir_all(&dest_root)?; | |
| } | |
| std::fs::create_dir_all(&dest_root)?; | |
| if dest_root.symlink_metadata().is_ok() { | |
| // The path exists. To ensure a clean state, we remove it. | |
| // This handles files, directories, and symlinks (including dangling ones). | |
| std::fs::remove_dir_all(dest_root).or_else(|_| std::fs::remove_file(dest_root))?; | |
| } | |
| std::fs::create_dir_all(dest_root)?; |
| // Hash the path to the workspace root to avoid collisions between different | ||
| // workspaces using the same target directory. | ||
| let workspace_root_hash = { | ||
| let mut hasher = std::hash::DefaultHasher::new(); |
There was a problem hiding this comment.
std::hash::DefaultHasher is not guaranteed to produce the same hash across different Rust versions or platforms. This means that if the Rust toolchain is updated, the generated hash for the workspace path might change. This would result in a new hermes_shadow_* directory being created in your target directory, leaving the old one as an orphaned artifact.
While this is not a correctness issue for a single run (as the shadow directory is cleaned), it can lead to clutter in the build output directory over time. If a stable path is desirable, consider using a stable hashing algorithm. You could use a hasher from a crate like seahash or ahash with a fixed seed, or a cryptographic hash like SHA-2.
gherrit-pr-id: G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a
joshlf
force-pushed
the
Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef
branch
from
169b4eb to
c475f79
Compare
joshlf
force-pushed
the
G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a
branch
from
7ee6fde to
1f72412
Compare
2 participants
transform::apply_edits#3025mod foo;declarations #3008Latest Update: v30 — Compare vs v29
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a && git checkout -b pr-G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a FETCH_HEADCheckout
git fetch origin refs/heads/G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/G48b27aa33306f1b4bae6f7996a68c1a1869d0a9a && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.