bazel: fix installed binary Tcl init failure#10116
Conversation
install.sh deleted openroad.runfiles/_main which contains the Tcl resource files (init.tcl, tclreadline, etc.) needed by the Bazel-built binary. Remove that rm so the runfiles tree stays intact after install. Add an install smoke test that extracts the packaging tarball and verifies the binary can evaluate a Tcl expression — catches regressions like this automatically. Validate: bazelisk test //test/install/... Fixes The-OpenROAD-Project#10115 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> 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 introduces a smoke test to verify that the packaged OpenROAD binary initializes correctly after extraction. It includes a new Bazel test target, a shell script for the test logic, and documentation. Feedback includes removing the "local" tag from the test to allow sandboxing, using "rm -rf" for consistency when removing the repo mapping, and refactoring the test execution logic to ensure diagnostic output is displayed even if the binary fails to start.
| sh_test( | ||
| name = "install_test", | ||
| srcs = ["install_test.sh"], | ||
| args = ["$(rootpath //packaging:tarfile)"], | ||
| data = ["//packaging:tarfile"], | ||
| tags = ["local"], | ||
| ) |
There was a problem hiding this comment.
The local tag disables sandboxing and remote execution for this test, which can negatively impact build performance and reproducibility. Unless this test specifically requires access to host resources that cannot be provided in a sandbox (e.g., specific hardware or global paths), it is recommended to remove this tag to allow for better caching and parallel execution.
sh_test(
name = "install_test",
srcs = ["install_test.sh"],
args = ["$(rootpath //packaging:tarfile)"],
data = ["//packaging:tarfile"],
)
There was a problem hiding this comment.
the test needs to be verified, i.e. that it is no more complicated than it needs to be to test this. Testing install like this is tricky because we're testing that an install used outside of bazel and not relying on pwd works. So an "hors format" bazel test.
|
clang-tidy review says "All clean, LGTM! 👍" |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@hzeller FYI |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Once we know why |
But meanwhile this is what is needed? |
|
@maliberty if the tcl 9 update makes this moot in a timeframe that works, then I am happy to close it. I have the workaround. @hzeller Thoughts? |
|
I am working in my spare time on this, so I can't give any time estimates. |
|
@maliberty Go with this fix meanwhile? |
|
clang-tidy review says "All clean, LGTM! 👍" |
3 participants
install.sh deleted openroad.runfiles/_main which contains the Tcl resource files (init.tcl, tclreadline, etc.) needed by the Bazel-built binary. Remove that rm so the runfiles tree stays intact after install.
Add an install smoke test that extracts the packaging tarball and verifies the binary can evaluate a Tcl expression — catches regressions like this automatically.
Validate: bazelisk test //test/install/...
Fixes #10115