Skip to content

rcx: fix the diff_spef test helper - #11070

Open
naveenvenk17 wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
naveenvenk17:rcx-fix-diff-spef-helper-filename
Open

rcx: fix the diff_spef test helper#11070
naveenvenk17 wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
naveenvenk17:rcx-fix-diff-spef-helper-filename

Conversation

@naveenvenk17

@naveenvenk17 naveenvenk17 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Fixes the broken test helper reported in #11050.

The helper accepts a filename but was passing an undefined variable into DiffOptions. This patch changes only diff_spef to use the filename argument and leaves write_spef unchanged.

This PR covers the helper bug only. It does not claim to fix the separate diff_spef output and calibration issues from the report.

Tested on the VM:
bazel test //src/rcx/test:rcx_aux_test --test_output=errors

@naveenvenk17
naveenvenk17 requested a review from a team as a code owner August 5, 2026 07:47
@naveenvenk17
naveenvenk17 requested a review from AcKoucher August 5, 2026 07:47

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a typo in src/rcx/test/rcx_aux.py by changing filename to filenamename in the write_spef function. The reviewer correctly identified that this introduces an undefined variable causing a NameError and suggested reverting this change and applying the fix to the correct function (diff_spef) instead.

Comment thread src/rcx/test/rcx_aux.py Outdated
def write_spef(design, *, filename="", nets="", net_id=0, coordinates=False):
opts = rcx.SpefOptions()
opts.file = filename
opts.file = filenamename

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

This change introduces an undefined variable filenamename, which will cause a NameError when write_spef is called. Additionally, this modifies write_spef instead of diff_spef (located at line 113), which is the helper that actually has the undefined file variable. Please revert this change and apply the fix to diff_spef instead.

Suggested change
opts.file = filenamename
opts.file = filename

@AcKoucher AcKoucher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixes #11050.

There are 4 things being reported in that issue and this PR addresses only one of them. Please, fix the description.

Comment thread src/rcx/test/rcx_aux.py Outdated
@@ -59,7 +55,7 @@ def extract_parasitics(

def write_spef(design, *, filename="", nets="", net_id=0, coordinates=False):
opts = rcx.SpefOptions()
opts.file = filename
opts.file = filenamename

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo?

Copy link
Copy Markdown
Contributor Author

I fixed the review issue. The accidental write_spef change is reverted, and only diff_spef now uses its filename argument. py_compile and an AST-based helper check both pass on the VM. I also updated the description so this PR claims only the helper fix, not the other diff_spef issues.

@github-actions github-actions Bot added size/S and removed size/XS labels Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

I fixed the requested issue and added a small behavioral test for the helper. It calls diff_spef with a fake RCX API and checks that the filename and option flags reach DiffOptions. The Bazel test passes on the VM, and the PR description now covers only this helper bug.

Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
@naveenvenk17
naveenvenk17 force-pushed the rcx-fix-diff-spef-helper-filename branch from 0707cee to e718f48 Compare August 6, 2026 16:43

Copy link
Copy Markdown
Contributor Author

I corrected the description to cover only the diff_spef filename bug, restored write_spef, and added a behavioral test that checks the filename and flags reach DiffOptions. I also cleaned up the DCO issue by replacing the unsigned branch history with one signed commit. The focused Bazel test passes on the GCP VM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants