Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 22 additions & 0 deletions .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,25 @@ jobs:

- name: Test stdlib builds
run: pcb build stdlib

- name: Checkout kicad-test-fixtures
uses: actions/checkout@v5
with:
repository: diodeinc/kicad-test-fixtures
token: ${{ secrets.DIODE_ROBOT_TOKEN }}
path: kicad-test-fixtures

- name: Test pcb import (E2E)
run: |
pcb new --workspace test-import --repo github.com/test/test-import
find kicad-test-fixtures -name '*.kicad_pro' | while IFS= read -r pro; do
echo "=== Importing $pro ==="
pcb import "$pro" ./test-import
done
Copy link

Choose a reason for hiding this comment

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

Pipeline while loop may silently swallow import failures

Medium Severity

The find ... | while read pattern runs the while loop in a subshell. While GitHub Actions uses set -eo pipefail, set -e behavior inside a while loop body running in a pipeline subshell is historically inconsistent across bash versions and can silently continue past a failing pcb import without aborting the step. Since this is a test meant to catch import failures, a silently passing step defeats its purpose. Using find -exec or a for loop over a glob (or adding || exit 1 after the pcb import call) would reliably propagate errors.

Fix in Cursor Fix in Web

for board_dir in ./test-import/boards/*/; do
board=$(basename "$board_dir")
echo "=== Build $board ==="
pcb build "./test-import/boards/$board/$board.zen"
echo "=== Layout $board ==="
pcb layout "./test-import/boards/$board/$board.zen" --no-open
done
29 changes: 22 additions & 7 deletions crates/pcb/src/import/flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ pub(super) fn execute(args: ImportArgs) -> Result<()> {
let ctx = ImportContext::new(args)?;

let discovered = Discovered::run(ctx)?;
prepare_output(&discovered.ctx.paths, &discovered.selection)?;
prepare_output(
&discovered.ctx.paths,
&discovered.selection,
&discovered.ctx.args,
)?;
let validated = Validated::run(discovered)?;
let extracted = Extracted::run(validated)?;
let hierarchized = Hierarchized::run(extracted);
Expand Down Expand Up @@ -60,18 +64,29 @@ impl Discovered {
}
}

fn prepare_output(paths: &ImportPaths, selection: &ImportSelection) -> Result<()> {
fn prepare_output(
paths: &ImportPaths,
selection: &ImportSelection,
args: &ImportArgs,
) -> Result<()> {
let board_dir = paths
.workspace_root
.join("boards")
.join(&selection.board_name);
if board_dir.exists() {
std::fs::remove_dir_all(&board_dir).with_context(|| {
format!(
"Failed to remove existing board dir {}",
if args.force {
std::fs::remove_dir_all(&board_dir).with_context(|| {
format!(
"Failed to remove existing board dir {}",
board_dir.display()
)
})?;
} else {
anyhow::bail!(
"Board directory already exists: {}. Use --force to overwrite.",
board_dir.display()
)
})?;
);
}
}

let board_scaffold = crate::new::scaffold_board(&paths.workspace_root, &selection.board_name)?;
Expand Down
2 changes: 1 addition & 1 deletion crates/pcb/src/import/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ pub struct ImportArgs {
#[arg(value_name = "OUTPUT_DIR", value_hint = clap::ValueHint::AnyPath)]
pub output_dir: PathBuf,

/// Skip interactive confirmations (continue even if ERC/DRC errors are present)
/// Overwrite existing board directory if it already exists
#[arg(long = "force")]
pub force: bool,
}
Expand Down
25 changes: 4 additions & 21 deletions crates/pcb/src/import/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use pcb_zen_core::Diagnostics;
pub(super) fn validate(
paths: &ImportPaths,
selection: &ImportSelection,
args: &ImportArgs,
_args: &ImportArgs,
) -> Result<ImportValidationRun> {
let kicad_pro_abs = paths.kicad_project_root.join(&selection.selected.kicad_pro);
let kicad_sch_abs = paths.kicad_project_root.join(&selection.selected.kicad_sch);
Expand Down Expand Up @@ -87,26 +87,9 @@ pub(super) fn validate(

let error_count = diagnostics_for_render.error_count();
if error_count > 0 {
if args.force {
eprintln!(
"Warning: KiCad ERC/DRC reported {error_count} errors; continuing due to --force."
);
} else if !crate::tty::is_interactive() || std::env::var("CI").is_ok() {
anyhow::bail!(
"KiCad ERC/DRC reported {error_count} errors. Fix them, or re-run in an interactive terminal to confirm continuing."
);
} else {
let continue_anyway = inquire::Confirm::new(&format!(
"KiCad ERC/DRC reported {error_count} errors. Continue anyway?"
))
.with_default(false)
.prompt()
.context("Failed to read confirmation")?;

if !continue_anyway {
anyhow::bail!("Aborted due to KiCad ERC/DRC errors");
}
}
eprintln!(
"Warning: KiCad ERC/DRC reported {error_count} errors; these do not block import."
);
}

Ok(ImportValidationRun {
Expand Down