feat(packages): .lnk resource pointers — fetch + verify + cache + materialize#119
Merged
feat(packages): .lnk resource pointers — fetch + verify + cache + materialize#119
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
actor CLI as Fbuild CLI
participant Scan as Scanner
participant Resolver as Resolver
participant Cache as DiskCache
participant HTTP as Remote Server
participant Mat as Materializer
participant Orch as Orchestrator
CLI->>Orch: start build / lnk pull/check
Orch->>Scan: scan_for_lnk(source_root)
Scan-->>Orch: DiscoveredLnk[]
Orch->>Resolver: resolve(lnk, cache)
Resolver->>Cache: lookup(Kind::LnkBlobs, url, sha256)
alt cache hit
Cache-->>Resolver: cached entry & path
Resolver->>Resolver: verify sha256
Resolver-->>Orch: ResolvedBlob (with lease)
else cache miss
Resolver->>HTTP: GET url
HTTP-->>Resolver: blob bytes
Resolver->>Resolver: verify sha256
Resolver->>Cache: record_archive(...)
Cache-->>Resolver: entry & path
Resolver-->>Orch: ResolvedBlob (with lease)
end
Orch->>Mat: materialize_all(discovered, build_resources_dir, cache)
Mat->>Resolver: (may reuse resolve results)
Mat->>Mat: extract/copy into build_resources_dir
Mat-->>Orch: MaterializedLnk[] (paths)
Orch->>CLI: return success / report failures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/fbuild-build/src/esp32/orchestrator.rs (2)
46-49:⚠️ Potential issue | 🟠 MajorInclude
.lnkmanifests in the fast-path fingerprint.A change to a
.lnkfile’s URL or SHA can affect embedded build inputs, but.lnkis not inFAST_PATH_EXTENSIONS, so the no-op path can incorrectly reuse stale artifacts.Proposed fix
const FAST_PATH_EXTENSIONS: &[&str] = &[ - "a", "bin", "c", "cc", "cpp", "csv", "elf", "h", "hh", "hpp", "ino", "json", "ld", "lds", "py", - "s", "S", "txt", + "a", "bin", "c", "cc", "cpp", "csv", "elf", "h", "hh", "hpp", "ino", "json", "ld", "lds", + "lnk", "py", "s", "S", "txt", ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/esp32/orchestrator.rs` around lines 46 - 49, FAST_PATH_EXTENSIONS currently omits "lnk", so changes to .lnk manifests can be ignored by the fast-path fingerprint; update the FAST_PATH_EXTENSIONS constant in orchestrator.rs to include "lnk" (add "lnk" to the array of extension strings) so .lnk files participate in the fast-path fingerprint logic used by the functions referencing FAST_PATH_EXTENSIONS.
1070-1093:⚠️ Potential issue | 🟠 MajorPreserve logical embed paths for
.lnkentries to avoid symbol name mismatches.Resolving
.lnkfiles to absolute materialized paths before passing them toprocess_embed_filescausesobjcopyto generate_binary_*symbols based on the mangled absolute path rather than the original logical path (e.g.,"config/data.lnk"stripped to"config/data"). The text files workaround recreates the relative path structure inembed_dir, but fails when the input is an absolute path from a.lnkmaterialization.This will cause linker failures if application code references symbols expecting the original logical path. Carry both the materialized source path and the original logical embed path (with
.lnkstripped) throughprocess_embed_filesto generate correct symbol names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/esp32/orchestrator.rs` around lines 1070 - 1093, The current expand() replaces .lnk entries with their materialized absolute paths via resolve_lnk(&p), which causes objcopy to use the absolute path for _binary_* symbol names; change expand (and the data passed to process_embed_files) so that for any entry where fbuild_packages::lnk::has_lnk_extension(&p) is true you produce and pass a pair (materialized_source_path, original_logical_path_without_.lnk) instead of a single path — i.e., keep both resolved path (from resolve_lnk) and the logical embed path (p with the .lnk suffix stripped) and update the call sites (resolved_embed_files, resolved_embed_txtfiles and process_embed_files) to accept and use these tuples so objcopy receives the correct logical path for symbol name generation while using the materialized file as the input.
🧹 Nitpick comments (3)
crates/fbuild-packages/src/lnk/materialize.rs (1)
198-220:place_filecan't recover from an extract-mode transition.
Zip/TarGzbranches (lines 77-94 / 104-121) correctly handle both file and directory attarget_path, butplace_fileonly callsremove_file. If a previous build materializedfoo.zipas a directory (viaExtractMode::Zip) and the.lnkis later flipped toExtractMode::File, the remove will fail with "is a directory" and the build errors out on stale state rather than healing.Worth matching the symmetric cleanup from the extract branches:
🔧 Suggested change
fn place_file(src: &Path, dst: &Path) -> Result<()> { - // Remove any existing target — replacing keeps semantics deterministic - // (a stale leftover from a prior build won't shadow the new blob). - if dst.exists() || dst.symlink_metadata().is_ok() { - std::fs::remove_file(dst).map_err(|e| { - FbuildError::PackageError(format!( - "failed to remove existing target {}: {e}", - dst.display() - )) - })?; - } + // Remove any existing target — replacing keeps semantics deterministic. + if let Ok(meta) = std::fs::symlink_metadata(dst) { + let r = if meta.is_dir() { + std::fs::remove_dir_all(dst) + } else { + std::fs::remove_file(dst) + }; + r.map_err(|e| { + FbuildError::PackageError(format!( + "failed to remove existing target {}: {e}", + dst.display() + )) + })?; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-packages/src/lnk/materialize.rs` around lines 198 - 220, place_file fails to heal when a previous materialization left a directory (from ExtractMode::Zip/TarGz) at dst because it only calls remove_file; change the cleanup to inspect dst.symlink_metadata()/dst.exists() and if metadata.is_dir() (or dst.is_dir()) call std::fs::remove_dir_all(dst) (handling errors into FbuildError::PackageError with a clear message referencing dst.display()), otherwise call std::fs::remove_file(dst) as before; preserve the subsequent hard_link then copy logic and error mapping so place_file recovers from extract→file transitions.crates/fbuild-cli/src/main.rs (2)
3105-3122: Manual JSON construction dropsextractand risks drift fromLnkFile.You build an
LnkFileon line 3105 but then discard it and hand-roll the JSON viajson!({...})without anextractfield. This works today because the parser defaults toFile, but:
- If
ExtractModegains a required field or the default changes,add-produced.lnks silently diverge from the parser's contract.- The manual path skips any
#[serde(...)]attributes onLnkFile(e.g. field renames).Simplest fix: serialize
&lnkdirectly. If you want to elide defaults, add#[serde(skip_serializing_if = "...")]onLnkFileinstead of reinventing the shape here.- let json = serde_json::json!({ - "v": lnk.version, - "url": lnk.url, - "sha256": lnk.sha256, - "size": lnk.size, - }); - let pretty = serde_json::to_string_pretty(&json).map_err(|e| { + let pretty = serde_json::to_string_pretty(&lnk).map_err(|e| { fbuild_core::FbuildError::PackageError(format!( "failed to serialize .lnk JSON: {e}" )) })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-cli/src/main.rs` around lines 3105 - 3122, The code constructs an LnkFile (LnkFile, ExtractMode) then manually builds JSON via serde_json::json!({...}) which omits the extract field and bypasses LnkFile's serde attributes; replace the manual json! block and call serde_json::to_string_pretty(&lnk) to serialize the constructed LnkFile directly so all fields and serde annotations are respected, and if you need to elide defaults instead add appropriate #[serde(skip_serializing_if = "...")] attributes on LnkFile rather than hand-omitting fields.
3036-3056:checkreads entire blob into memory and does byte-exact sha comparison.Two small concerns in the verify path:
std::fs::read(&blob_path)buffers the whole blob (potentially hundreds of MB per README) into memory just to hash it. Streaming the hash (std::io::copyintoSha256) keeps memory bounded and matches what a real verify tool should do.actual == d.lnk.sha256is case-sensitive, whileresolver::verify_sha256compares againstexpected.to_ascii_lowercase(). Ifformat::LnkFilevalidation ever accepts mixed-case hex (today it doesn't, but it's not enforced here),checkwill diverge fromresolve. Consideractual.eq_ignore_ascii_case(&d.lnk.sha256)for consistency.🔧 Streaming-hash variant
- let bytes = std::fs::read(&blob_path).map_err(|e| { - fbuild_core::FbuildError::PackageError(format!( - "failed to read {}: {e}", - blob_path.display() - )) - })?; - let mut h = Sha256::new(); - h.update(&bytes); - let actual = format!("{:x}", h.finalize()); - if actual == d.lnk.sha256 { + let mut f = std::fs::File::open(&blob_path).map_err(|e| { + fbuild_core::FbuildError::PackageError(format!( + "failed to open {}: {e}", + blob_path.display() + )) + })?; + let mut h = Sha256::new(); + std::io::copy(&mut f, &mut h).map_err(|e| { + fbuild_core::FbuildError::PackageError(format!( + "failed to read {}: {e}", + blob_path.display() + )) + })?; + let actual = format!("{:x}", h.finalize()); + if actual.eq_ignore_ascii_case(&d.lnk.sha256) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-cli/src/main.rs` around lines 3036 - 3056, The verify code currently reads the whole blob with std::fs::read and does a case-sensitive compare; change it to stream the file and do a case-insensitive hex compare: open the file with std::fs::File::open(&blob_path), read it in a small buffer loop calling h.update(&buf[..n]) on the Sha256 instance (instead of std::fs::read), finalize as before into actual, and compare using actual.eq_ignore_ascii_case(&d.lnk.sha256) (or normalize with to_ascii_lowercase) so the behavior matches resolver::verify_sha256; update the branches that increment ok/mismatched and print messages to use the new actual and comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fbuild-build/src/esp32/orchestrator.rs`:
- Around line 1046-1080: The current resolve_lnk drops the MaterializedLnk
immediately (only cloning target_path) so the RAII lease is never stored; fix by
keeping each lease in the _lnk_leases Vec: either (preferred) remove the
resolve_lnk shortcut and in expand's loop call
fbuild_packages::lnk::materialize_lnk_entry directly, push the returned
MaterializedLnk into _lnk_leases before using its .target_path, or change
resolve_lnk to accept &mut _lnk_leases (or return the MaterializedLnk and push
it into _lnk_leases at call sites) so the lease objects returned by
materialize_lnk_entry are stored in _lnk_leases and not dropped immediately
(update calls in expand and resolved_embed_* accordingly).
In `@crates/fbuild-cli/src/main.rs`:
- Around line 3068-3085: The basename extraction in the LnkAction::Add handler
is brittle: replace the naive url.rsplit('/') logic in the LnkAction::Add block
with proper URL path-segment parsing to avoid empty segments and strip
query/fragment data. Parse the URL with url::Url::parse(url) and use
url.path_segments().map(|s| s.filter(|seg|
!seg.is_empty()).last()).flatten().map(|s| s.to_string()).unwrap_or_else(||
"blob".into()) to get the basename (this removes trailing-slash empties and
excludes query/fragment), then use that basename to build output_path; ensure
you handle Url::parse errors by falling back to the above fallback or returning
the existing PackageError. Use the variables and symbols in this block (url,
basename, output_path, LnkAction::Add) to locate and update the code.
In `@crates/fbuild-packages/src/lnk/scanner.rs`:
- Line 40: Change the WalkDir usage to enable symlink traversal and stop
silently swallowing IO errors: replace WalkDir::new(root) with
WalkDir::new(root).follow_links(true) and replace the
`.into_iter().filter_map(|e| e.ok())` pattern with explicit error handling
(e.g., `.into_iter().map(|r| r.map_err(|e| { /* log or propagate e with context
*/ e })).filter_map(Result::ok)` or use a match inside the iterator to log
warnings for Err variants) so `.lnk` files under symlinked directories are
visited and filesystem errors are surfaced or logged; update the loop that
currently reads `for entry in WalkDir::new(root).into_iter().filter_map(|e|
e.ok())` accordingly.
---
Outside diff comments:
In `@crates/fbuild-build/src/esp32/orchestrator.rs`:
- Around line 46-49: FAST_PATH_EXTENSIONS currently omits "lnk", so changes to
.lnk manifests can be ignored by the fast-path fingerprint; update the
FAST_PATH_EXTENSIONS constant in orchestrator.rs to include "lnk" (add "lnk" to
the array of extension strings) so .lnk files participate in the fast-path
fingerprint logic used by the functions referencing FAST_PATH_EXTENSIONS.
- Around line 1070-1093: The current expand() replaces .lnk entries with their
materialized absolute paths via resolve_lnk(&p), which causes objcopy to use the
absolute path for _binary_* symbol names; change expand (and the data passed to
process_embed_files) so that for any entry where
fbuild_packages::lnk::has_lnk_extension(&p) is true you produce and pass a pair
(materialized_source_path, original_logical_path_without_.lnk) instead of a
single path — i.e., keep both resolved path (from resolve_lnk) and the logical
embed path (p with the .lnk suffix stripped) and update the call sites
(resolved_embed_files, resolved_embed_txtfiles and process_embed_files) to
accept and use these tuples so objcopy receives the correct logical path for
symbol name generation while using the materialized file as the input.
---
Nitpick comments:
In `@crates/fbuild-cli/src/main.rs`:
- Around line 3105-3122: The code constructs an LnkFile (LnkFile, ExtractMode)
then manually builds JSON via serde_json::json!({...}) which omits the extract
field and bypasses LnkFile's serde attributes; replace the manual json! block
and call serde_json::to_string_pretty(&lnk) to serialize the constructed LnkFile
directly so all fields and serde annotations are respected, and if you need to
elide defaults instead add appropriate #[serde(skip_serializing_if = "...")]
attributes on LnkFile rather than hand-omitting fields.
- Around line 3036-3056: The verify code currently reads the whole blob with
std::fs::read and does a case-sensitive compare; change it to stream the file
and do a case-insensitive hex compare: open the file with
std::fs::File::open(&blob_path), read it in a small buffer loop calling
h.update(&buf[..n]) on the Sha256 instance (instead of std::fs::read), finalize
as before into actual, and compare using
actual.eq_ignore_ascii_case(&d.lnk.sha256) (or normalize with
to_ascii_lowercase) so the behavior matches resolver::verify_sha256; update the
branches that increment ok/mismatched and print messages to use the new actual
and comparison.
In `@crates/fbuild-packages/src/lnk/materialize.rs`:
- Around line 198-220: place_file fails to heal when a previous materialization
left a directory (from ExtractMode::Zip/TarGz) at dst because it only calls
remove_file; change the cleanup to inspect dst.symlink_metadata()/dst.exists()
and if metadata.is_dir() (or dst.is_dir()) call std::fs::remove_dir_all(dst)
(handling errors into FbuildError::PackageError with a clear message referencing
dst.display()), otherwise call std::fs::remove_file(dst) as before; preserve the
subsequent hard_link then copy logic and error mapping so place_file recovers
from extract→file transitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4a488f0-1627-4d8f-97ed-bd27435f83ce
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/fbuild-build/src/esp32/orchestrator.rscrates/fbuild-cli/Cargo.tomlcrates/fbuild-cli/src/main.rscrates/fbuild-packages/Cargo.tomlcrates/fbuild-packages/src/disk_cache/paths.rscrates/fbuild-packages/src/extractor.rscrates/fbuild-packages/src/lib.rscrates/fbuild-packages/src/lnk/README.mdcrates/fbuild-packages/src/lnk/embed.rscrates/fbuild-packages/src/lnk/format.rscrates/fbuild-packages/src/lnk/materialize.rscrates/fbuild-packages/src/lnk/mod.rscrates/fbuild-packages/src/lnk/resolver.rscrates/fbuild-packages/src/lnk/scanner.rscrates/fbuild-packages/tests/lnk_e2e.rs
Comment on lines
+1046
to
+1080
| let lnk_dir = embed_dir.join("lnk"); | ||
| let mut _lnk_leases: Vec<fbuild_packages::lnk::MaterializedLnk> = Vec::new(); | ||
| let lnk_cache = fbuild_packages::DiskCache::open().ok(); | ||
|
|
||
| let resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> { | ||
| let cache = lnk_cache.as_ref().ok_or_else(|| { | ||
| fbuild_core::FbuildError::PackageError( | ||
| "disk cache unavailable; cannot resolve .lnk entries".to_string(), | ||
| ) | ||
| })?; | ||
| let m = fbuild_packages::lnk::materialize_lnk_entry(lnk_path, &lnk_dir, cache)?; | ||
| Ok(m.target_path.clone()) | ||
| }; | ||
| // Closures can't borrow `_lnk_leases` mutably while also being | ||
| // FnMut for both expansions, so we collect leases inline by | ||
| // calling `materialize_lnk_entry` directly inside a small loop. | ||
| let expand = |entries: &[String]| -> Result<Vec<String>> { | ||
| let mut out = Vec::with_capacity(entries.len()); | ||
| for entry in entries { | ||
| let p = if Path::new(entry).is_absolute() { | ||
| std::path::PathBuf::from(entry) | ||
| } else { | ||
| params.project_dir.join(entry) | ||
| }; | ||
| if fbuild_packages::lnk::has_lnk_extension(&p) { | ||
| let resolved = resolve_lnk(&p)?; | ||
| out.push(resolved.to_string_lossy().into_owned()); | ||
| } else { | ||
| out.push(entry.clone()); | ||
| } | ||
| } | ||
| Ok(out) | ||
| }; | ||
| let resolved_embed_files = expand(&embed_files)?; | ||
| let resolved_embed_txtfiles = expand(&embed_txtfiles)?; |
There was a problem hiding this comment.
Keep the MaterializedLnk leases alive as intended.
resolve_lnk drops m immediately after cloning target_path, so _lnk_leases never holds the RAII lease described in the comment.
Proposed fix
let lnk_dir = embed_dir.join("lnk");
let mut _lnk_leases: Vec<fbuild_packages::lnk::MaterializedLnk> = Vec::new();
let lnk_cache = fbuild_packages::DiskCache::open().ok();
- let resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> {
+ let mut resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> {
let cache = lnk_cache.as_ref().ok_or_else(|| {
fbuild_core::FbuildError::PackageError(
"disk cache unavailable; cannot resolve .lnk entries".to_string(),
)
})?;
let m = fbuild_packages::lnk::materialize_lnk_entry(lnk_path, &lnk_dir, cache)?;
- Ok(m.target_path.clone())
+ let target_path = m.target_path.clone();
+ _lnk_leases.push(m);
+ Ok(target_path)
};
- // Closures can't borrow `_lnk_leases` mutably while also being
- // FnMut for both expansions, so we collect leases inline by
- // calling `materialize_lnk_entry` directly inside a small loop.
- let expand = |entries: &[String]| -> Result<Vec<String>> {
+ let mut expand = |entries: &[String]| -> Result<Vec<String>> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let lnk_dir = embed_dir.join("lnk"); | |
| let mut _lnk_leases: Vec<fbuild_packages::lnk::MaterializedLnk> = Vec::new(); | |
| let lnk_cache = fbuild_packages::DiskCache::open().ok(); | |
| let resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> { | |
| let cache = lnk_cache.as_ref().ok_or_else(|| { | |
| fbuild_core::FbuildError::PackageError( | |
| "disk cache unavailable; cannot resolve .lnk entries".to_string(), | |
| ) | |
| })?; | |
| let m = fbuild_packages::lnk::materialize_lnk_entry(lnk_path, &lnk_dir, cache)?; | |
| Ok(m.target_path.clone()) | |
| }; | |
| // Closures can't borrow `_lnk_leases` mutably while also being | |
| // FnMut for both expansions, so we collect leases inline by | |
| // calling `materialize_lnk_entry` directly inside a small loop. | |
| let expand = |entries: &[String]| -> Result<Vec<String>> { | |
| let mut out = Vec::with_capacity(entries.len()); | |
| for entry in entries { | |
| let p = if Path::new(entry).is_absolute() { | |
| std::path::PathBuf::from(entry) | |
| } else { | |
| params.project_dir.join(entry) | |
| }; | |
| if fbuild_packages::lnk::has_lnk_extension(&p) { | |
| let resolved = resolve_lnk(&p)?; | |
| out.push(resolved.to_string_lossy().into_owned()); | |
| } else { | |
| out.push(entry.clone()); | |
| } | |
| } | |
| Ok(out) | |
| }; | |
| let resolved_embed_files = expand(&embed_files)?; | |
| let resolved_embed_txtfiles = expand(&embed_txtfiles)?; | |
| let lnk_dir = embed_dir.join("lnk"); | |
| let mut _lnk_leases: Vec<fbuild_packages::lnk::MaterializedLnk> = Vec::new(); | |
| let lnk_cache = fbuild_packages::DiskCache::open().ok(); | |
| let mut resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> { | |
| let cache = lnk_cache.as_ref().ok_or_else(|| { | |
| fbuild_core::FbuildError::PackageError( | |
| "disk cache unavailable; cannot resolve .lnk entries".to_string(), | |
| ) | |
| })?; | |
| let m = fbuild_packages::lnk::materialize_lnk_entry(lnk_path, &lnk_dir, cache)?; | |
| let target_path = m.target_path.clone(); | |
| _lnk_leases.push(m); | |
| Ok(target_path) | |
| }; | |
| let mut expand = |entries: &[String]| -> Result<Vec<String>> { | |
| let mut out = Vec::with_capacity(entries.len()); | |
| for entry in entries { | |
| let p = if Path::new(entry).is_absolute() { | |
| std::path::PathBuf::from(entry) | |
| } else { | |
| params.project_dir.join(entry) | |
| }; | |
| if fbuild_packages::lnk::has_lnk_extension(&p) { | |
| let resolved = resolve_lnk(&p)?; | |
| out.push(resolved.to_string_lossy().into_owned()); | |
| } else { | |
| out.push(entry.clone()); | |
| } | |
| } | |
| Ok(out) | |
| }; | |
| let resolved_embed_files = expand(&embed_files)?; | |
| let resolved_embed_txtfiles = expand(&embed_txtfiles)?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-build/src/esp32/orchestrator.rs` around lines 1046 - 1080, The
current resolve_lnk drops the MaterializedLnk immediately (only cloning
target_path) so the RAII lease is never stored; fix by keeping each lease in the
_lnk_leases Vec: either (preferred) remove the resolve_lnk shortcut and in
expand's loop call fbuild_packages::lnk::materialize_lnk_entry directly, push
the returned MaterializedLnk into _lnk_leases before using its .target_path, or
change resolve_lnk to accept &mut _lnk_leases (or return the MaterializedLnk and
push it into _lnk_leases at call sites) so the lease objects returned by
materialize_lnk_entry are stored in _lnk_leases and not dropped immediately
(update calls in expand and resolved_embed_* accordingly).
Comment on lines
+3068
to
+3085
| LnkAction::Add { url, output } => { | ||
| // Determine output path before downloading so we fail early on a | ||
| // bad output spec. | ||
| let basename = url.rsplit('/').next().unwrap_or("blob"); | ||
| let output_path = match output { | ||
| Some(p) => PathBuf::from(p), | ||
| None => PathBuf::from(format!("{basename}.lnk")), | ||
| }; | ||
| if let Some(parent) = output_path.parent() { | ||
| if !parent.as_os_str().is_empty() { | ||
| std::fs::create_dir_all(parent).map_err(|e| { | ||
| fbuild_core::FbuildError::PackageError(format!( | ||
| "failed to create {}: {e}", | ||
| parent.display() | ||
| )) | ||
| })?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add basename parsing breaks for common URL shapes.
url.rsplit('/').next().unwrap_or("blob") returns Some("") when url ends with /, so unwrap_or("blob") never kicks in and you end up writing .lnk to the cwd. It also happily includes query/fragment strings — e.g. https://example.com/asset.bin?token=xyz produces asset.bin?token=xyz.lnk, which is not a valid filename on Windows.
🔧 Proposed fix
- let basename = url.rsplit('/').next().unwrap_or("blob");
+ // Strip query/fragment, then take the final path segment.
+ let clean = url.split(['?', '#']).next().unwrap_or(&url);
+ let basename = clean
+ .rsplit('/')
+ .find(|s| !s.is_empty())
+ .unwrap_or("blob");
let output_path = match output {
Some(p) => PathBuf::from(p),
None => PathBuf::from(format!("{basename}.lnk")),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| LnkAction::Add { url, output } => { | |
| // Determine output path before downloading so we fail early on a | |
| // bad output spec. | |
| let basename = url.rsplit('/').next().unwrap_or("blob"); | |
| let output_path = match output { | |
| Some(p) => PathBuf::from(p), | |
| None => PathBuf::from(format!("{basename}.lnk")), | |
| }; | |
| if let Some(parent) = output_path.parent() { | |
| if !parent.as_os_str().is_empty() { | |
| std::fs::create_dir_all(parent).map_err(|e| { | |
| fbuild_core::FbuildError::PackageError(format!( | |
| "failed to create {}: {e}", | |
| parent.display() | |
| )) | |
| })?; | |
| } | |
| } | |
| LnkAction::Add { url, output } => { | |
| // Determine output path before downloading so we fail early on a | |
| // bad output spec. | |
| // Strip query/fragment, then take the final path segment. | |
| let clean = url.split(['?', '#']).next().unwrap_or(&url); | |
| let basename = clean | |
| .rsplit('/') | |
| .find(|s| !s.is_empty()) | |
| .unwrap_or("blob"); | |
| let output_path = match output { | |
| Some(p) => PathBuf::from(p), | |
| None => PathBuf::from(format!("{basename}.lnk")), | |
| }; | |
| if let Some(parent) = output_path.parent() { | |
| if !parent.as_os_str().is_empty() { | |
| std::fs::create_dir_all(parent).map_err(|e| { | |
| fbuild_core::FbuildError::PackageError(format!( | |
| "failed to create {}: {e}", | |
| parent.display() | |
| )) | |
| })?; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-cli/src/main.rs` around lines 3068 - 3085, The basename
extraction in the LnkAction::Add handler is brittle: replace the naive
url.rsplit('/') logic in the LnkAction::Add block with proper URL path-segment
parsing to avoid empty segments and strip query/fragment data. Parse the URL
with url::Url::parse(url) and use url.path_segments().map(|s| s.filter(|seg|
!seg.is_empty()).last()).flatten().map(|s| s.to_string()).unwrap_or_else(||
"blob".into()) to get the basename (this removes trailing-slash empties and
excludes query/fragment), then use that basename to build output_path; ensure
you handle Url::parse errors by falling back to the above fallback or returning
the existing PackageError. Use the variables and symbols in this block (url,
basename, output_path, LnkAction::Add) to locate and update the code.
| return Ok(out); | ||
| } | ||
|
|
||
| for entry in WalkDir::new(root).into_iter().filter_map(|e| e.ok()) { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
walkdir WalkDir follow_links default behavior Rust
💡 Result:
The default behavior of WalkDir::follow_links in the Rust walkdir crate is false (disabled). Symbolic links are not followed by default. To enable following symbolic links, call .follow_links(true) on the WalkDir builder. When disabled, symbolic links are treated as files or directories without recursing into them if they point to directories. The root path, if a symlink, is always followed regardless of this setting.
Citations:
- 1: https://docs.rs/walkdir/2.2.8/walkdir/struct.WalkDir.html
- 2: https://docs.rs/walkdir/2.0.0/walkdir/struct.WalkDir.html
- 3: https://docs.rs/walkdir/
- 4: https://github.com/burntsushi/walkdir
🏁 Script executed:
cd crates/fbuild-packages/src/lnk && cat -n scanner.rs | head -65Repository: FastLED/fbuild
Length of output: 2502
🏁 Script executed:
# Check module-level documentation
rg -A 20 "^///" crates/fbuild-packages/src/lnk/scanner.rs | head -50Repository: FastLED/fbuild
Length of output: 1227
🏁 Script executed:
# Check what error types are available in fbuild_core
rg "pub enum.*Error\|pub struct.*Error" crates/fbuild-core/src/lib.rs -A 5Repository: FastLED/fbuild
Length of output: 40
🏁 Script executed:
# Check Cargo.toml for walkdir version to confirm web search applies
rg "walkdir" crates/fbuild-packages/Cargo.tomlRepository: FastLED/fbuild
Length of output: 88
🏁 Script executed:
# Search more broadly for error types in fbuild_core
rg "PackageError\|FbuildError" crates/fbuild-core/src/ -B 2 -A 2Repository: FastLED/fbuild
Length of output: 40
🏁 Script executed:
# Check fbuild_core's lib.rs or error module structure
fd -t f '(lib|error)\.rs' crates/fbuild-core/src/Repository: FastLED/fbuild
Length of output: 87
🏁 Script executed:
# Check how errors are typically handled in fbuild-packages
rg "FbuildError\|Result<" crates/fbuild-packages/src/ -A 1 | head -40Repository: FastLED/fbuild
Length of output: 40
🏁 Script executed:
cat -n crates/fbuild-core/src/lib.rs | head -150Repository: FastLED/fbuild
Length of output: 5320
🏁 Script executed:
# Check how other parts of fbuild-packages handle errors
rg "Result\|Error" crates/fbuild-packages/src/lib.rs -B 1 -A 2 | head -60Repository: FastLED/fbuild
Length of output: 40
Enable symlink traversal to match the documented behavior.
Line 40 uses WalkDir::new(root) without .follow_links(true), causing .lnk files in symlinked shared directories to be silently skipped, despite the module documentation explicitly promising symlink traversal. The code also silently drops filesystem traversal errors via .filter_map(|e| e.ok()).
Proposed fix
-use fbuild_core::Result;
+use fbuild_core::{FbuildError, Result};
@@
- for entry in WalkDir::new(root).into_iter().filter_map(|e| e.ok()) {
+ for entry in WalkDir::new(root).follow_links(true).into_iter() {
+ let entry = entry.map_err(|e| {
+ FbuildError::PackageError(format!(
+ "failed to scan .lnk files under {}: {e}",
+ root.display()
+ ))
+ })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-packages/src/lnk/scanner.rs` at line 40, Change the WalkDir
usage to enable symlink traversal and stop silently swallowing IO errors:
replace WalkDir::new(root) with WalkDir::new(root).follow_links(true) and
replace the `.into_iter().filter_map(|e| e.ok())` pattern with explicit error
handling (e.g., `.into_iter().map(|r| r.map_err(|e| { /* log or propagate e with
context */ e })).filter_map(Result::ok)` or use a match inside the iterator to
log warnings for Err variants) so `.lnk` files under symlinked directories are
visited and filesystem errors are surfaced or logged; update the loop that
currently reads `for entry in WalkDir::new(root).into_iter().filter_map(|e|
e.ok())` accordingly.
zackees
force-pushed
the
feat/lnk-resources
branch
2 times, most recently
from
fc32f97 to
8a4674c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fbuild-cli/src/main.rs (1)
411-426:⚠️ Potential issue | 🟠 MajorRegister
lnkas a known subcommand for positional-dir rewriting.
fbuild <project-dir> lnk pullwill not be rewritten because"lnk"is missing here, even thoughCommands::Lnkwas added.🛠️ Proposed fix
"clang-query", "test-emu", + "lnk", ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-cli/src/main.rs` around lines 411 - 426, KNOWN_SUBCOMMANDS is missing the "lnk" entry so positional-dir rewriting won't detect fbuild <project-dir> lnk pull; update the KNOWN_SUBCOMMANDS constant (the array bound referenced in main.rs) to include the string "lnk" alongside the other subcommands so it matches the newly added Commands::Lnk variant used for rewrite logic.
♻️ Duplicate comments (1)
crates/fbuild-cli/src/main.rs (1)
3068-3075:⚠️ Potential issue | 🟡 Minor
Addbasename parsing still breaks on trailing slashes and query strings.
url.rsplit('/').next()returns""for trailing-slash URLs and includes?query/#fragmentin the filename. This can produce.lnkor invalid Windows paths.🛠️ Proposed fix
- let basename = url.rsplit('/').next().unwrap_or("blob"); + let clean = url.split(['?', '#']).next().unwrap_or(&url); + let basename = clean + .rsplit('/') + .find(|segment| !segment.is_empty()) + .unwrap_or("blob"); let output_path = match output {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-cli/src/main.rs` around lines 3068 - 3075, The basename extraction in the LnkAction::Add block is brittle: compute a safe basename by first trimming trailing slashes from url, then take the last path segment (the last non-empty entry from url.split('/')), then strip any query or fragment by splitting on '?' and '#' and taking the first part; if the result is empty use "blob". Replace the current basename = url.rsplit('/').next().unwrap_or("blob") logic with this sanitizing sequence before building output_path (variables: basename, output_path, LnkAction::Add).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fbuild-cli/src/main.rs`:
- Around line 3129-3135: The trailing newline write currently swallows errors
with f.write_all(b"\n").ok(); instead propagate failures like the previous
write: replace the .ok() with a map_err to
fbuild_core::FbuildError::PackageError that includes output_path and the error
(or simply use ? after mapping), so the function (in main.rs where f, pretty and
output_path are used) returns an error on disk/permission problems instead of
silently continuing.
In `@crates/fbuild-packages/src/lnk/resolver.rs`:
- Around line 101-146: The staging/promotion currently races because every
resolver uses the same staging_dir and downloaded filename; change download_file
+ staging handling to write into a per-attempt temp path (unique filename or
temp dir) and run verify_sha256 on that temp file, then before replacing
final_path check if final_path already exists and is valid: if final_path
exists, verify its SHA via verify_sha256 and if it matches lnk.sha256 delete
your temp and reuse final_path; otherwise attempt an atomic move
(std::fs::rename) of your verified temp into archive_dir as final_path and if
the move fails due to existing file, re-check final_path's SHA and behave
accordingly (cleanup temp on success or mismatch). Apply this logic around the
symbols staging_dir, archive_dir, download_file, downloaded, verify_sha256, and
final_path so concurrent resolves race-safe promote verified blobs atomically.
---
Outside diff comments:
In `@crates/fbuild-cli/src/main.rs`:
- Around line 411-426: KNOWN_SUBCOMMANDS is missing the "lnk" entry so
positional-dir rewriting won't detect fbuild <project-dir> lnk pull; update the
KNOWN_SUBCOMMANDS constant (the array bound referenced in main.rs) to include
the string "lnk" alongside the other subcommands so it matches the newly added
Commands::Lnk variant used for rewrite logic.
---
Duplicate comments:
In `@crates/fbuild-cli/src/main.rs`:
- Around line 3068-3075: The basename extraction in the LnkAction::Add block is
brittle: compute a safe basename by first trimming trailing slashes from url,
then take the last path segment (the last non-empty entry from url.split('/')),
then strip any query or fragment by splitting on '?' and '#' and taking the
first part; if the result is empty use "blob". Replace the current basename =
url.rsplit('/').next().unwrap_or("blob") logic with this sanitizing sequence
before building output_path (variables: basename, output_path, LnkAction::Add).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a475ce8c-9560-42d8-80cc-87e8ff645b42
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/fbuild-build/src/esp32/orchestrator.rscrates/fbuild-cli/Cargo.tomlcrates/fbuild-cli/src/main.rscrates/fbuild-packages/Cargo.tomlcrates/fbuild-packages/src/disk_cache/paths.rscrates/fbuild-packages/src/extractor.rscrates/fbuild-packages/src/lib.rscrates/fbuild-packages/src/lnk/README.mdcrates/fbuild-packages/src/lnk/embed.rscrates/fbuild-packages/src/lnk/format.rscrates/fbuild-packages/src/lnk/materialize.rscrates/fbuild-packages/src/lnk/mod.rscrates/fbuild-packages/src/lnk/resolver.rscrates/fbuild-packages/src/lnk/scanner.rscrates/fbuild-packages/tests/lnk_e2e.rs
✅ Files skipped from review due to trivial changes (5)
- crates/fbuild-cli/Cargo.toml
- crates/fbuild-packages/src/lib.rs
- crates/fbuild-packages/src/lnk/scanner.rs
- crates/fbuild-packages/src/lnk/mod.rs
- crates/fbuild-packages/src/lnk/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/fbuild-packages/Cargo.toml
- crates/fbuild-packages/src/extractor.rs
- crates/fbuild-build/src/esp32/orchestrator.rs
- crates/fbuild-packages/src/disk_cache/paths.rs
- crates/fbuild-packages/src/lnk/materialize.rs
- crates/fbuild-packages/src/lnk/embed.rs
Comment on lines
+3129
to
+3135
| f.write_all(pretty.as_bytes()).map_err(|e| { | ||
| fbuild_core::FbuildError::PackageError(format!( | ||
| "failed to write {}: {e}", | ||
| output_path.display() | ||
| )) | ||
| })?; | ||
| f.write_all(b"\n").ok(); |
There was a problem hiding this comment.
Propagate the final write error.
Line 3135 drops errors from writing the trailing newline, so disk-full/permission failures can still print wrote ... after producing a partial manifest.
🛠️ Proposed fix
- f.write_all(b"\n").ok();
+ f.write_all(b"\n").map_err(|e| {
+ fbuild_core::FbuildError::PackageError(format!(
+ "failed to write {}: {e}",
+ output_path.display()
+ ))
+ })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| f.write_all(pretty.as_bytes()).map_err(|e| { | |
| fbuild_core::FbuildError::PackageError(format!( | |
| "failed to write {}: {e}", | |
| output_path.display() | |
| )) | |
| })?; | |
| f.write_all(b"\n").ok(); | |
| f.write_all(pretty.as_bytes()).map_err(|e| { | |
| fbuild_core::FbuildError::PackageError(format!( | |
| "failed to write {}: {e}", | |
| output_path.display() | |
| )) | |
| })?; | |
| f.write_all(b"\n").map_err(|e| { | |
| fbuild_core::FbuildError::PackageError(format!( | |
| "failed to write {}: {e}", | |
| output_path.display() | |
| )) | |
| })?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-cli/src/main.rs` around lines 3129 - 3135, The trailing newline
write currently swallows errors with f.write_all(b"\n").ok(); instead propagate
failures like the previous write: replace the .ok() with a map_err to
fbuild_core::FbuildError::PackageError that includes output_path and the error
(or simply use ? after mapping), so the function (in main.rs where f, pretty and
output_path are used) returns an error on disk/permission problems instead of
silently continuing.
Comment on lines
+101
to
+146
| // Stage into the per-entry archive dir. The cache's path helpers give | ||
| // us a stable, sanitized location keyed on (kind, url, version). | ||
| let staging_dir = cache.archive_staging_dir(Kind::LnkBlobs, &lnk.url, &lnk.sha256); | ||
| let archive_dir = cache.archive_dir(Kind::LnkBlobs, &lnk.url, &lnk.sha256); | ||
| std::fs::create_dir_all(&staging_dir).map_err(|e| { | ||
| FbuildError::PackageError(format!( | ||
| "failed to create lnk staging dir {}: {e}", | ||
| staging_dir.display() | ||
| )) | ||
| })?; | ||
|
|
||
| let downloaded = | ||
| crate::block_on_package_future(async { download_file(&lnk.url, &staging_dir).await })?; | ||
|
|
||
| verify_sha256(&downloaded, &lnk.sha256).map_err(|e| { | ||
| // Clean up the staging file so a retry starts fresh. | ||
| let _ = std::fs::remove_file(&downloaded); | ||
| e | ||
| })?; | ||
|
|
||
| let archive_bytes = std::fs::metadata(&downloaded) | ||
| .map(|m| m.len() as i64) | ||
| .unwrap_or(0); | ||
|
|
||
| // Promote staging → archive. | ||
| std::fs::create_dir_all(&archive_dir).map_err(|e| { | ||
| FbuildError::PackageError(format!( | ||
| "failed to create lnk archive dir {}: {e}", | ||
| archive_dir.display() | ||
| )) | ||
| })?; | ||
| let final_path = archive_dir.join( | ||
| downloaded | ||
| .file_name() | ||
| .ok_or_else(|| FbuildError::PackageError("downloaded file has no name".to_string()))?, | ||
| ); | ||
| if final_path.exists() { | ||
| let _ = std::fs::remove_file(&final_path); | ||
| } | ||
| std::fs::rename(&downloaded, &final_path).map_err(|e| { | ||
| FbuildError::PackageError(format!( | ||
| "failed to move lnk blob {} → {}: {e}", | ||
| downloaded.display(), | ||
| final_path.display() | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
Make staging/promotion safe for concurrent resolves.
Two builds resolving the same .lnk use the same staging directory and downloaded filename, then one can remove/rename the file while the other is verifying or promoting it. Use a per-attempt temp file/dir and atomically reuse an already-verified final_path instead of deleting it.
🛠️ Directional fix
- let staging_dir = cache.archive_staging_dir(Kind::LnkBlobs, &lnk.url, &lnk.sha256);
+ let staging_parent = cache.archive_staging_dir(Kind::LnkBlobs, &lnk.url, &lnk.sha256);
let archive_dir = cache.archive_dir(Kind::LnkBlobs, &lnk.url, &lnk.sha256);
- std::fs::create_dir_all(&staging_dir).map_err(|e| {
+ std::fs::create_dir_all(&staging_parent).map_err(|e| {
FbuildError::PackageError(format!(
"failed to create lnk staging dir {}: {e}",
- staging_dir.display()
+ staging_parent.display()
))
})?;
+ let staging_dir = tempfile::Builder::new()
+ .prefix("lnk-")
+ .tempdir_in(&staging_parent)
+ .map_err(|e| FbuildError::PackageError(format!("failed to create lnk temp dir: {e}")))?;
let downloaded =
- crate::block_on_package_future(async { download_file(&lnk.url, &staging_dir).await })?;
+ crate::block_on_package_future(async { download_file(&lnk.url, staging_dir.path()).await })?;Then, before replacing final_path, verify and reuse it if another resolver already promoted the same SHA.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-packages/src/lnk/resolver.rs` around lines 101 - 146, The
staging/promotion currently races because every resolver uses the same
staging_dir and downloaded filename; change download_file + staging handling to
write into a per-attempt temp path (unique filename or temp dir) and run
verify_sha256 on that temp file, then before replacing final_path check if
final_path already exists and is valid: if final_path exists, verify its SHA via
verify_sha256 and if it matches lnk.sha256 delete your temp and reuse
final_path; otherwise attempt an atomic move (std::fs::rename) of your verified
temp into archive_dir as final_path and if the move fails due to existing file,
re-check final_path's SHA and behave accordingly (cleanup temp on success or
mismatch). Apply this logic around the symbols staging_dir, archive_dir,
download_file, downloaded, verify_sha256, and final_path so concurrent resolves
race-safe promote verified blobs atomically.
… materialize
`.lnk` files are tiny JSON manifests checked into source control that
point at remote binary blobs. At build time fbuild fetches them,
verifies the sha256, caches them in the existing two-phase disk cache,
and materializes them into the build tree so downstream steps consume
them as if they had always been in the source.
The intent: keep the source repo small, keep large/binary assets out of
git history, but have them appear as if they were always there during
builds. Sha256 is mandatory — reproducible builds and content-addressable
caching both depend on it.
## Schema (v1)
```json
{
"v": 1,
"url": "https://example.com/asset.bin",
"sha256": "abcdef0123...64-hex...",
"size": 1234567,
"extract": "file"
}
```
`extract` defaults to "file"; "zip" and "tar.gz" extract into a
directory at the materialized path.
## New module: `fbuild-packages/src/lnk/`
| File | Purpose |
|------|---------|
| `format.rs` | LnkFile struct + JSON parser + validation |
| `scanner.rs` | walk a tree, collect every parsed `.lnk` |
| `resolver.rs` | cache lookup; on miss fetch + verify + record |
| `materialize.rs` | hardlink/copy or extract into build tree |
| `embed.rs` | glue for embed_files-style entry lists |
| `README.md` | format spec, design rationale, CLI usage, FAQ |
Cache layer: extends DiskCache with `Kind::LnkBlobs`. Cache key triple
is `(LnkBlobs, url, sha256)` — sha256 in the "version" slot ensures a
.lnk content change forces a refetch. Reuses the existing LRU + lease +
GC infrastructure.
## Pipeline integration
esp32 orchestrator pre-resolves any `.lnk` entries in
`board_build.embed_files` / `embed_txtfiles` before passing them to
`process_embed_files`. Materialized paths reach `objcopy`; the original
`.lnk` is invisible downstream. Cache leases are held in scope so the
GC can't reap a blob mid-build.
## CLI: `fbuild lnk`
- `pull [<dir>]` — scan + fetch every `.lnk` blob into the cache
- `check [<dir>]` — verify cached blobs against their sha256 (no network)
- `add <url> [-o <path>]` — download once, hash, write a new `.lnk`
## Composition with zccache
Zero changes needed. The compile step that consumes a materialized blob
already hashes its inputs as part of the cache key. Because the blob's
on-disk content equals its sha256, the cache key changes whenever the
.lnk's sha256 changes.
## Test coverage
- 36 unit tests in the new module (format/scanner/resolver/
materialize/embed)
- 4 end-to-end integration tests against an in-process axum HTTP server
(full fetch+verify+materialize, sha mismatch rejection, 404 handling,
cache-hit-skips-network)
- Total 960+ tests still green across fbuild-packages, fbuild-config,
fbuild-build, fbuild-cli
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zackees
force-pushed
the
feat/lnk-resources
branch
from
8a4674c to
ca583d5
Compare
zackees
added a commit
that referenced
this pull request
Apr 19, 2026
Includes the new .lnk resource pointer feature merged in #119. Also bumps the zccache dep pin to >=1.2.12 (post-link deploy hook). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Apr 19, 2026
Includes the new .lnk resource pointer feature merged in #119. Also bumps the zccache dep pin to >=1.2.12 (post-link deploy hook). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zackees
added a commit
to FastLED/FastLED
that referenced
this pull request
Apr 19, 2026
fbuild 2.1.19 on PyPI ships the new `.lnk` resource pointer module (FastLED/fbuild#119) and integrates the zccache 1.2.12 post-link deploy hook. This is the minimum version needed to consume either feature from the FastLED side. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zackees
added a commit
to FastLED/FastLED
that referenced
this pull request
Apr 19, 2026
fbuild 2.1.19 on PyPI ships the new `.lnk` resource pointer module (FastLED/fbuild#119) and integrates the zccache 1.2.12 post-link deploy hook. This is the minimum version needed to consume either feature from the FastLED side. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
zackees
added a commit
that referenced
this pull request
Apr 19, 2026
Older production cache databases (pre-#119) lack the `leases.refcount` column that `pin()`/`unpin()` expect, causing every call to `DiskCache::lease()` to error with "no such column: refcount" and blocking any `.lnk` pull against a long-lived cache. Replace the single-step `migrate_v1` with an append-only versioned migration framework tracked in a `schema_migrations` table, keyed by stable migration ids: - m001_initial_schema — original tables + indexes (idempotent CREATE TABLE IF NOT EXISTS). - m002_add_leases_refcount — ALTER TABLE adds refcount INTEGER NOT NULL DEFAULT 1 when the column is absent; no-op on new caches that already have it from m001. Each migration runs in its own transaction and is skipped if its id is already present in `schema_migrations`. Future deltas just append. The legacy `cache_meta.schema_version` key is still mirrored for tooling that inspects it, but the authoritative source of truth is now the migrations table. Revert the `best_effort_lease()` workaround in `lnk/resolver.rs` that #119 added: the lease is once again a hard requirement, since the migration guarantees the column exists on every opened cache. Tests: - `test_legacy_schema_missing_refcount_is_migrated` (unit): hand- seeds the pre-migration schema, opens via `CacheIndex::open`, asserts `pin()`/`unpin()` succeed. - `test_migrations_idempotent_across_reopens` (unit): second open must not re-apply migrations or double-error on ALTER TABLE. - `test_all_migrations_recorded_after_open` (unit): regression guard ensuring every registered migration is recorded. - `legacy_cache_migrates_and_lease_succeeds` (integration): full `DiskCache::open_at` → `cache.lease()` flow against a seeded legacy DB, directly reproducing the issue-#124 scenario. - `fresh_cache_lease_still_works` (integration): regression guard for the fresh-DB happy path. Closes #124 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
zackees
added a commit
that referenced
this pull request
Apr 19, 2026
…#133) Older production cache databases (pre-#119) lack the `leases.refcount` column that `pin()`/`unpin()` expect, causing every call to `DiskCache::lease()` to error with "no such column: refcount" and blocking any `.lnk` pull against a long-lived cache. Replace the single-step `migrate_v1` with an append-only versioned migration framework tracked in a `schema_migrations` table, keyed by stable migration ids: - m001_initial_schema — original tables + indexes (idempotent CREATE TABLE IF NOT EXISTS). - m002_add_leases_refcount — ALTER TABLE adds refcount INTEGER NOT NULL DEFAULT 1 when the column is absent; no-op on new caches that already have it from m001. Each migration runs in its own transaction and is skipped if its id is already present in `schema_migrations`. Future deltas just append. The legacy `cache_meta.schema_version` key is still mirrored for tooling that inspects it, but the authoritative source of truth is now the migrations table. Revert the `best_effort_lease()` workaround in `lnk/resolver.rs` that #119 added: the lease is once again a hard requirement, since the migration guarantees the column exists on every opened cache. Tests: - `test_legacy_schema_missing_refcount_is_migrated` (unit): hand- seeds the pre-migration schema, opens via `CacheIndex::open`, asserts `pin()`/`unpin()` succeed. - `test_migrations_idempotent_across_reopens` (unit): second open must not re-apply migrations or double-error on ALTER TABLE. - `test_all_migrations_recorded_after_open` (unit): regression guard ensuring every registered migration is recorded. - `legacy_cache_migrates_and_lease_succeeds` (integration): full `DiskCache::open_at` → `cache.lease()` flow against a seeded legacy DB, directly reproducing the issue-#124 scenario. - `fresh_cache_lease_still_works` (integration): regression guard for the fresh-DB happy path. Closes #124 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Apr 19, 2026
…arm build On Unix, `fbuild_core::containment::spawn_contained` delegated to `ContainedProcessGroup::spawn_with_containment` from the `running-process-core` crate. That implementation stores the first spawned child's PID as the group's PGID and then has every subsequent child call `setpgid(0, first_child_pid)` from its `pre_exec` hook. Once the first child exits and is reaped (e.g. the short `avr-gcc -dumpversion` call emitted by `log_toolchain_version`), the kernel tears down that process group. The second spawn's `setpgid(0, stale_pgid)` then fails with EPERM, which surfaces as `build error: build failed: io error: Operation not permitted (os error 1)` immediately after the `Toolchain: avr-gcc 7.3.0` line — exactly the failure reported in #129. This is reproducible on Linux CI from 2.1.17 onwards (Build Leonardo et al.) and blocks every AVR / ESP32 / etc. build made via the daemon. Fix: bypass the shared-pgid behaviour on Unix. Install a per-child `pre_exec` hook that creates a fresh process group with `setpgid(0, 0)` and, on Linux, requests `PR_SET_PDEATHSIG(SIGKILL)` so the kernel still kills the child when the spawning daemon thread exits. Windows is unchanged — Job Object assignment is stateless and has no analogous failure mode. macOS loses the drop-time `killpg` backstop, which was already a no-op in practice because the global `ContainedProcessGroup` lives in a `OnceLock` that never drops; this is the same coverage profile as before the fix. Regression test: `sequential_contained_spawns_do_not_fail_with_eperm` in `crates/fbuild-core/src/containment.rs` initialises the global group and performs two consecutive `spawn_contained` calls with a wait+sleep between them, mirroring the AVR build's "dumpversion then compile" shape. Refs #129. Reproducing commits: #108 (containment feature) + the interaction surfaced by #120 / #119 that made the second-spawn path universally reachable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Apr 19, 2026
…arm build On Unix, `fbuild_core::containment::spawn_contained` delegated to `ContainedProcessGroup::spawn_with_containment` from the `running-process-core` crate. That implementation stores the first spawned child's PID as the group's PGID and then has every subsequent child call `setpgid(0, first_child_pid)` from its `pre_exec` hook. Once the first child exits and is reaped (e.g. the short `avr-gcc -dumpversion` call emitted by `log_toolchain_version`), the kernel tears down that process group. The second spawn's `setpgid(0, stale_pgid)` then fails with EPERM, which surfaces as `build error: build failed: io error: Operation not permitted (os error 1)` immediately after the `Toolchain: avr-gcc 7.3.0` line — exactly the failure reported in #129. This is reproducible on Linux CI from 2.1.17 onwards (Build Leonardo et al.) and blocks every AVR / ESP32 / etc. build made via the daemon. Fix: bypass the shared-pgid behaviour on Unix. Install a per-child `pre_exec` hook that creates a fresh process group with `setpgid(0, 0)` and, on Linux, requests `PR_SET_PDEATHSIG(SIGKILL)` so the kernel still kills the child when the spawning daemon thread exits. Windows is unchanged — Job Object assignment is stateless and has no analogous failure mode. macOS loses the drop-time `killpg` backstop, which was already a no-op in practice because the global `ContainedProcessGroup` lives in a `OnceLock` that never drops; this is the same coverage profile as before the fix. Regression test: `sequential_contained_spawns_do_not_fail_with_eperm` in `crates/fbuild-core/src/containment.rs` initialises the global group and performs two consecutive `spawn_contained` calls with a wait+sleep between them, mirroring the AVR build's "dumpversion then compile" shape. Refs #129. Reproducing commits: #108 (containment feature) + the interaction surfaced by #120 / #119 that made the second-spawn path universally reachable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a new
lnkmodule tofbuild-packages+ wire it into the esp32 orchestrator + afbuild lnkCLI..lnkfiles are tiny JSON manifests checked into source control that point at remote binary blobs. At build time fbuild fetches them, verifies the sha256, caches them in the existing two-phase disk cache, and materializes them into the build tree so downstream steps consume them as if they had always been in the source.Sha256 is mandatory — reproducible builds and content-addressable caching both depend on it.
Motivation
Binary assets (audio samples, image atlases, FFT tables, pre-built blobs, ML weights) don't belong in git history but often need to reach firmware. Today an author uploads them to a GitHub release and hand-wires
platformio.inito reference the downloaded path. This feature moves that workflow into fbuild and makes it first-class: caching, integrity verification, deduplication across projects on the same machine, and a clean CLI.Concrete first customer: the FastLED MoodRing audio fixtures tracked in FastLED/FastLED#2256.
Format (v1)
{ "v": 1, "url": "https://example.com/asset.bin", "sha256": "abcdef0123...64-hex...", "size": 1234567, "extract": "file" }extractdefaults to"file";"zip"and"tar.gz"extract into a directory at the materialized path.Architecture
Cache layer: extends
DiskCachewithKind::LnkBlobs. Cache key triple is(LnkBlobs, url, sha256)— sha256 in the "version" slot ensures a.lnkcontent change forces a refetch. Reuses existing LRU + lease-aware GC infrastructure.Composition with zccache — zero changes needed. The compile step that consumes a materialized blob (e.g.
objcopy) already hashes its inputs as part of the cache key. Because the blob's on-disk content IS its sha256, the cache key flips automatically whenever the.lnksha256 flips.New module:
fbuild-packages/src/lnk/format.rsLnkFilestruct + JSON parser + validationscanner.rsscan_for_lnk(root)walks a tree, collects every parsed.lnkresolver.rsmaterialize.rsembed.rsembed_files-style entry listsREADME.mdPipeline integration
esp32/orchestrator.rspre-resolves any.lnkentries inboard_build.embed_files/embed_txtfilesbefore passing them toprocess_embed_files. Materialized paths reachobjcopy; the original.lnkis invisible downstream. Cache leases held in the orchestrator scope so GC can't reap blobs mid-build.CLI:
fbuild lnkTest coverage
All existing tests in
fbuild-packages,fbuild-config,fbuild-build,fbuild-clistill pass (~960+ total).Non-goals / v2 follow-ups
FBUILD_LNK_AUTH_<host>)file/zip/tar.gzembed_files, so this naturally ships narrow)Related
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
.lnkresource pointer system and public APIs for parsing, resolving, and materializing remote blobs.fbuild lnkCLI withpull,check, andadd..lnkentries are pre-resolved when processing embed_files/embed_txtfiles.Documentation
.lnkguide and usage docs.Tests