flow/util/generate_klayout_tech: write absolute LEF paths for Bazel sandbox#4234
Open
oharboe wants to merge 6 commits into
Open
flow/util/generate_klayout_tech: write absolute LEF paths for Bazel sandbox#4234oharboe wants to merge 6 commits into
oharboe wants to merge 6 commits into
Conversation
os.path.realpath() follows symlinks. Inside a Bazel sandbox, the bazel-out/ tree is symlinked from the sandbox back to the bare execroot; realpath resolves through that symlink and yields the bare-execroot path, not the sandbox path. That alone doesn't matter for os.path.relpath(a, b) when both operands are realpath'd from the same sandbox — the relative result is unchanged. But the resulting <lef-files> path in the generated klayout.lyt is later resolved by klayout against the LYT file's location. Klayout opens the LYT (also through a symlink), resolves through to the bare execroot, and then looks for the sibling klayout_tech.lef at the bare-execroot path — where the in-flight file does not exist during action execution (only the sandbox copy does), so def2stream fails with errno=2. Fix: don't realpath. os.path.relpath produces the correct relative path from sandbox-relative inputs directly. Map files keep their absolute form via abspath for the unchanged-under-non-sandbox case. Surfaced for the first time on bazelisk build //flow/designs/sky130hd/gcd:gcd_final_blender_html — the orfs_blender macro is the first bazel-side consumer of orfs_gds (klayout def2stream), so the latent path bug had no prior trigger. Mirrors bazel-orfs's patches/0037-fix-generate_klayout_tech-drop-realpath.patch into ORFS directly so bazel-orfs no longer needs to carry it. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
The previous switch to relpath (commit 01652ca) cleaned up the content but did not actually fix def2stream's errno=2 under a Bazel sandbox. Investigation via strace shows klayout's Layout.read(def, layout_options) opens the input DEF (a sandbox symlink to the bare execroot, staged from orfs_final), realpath's it to the bare-execroot results dir, and then resolves <lef-files> relative paths against THAT dir. Sibling intermediates like objects/klayout_tech.lef only exist in the per-action sandbox, not at the bare execroot, so the lookup lands at .../execroot/_main/bazel-out/.../klayout_tech.lef and fails with errno=2. Declaring klayout_tech.lef as a Bazel output of orfs_gds doesn't help -- Bazel only materialises declared outputs at the bare execroot at action completion, not while the action is still running and klayout is reading them. Plain abspath (NOT realpath -- realpath would chase Bazel input-file symlinks back out to the bare execroot for any input already at the bare execroot) writes the in-sandbox absolute LEF path into the LYT. KLayout opens that absolute path directly with no relative-path resolution involved, so the sibling lookup never escapes the sandbox. Cost: the LYT's content now embeds the per-invocation sandbox number, so the orfs_gds action cannot be cross-action cached. That's an acceptable trade-off for correctness while we wait for a klayout-level fix or a Bazel feature that lets us write a stable bare-execroot path from inside the action. Confirmed end-to-end: bazelisk build //flow/designs/sky130hd/gcd:gcd_final_blender_html produces 6_final.gds and the blender_html sh_binary, with both the sandbox sandbox-id N going forward and after fresh action re-runs (verified by deleting outputs and rebuilding). Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Earlier commit a1552a8 switched generate_klayout_tech.py to write absolute LEF paths into the LYT (bypasses klayout's relative-resolution escape to the bare-execroot under a Bazel sandbox). The unit test test_basic_generation still asserted the old relpath form and failed in CI: AssertionError: '../tech.lef' not found in '...<lef-files>/tmp/tmpwlgkq96a/tech.lef</lef-files>...' Update the assertion to expect os.path.abspath(lef_path). Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
![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 updates the Klayout technology generation to use absolute paths instead of relative or real paths for LEF and map files, ensuring correct resolution within Bazel sandboxes. Review feedback highlights that the reference_dir and use_relative_paths parameters are now unused and should be cleaned up. Additionally, an inconsistency was noted where the test_with_map_files test still expects realpath, which may cause failures on certain platforms.
…enROAD-Project#4234 Two follow-ups from gemini's review of The-OpenROAD-Project#4234: * The map-file test in test_with_map_files still asserted `os.path.realpath(map_path)`, inconsistent with the LEF test's `os.path.abspath` switch. On platforms whose tmpdir contains symlinks (macOS /var -> /private/var) the test would fail. Switch to `os.path.abspath` to match the LEF case. * `reference_dir` and `use_relative_paths` are now both unused. Update the function docstring to say so, default both to `None` / `False` so callers can drop them, and demote the CLI flags to `required=False` so external scripts that don't pass them still work. flow/Makefile still passes `--reference-dir` so we keep accepting the flag. Signed-off-by: Øyvind Harboe <oyvind@ascenium.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Collaborator
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the KLayout technology generation utility to use absolute paths for LEF and map files, which prevents path resolution failures within Bazel sandboxes. It also deprecates the reference_dir and use_relative_paths parameters by making them optional. The review feedback correctly identifies that the change in the function's argument order breaks backward compatibility for positional calls and suggests restoring the original order, updating the docstring for consistency, and ensuring map_files is handled safely if passed as null.
…ect#4234 Three follow-ups from gemini's second review pass: * Restore the original positional argument order (template_lyt, output_lyt, lef_files, reference_dir, map_files, use_relative_paths) so Python callers that pass the args positionally still work. Prior fix reshuffled `map_files` ahead of `reference_dir`, breaking positional compatibility. * Re-order the Args block in the docstring to match the new signature order. * `map_files` is now `None`-defaulted (to keep the positional-order fix above), so iterate via `map_files or []` rather than dereferencing `None` directly. Signed-off-by: Øyvind Harboe <oyvind@ascenium.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Collaborator
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Klayout technology generation utility to use absolute paths for LEF and map files instead of relative or real paths. This change ensures that file resolution works correctly within Bazel sandboxed environments where relative path resolution against the realpath of the input DEF would otherwise fail. The reference_dir and use_relative_paths parameters have been deprecated and are now unused, though they remain for backward compatibility. I have no feedback to provide as there were no review comments.
maliberty
reviewed
May 18, 2026
Comment on lines
+47
to
+49
| reference_dir: Unused. Accepted for backward compatibility with | ||
| callers (e.g. flow/Makefile) that still pass it from when | ||
| paths were resolved relative to this directory. |
Member
There was a problem hiding this comment.
Just remove the unused args and update the Makefile.
…e_paths Per maliberty's review on The-OpenROAD-Project#4234: rather than keep `reference_dir` and `use_relative_paths` around as ignored backward-compat parameters, drop them entirely and update flow/Makefile to stop passing `--reference-dir`. No remaining caller in the tree relies on either. Changes: * flow/util/generate_klayout_tech.py — `generate_klayout_tech()` signature is now (template_lyt, output_lyt, lef_files, map_files=None). CLI loses `--reference-dir` and `--use-relative-paths`. Stale comment / docstring lines about the dropped args go too. * flow/Makefile — `do-klayout` and `do-klayout_wrap` both stop passing `--reference-dir`. * flow/test/test_generate_klayout_tech.py — drops the now-invalid `reference_dir=` / `use_relative_paths=` kwargs from the three integration-style tests. Existing 13 tests still pass. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
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
2 participants
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
flow/util/generate_klayout_tech.pyused to emit LEF entries into thegenerated
.lytas paths relative toreference_dir, with the filefirst canonicalised via
os.path.realpath. Both choices break theorfs_gdsstep when ORFS runs under a Bazel sandbox:realpath()follows symlinks. Inside a Bazel sandbox,bazel-out/is symlinked back to the bare execroot, so paths get rewritten to
the execroot location — which doesn't carry the per-action sibling
files that exist only in the per-action sandbox.
Even after dropping
realpath(), klayout'sLayout.read(def, layout_options)resolves the<lef-files>relative entries againstthe realpath of the DEF it's merging — which is again the bare
execroot. Sibling intermediates like
objects/klayout_tech.lefonlyexist in the per-action sandbox, so resolution fails with errno=2:
Plain
abspath(no symlink following) writes the in-sandbox absoluteLEF path into the LYT, which klayout then opens directly with no
relative-resolution dance. Same fix is applied to the .map paths a
few lines below.
Test (
flow/test/test_generate_klayout_tech.py::test_with_template)is updated to assert
os.path.abspath(lef_path)instead of the prioros.path.relpath-against-realpath form.Test plan
flow/test/test_generate_klayout_tech.py(13 tests)passes against the new behaviour.
still work — paths going abspath rather than relpath should be
a strict superset of what klayout accepts.
🤖 Generated with Claude Code