refactor:remove stale packages from dependency list#9871
refactor:remove stale packages from dependency list#9871alokkumardalei-wq wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Conversation
![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 removes several unused packages from the dependency installation scripts, which is a welcome cleanup. My review found an opportunity to further improve the maintainability of these scripts by sorting the package lists, making them easier to read and manage in the future.
etc/DependencyInstaller.sh
Outdated
| apt-transport-https ca-certificates curl gnupg python3 \ | ||
| python3-pip python3-pandas jq parallel \ | ||
| software-properties-common time unzip zip |
There was a problem hiding this comment.
Since this PR is already focused on cleaning up dependencies, it would be a great opportunity to also sort these package lists alphabetically. This improves readability and makes it easier to manage dependencies in the future. This suggestion could be applied to the other package lists in this file as well.
| apt-transport-https ca-certificates curl gnupg python3 \ | |
| python3-pip python3-pandas jq parallel \ | |
| software-properties-common time unzip zip | |
| apt-transport-https ca-certificates curl gnupg jq parallel \ | |
| python3 python3-pandas python3-pip software-properties-common \ | |
| time unzip zip |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Removes `python3-click`, `qimgv`, `default-jdk`, and `lsb-release` from the dependency scripts as requested in The-OpenROAD-Project#9868. Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
alokkumardalei-wq
force-pushed
the
cleanup/remove-stale-dependencies
branch
from
1f7ecbd to
97a2575
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
sombraSoft
left a comment
sombraSoft
left a comment
There was a problem hiding this comment.
LTGM!
Unless @vvbandeira knows about some utility for any of these packages, I think it's a go.
|
Actually the only reason I think we would need |
|
Why does bazelisk need a jdk? |
|
not actually bazelisk but bazel does! if the goal of this script is to setup bazel too we need jdk, no? |
|
Why does bazel need a jdk? |
|
Hello @sombraSoft thank you for the feedback but I think @maliberty asked the right question since I think (I might be wrong here) modern versions of Bazel actually bundle their own internal hermetic JRE inside the binary for execution. Since OpenROAD is a C++/TCL codebase and doesn't compile Java targets, we do not need to install the heavy Please let me know if I missed something here. |
|
Well, I guess I couldn't expect less from Bazel 😅 |
| _execute "Updating package lists..." apt-get -y update | ||
| _execute "Installing CI packages..." apt-get -y install --no-install-recommends \ | ||
| apt-transport-https ca-certificates curl default-jdk gnupg python3 \ | ||
| python3-pip python3-pandas jq lsb-release parallel \ |
There was a problem hiding this comment.
Please keep python3-pip lsb-release they are required for some CI tools
|
The request to keep lsb-release needs to be addressed. |
4 participants
What does this PR do?
Fixes #9868.
This PR cleans up the dependency installer scripts by removing a few outdated packages that OpenROAD doesn't actually use anymore, exactly as reported in #9868.
Specifically, it removes:
python3-clickqimgvdefault-jdklsb-releaseBefore removing them, I ran a full search across the entire OpenROAD codebase and verified that there is zero source code or scripts currently relying on these packages.
Potential Impact
Removing these will help speed up the Docker builds and save local developers from downloading hundreds of megabytes of unnecessary software (especially the Java Development Kit!).