Add native PlatformIO source filters and debug flag support#44
Merged
Conversation
📝 WalkthroughWalkthroughThis PR extends native platformio.ini compatibility by implementing source filtering via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (3)
crates/fbuild-build/src/pipeline.rs (1)
235-253: Consider edge case:-ggdbwithout level suffix.The
is_platformio_debug_cleanup_flagfunction handles-ggdb[0-3](6-character strings) but doesn't handle bare-ggdb(5 characters). PlatformIO's default debug flags include-ggdb2, so this may not matter in practice, but if a user specifies-ggdbwithout a level, it won't be cleaned up before adding the debug flags.Similarly,
is_platformio_debug_link_flagusesflag.starts_with("-g")which would match-ggdb, but the cleanup function wouldn't remove it.This is a minor edge case that's unlikely to cause issues in practice.
♻️ Optional: Handle bare `-ggdb` in cleanup
fn is_platformio_debug_cleanup_flag(flag: &str) -> bool { if flag == "-Os" || flag == "-g" { return true; } + if flag == "-ggdb" { + return true; + } if flag.len() == 3 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/pipeline.rs` around lines 235 - 253, The cleanup logic in is_platformio_debug_cleanup_flag misses the bare "-ggdb" (5 chars) case; update is_platformio_debug_cleanup_flag (and only that function) to treat "-ggdb" as a cleanup flag in addition to "-ggdb[0-3]" — e.g. accept either a 5-char exact "-ggdb" or a 6-char "-ggdbN" where N ∈ {0,1,2,3}; leave is_platformio_debug_link_flag as-is so it continues to match "-g" and "-ggdb" during linking.crates/fbuild-build/src/source_scanner.rs (2)
61-71: Consider adding doc comments explaining the matching semantics.These structs implement PlatformIO-style source filtering, but the behavior isn't documented. The
has_include_rulesfield particularly affects default matching behavior in a non-obvious way—when include rules exist, unmatched files are excluded; otherwise, they're included by default.📝 Suggested documentation
+/// PlatformIO-style source filter for selective file inclusion/exclusion. +/// +/// Rules are evaluated in order; the last matching rule determines inclusion. +/// If any `+<pattern>` rules exist, files not matching any rule are excluded by default. +/// If only `-<pattern>` rules exist, files are included unless explicitly excluded. #[derive(Debug)] struct SourceFilter { rules: Vec<SourceFilterRule>, + /// When true, unmatched files are excluded by default has_include_rules: bool, } +/// A single include (+) or exclude (-) rule with a compiled regex matcher. #[derive(Debug)] struct SourceFilterRule { include: bool, matcher: Regex, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/source_scanner.rs` around lines 61 - 71, Add doc comments to SourceFilter and SourceFilterRule describing their PlatformIO-style matching semantics: explain that SourceFilter.rules is a list of SourceFilterRule entries, each rule's include boolean indicates whether matching paths are allowed (include=true) or denied (include=false), and matcher (Regex) is used to test paths; clarify that SourceFilter.has_include_rules toggles the default for unmatched files — if any include rules exist unmatched files are excluded, otherwise unmatched files are included by default. Place these comments above the struct definitions for SourceFilter and SourceFilterRule to make the behavior explicit to future readers.
781-812: Good foundational tests; consider expanding coverage.The tests validate the core functionality well. For robustness, consider adding tests for:
- Glob wildcards:
**,?,*.cpp- Patterns containing regex-special characters (
.,+)- Error cases (malformed patterns like
+<>,+< >,foo)- Windows-style backslash patterns as input
🧪 Example additional test
#[test] fn test_source_filter_glob_wildcards() { let (_tmp, src_dir, build_dir) = setup_project(&[ ("foo.cpp", ""), ("bar.cpp", ""), ("sub/baz.cpp", ""), ]); let scanner = SourceScanner::new(&src_dir, &build_dir); // Test *.cpp pattern matches all .cpp files let sources = scanner .scan_sketch_sources_filtered(Some("+<*.cpp>")) .unwrap(); assert_eq!(sources.len(), 3); } #[test] fn test_source_filter_invalid_pattern_error() { let (_tmp, src_dir, build_dir) = setup_project(&[("main.cpp", "")]); let scanner = SourceScanner::new(&src_dir, &build_dir); let result = scanner.scan_sketch_sources_filtered(Some("invalid")); assert!(result.is_err()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/source_scanner.rs` around lines 781 - 812, Add unit tests expanding coverage for SourceScanner::scan_sketch_sources_filtered: create new tests (e.g., test_source_filter_glob_wildcards, test_source_filter_special_chars, test_source_filter_windows_backslashes, test_source_filter_invalid_pattern_error) that use setup_project to create files and assert behavior for glob patterns like "*.cpp", "**", "?", patterns containing regex-special chars (e.g., ".", "+"), Windows-style backslash inputs, and malformed patterns (e.g., "+<>", "invalid") to ensure correct matching and error handling; keep tests concise, call SourceScanner::new(&src_dir, &build_dir) and verify returned Ok results and Err for invalid patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/fbuild-build/src/pipeline.rs`:
- Around line 235-253: The cleanup logic in is_platformio_debug_cleanup_flag
misses the bare "-ggdb" (5 chars) case; update is_platformio_debug_cleanup_flag
(and only that function) to treat "-ggdb" as a cleanup flag in addition to
"-ggdb[0-3]" — e.g. accept either a 5-char exact "-ggdb" or a 6-char "-ggdbN"
where N ∈ {0,1,2,3}; leave is_platformio_debug_link_flag as-is so it continues
to match "-g" and "-ggdb" during linking.
In `@crates/fbuild-build/src/source_scanner.rs`:
- Around line 61-71: Add doc comments to SourceFilter and SourceFilterRule
describing their PlatformIO-style matching semantics: explain that
SourceFilter.rules is a list of SourceFilterRule entries, each rule's include
boolean indicates whether matching paths are allowed (include=true) or denied
(include=false), and matcher (Regex) is used to test paths; clarify that
SourceFilter.has_include_rules toggles the default for unmatched files — if any
include rules exist unmatched files are excluded, otherwise unmatched files are
included by default. Place these comments above the struct definitions for
SourceFilter and SourceFilterRule to make the behavior explicit to future
readers.
- Around line 781-812: Add unit tests expanding coverage for
SourceScanner::scan_sketch_sources_filtered: create new tests (e.g.,
test_source_filter_glob_wildcards, test_source_filter_special_chars,
test_source_filter_windows_backslashes,
test_source_filter_invalid_pattern_error) that use setup_project to create files
and assert behavior for glob patterns like "*.cpp", "**", "?", patterns
containing regex-special chars (e.g., ".", "+"), Windows-style backslash inputs,
and malformed patterns (e.g., "+<>", "invalid") to ensure correct matching and
error handling; keep tests concise, call SourceScanner::new(&src_dir,
&build_dir) and verify returned Ok results and Err for invalid patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c292d00-4482-4ded-beb0-497f75c2abce
📒 Files selected for processing (17)
crates/fbuild-build/src/apollo3/orchestrator.rscrates/fbuild-build/src/avr/orchestrator.rscrates/fbuild-build/src/ch32v/orchestrator.rscrates/fbuild-build/src/esp32/orchestrator.rscrates/fbuild-build/src/esp8266/orchestrator.rscrates/fbuild-build/src/nrf52/orchestrator.rscrates/fbuild-build/src/pipeline.rscrates/fbuild-build/src/renesas/orchestrator.rscrates/fbuild-build/src/rp2040/orchestrator.rscrates/fbuild-build/src/sam/orchestrator.rscrates/fbuild-build/src/script_runtime.rscrates/fbuild-build/src/script_runtime_harness.pycrates/fbuild-build/src/silabs/orchestrator.rscrates/fbuild-build/src/source_scanner.rscrates/fbuild-build/src/stm32/orchestrator.rscrates/fbuild-build/src/teensy/orchestrator.rscrates/fbuild-config/src/ini_parser.rs
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
build_src_filter/src_filtersupport and apply it to sketch source discovery across native orchestratorsbuild_unflags,build_type=debug, anddebug_build_flagsin the build pipelineDump,BoardConfig(), andPioPlatform()Testing
cargo test -p fbuild-config --libcargo test -p fbuild-build --libCloses #43
Summary by CodeRabbit
Release Notes
New Features
Documentation