etc: fix dependency install order#4248
Conversation
Signed-off-by: Vitor Bandeira <vvbandeira@precisioninno.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 DependencyInstaller.sh to include tcl-tclreadline and tcl-tclreadline-devel across several Linux distributions and modifies the Dockerfile to split dependency installation into base and common phases. Feedback suggests that hardcoding a direct RPM URL for EL9 is brittle and should be replaced with a standard package name. Furthermore, the reviewer recommends improving the script's internal sequencing logic instead of splitting calls in the Dockerfile and ensuring consistent package installation for EL9.
|
|
||
| if [[ "${elVersion}" == "9" ]]; then | ||
| dnf -y install \ | ||
| https://mirror.stream.centos.org/9-stream/AppStream/x86_64/os/Packages/readline-devel-8.1-4.el9.x86_64.rpm |
There was a problem hiding this comment.
Hardcoding a direct URL to a specific RPM version on a mirror is brittle. This URL will break if the package is updated or moved to the archives on the mirror. It is recommended to use the package name readline-devel instead, which allows the package manager to resolve the correct version from the configured repositories.
| https://mirror.stream.centos.org/9-stream/AppStream/x86_64/os/Packages/readline-devel-8.1-4.el9.x86_64.rpm | |
| readline-devel |
| RUN ./DependencyInstaller.sh -base $options $constantBuildDir -save-deps-prefixes=/etc/openroad_deps_prefixes.txt \ | ||
| && ./DependencyInstaller.sh -common $options $constantBuildDir -save-deps-prefixes=/etc/openroad_deps_prefixes.txt \ |
There was a problem hiding this comment.
Splitting the -all flag into separate -base and -common calls is a workaround for the execution order in DependencyInstaller.sh. A more robust fix would be to update the script itself to correctly sequence the base and common phases when -all is used. This would ensure that all base dependencies (including those in the sub-script) are installed before any common/pip dependencies are processed, benefiting all users of the script and avoiding redundant script initialization.
| if [[ "${elVersion}" == "9" ]]; then | ||
| dnf -y install \ | ||
| https://mirror.stream.centos.org/9-stream/AppStream/x86_64/os/Packages/readline-devel-8.1-4.el9.x86_64.rpm | ||
| fi |
There was a problem hiding this comment.
The tcl-tclreadline and tcl-tclreadline-devel packages are added for EL7 and EL8 but are missing for EL9. If these are required dependencies for the project, they should be included for EL9 as well to maintain consistency across supported Enterprise Linux versions.
|
Is this to fix ORFS master? @sombraSoft was just working on #4249 which seems similar |
Yes. Secure Branches Nightly already passed the yosys install. |
|
Thanks for fixing it. |
2 participants
No description provided.