tcl_lang: fix tcl_encode_sta.bzl rule - (#9524)#9530
tcl_lang: fix tcl_encode_sta.bzl rule - (#9524)#9530fredowski wants to merge 3 commits intoThe-OpenROAD-Project:masterfrom
Conversation
The tcl_lang package does not provide the default tcl library files like init.tcl with the tclsh. That is different compared to the rules_hdl version. tcl_lang provides the library files via tcl_core. I derived the path and set TCL_LIBRARY such that tclsh finds the right init.tcl file. Closes: The-OpenROAD-Project#9524 Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
![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
The pull request correctly addresses the issue where tclsh could not find the default Tcl library files by setting the TCL_LIBRARY environment variable and including the necessary files in the action's inputs. The changes also correctly move the encoding script from tools to inputs as it is a data dependency for the Tcl interpreter, which is a more accurate representation of the action's dependencies.
|
|
||
| # All tcl_core files live under library/, so split any file's path on | ||
| # "/library/" to obtain the library directory for TCL_LIBRARY. | ||
| tcl_lib_dir = ctx.files._tcl_library_files[0].path.split("/library/", 1)[0] + "/library" |
There was a problem hiding this comment.
The calculation of tcl_lib_dir assumes that ctx.files._tcl_library_files is not empty. If the file list is empty, this will cause a list index out of range error during the analysis phase. It is safer to verify that the list contains at least one file before accessing it, even though this is a private attribute with a default value.
if not ctx.files._tcl_library_files:
fail("No files found in _tcl_library_files")
tcl_lib_dir = ctx.files._tcl_library_files[0].path.split("/library/", 1)[0] + "/library"
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@hzeller review? |
|
(might need some |
|
clang-tidy review says "All clean, LGTM! 👍" |
fredowski
force-pushed
the
tcl_encode_sta
branch
from
b77e452 to
40d14a6
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
buildifier lint suggested this change according to https://github.com/bazelbuild/buildtools/blob/main/WARNINGS.md#unsorted-dict-items Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
Signed-off-by: Friedrich Beckmann <friedrich.beckmann@tha.de>
fredowski
force-pushed
the
tcl_encode_sta
branch
from
40d14a6 to
d55474e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
The linter is happy now. And i added a small build test. |
3 participants
The tcl_lang package does not provide the default tcl library files like init.tcl with the tclsh. That is different compared to the rules_hdl version. tcl_lang provides the library files via tcl_core. I derived the path and set TCL_LIBRARY such that tclsh finds the right init.tcl file.
Closes: #9524