Fix Abseil conflict with or-tools'#10772
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 the CMake policy CMP0144 override and updates the Abseil dependency version in the installer script to match prebuilt or-tools. It also enhances the Abseil installation check to support both lib and lib64 directories. Feedback on these changes suggests declaring the new loop variables as local to prevent variable leakage in Bash, and removing a redundant loop that sets a local variable that is never used again.
| local absl_prefix_found="" | ||
| local absl_version_file="" |
There was a problem hiding this comment.
The loop variables absl_version_file_default and absl_version_file_or_tools are used in for loops within this function but are not declared as local. In Bash, variables are global by default unless declared with local, which can lead to variable leakage or unintended side effects in the calling scope. Declaring them as local at the beginning of the function is recommended.
| local absl_prefix_found="" | |
| local absl_version_file="" | |
| local absl_prefix_found="" | |
| local absl_version_file="" | |
| local absl_version_file_default="" | |
| local absl_version_file_or_tools="" |
| for absl_version_file_default in \ | ||
| "${absl_prefix_install}/lib64/cmake/absl/abslConfigVersion.cmake" \ | ||
| "${absl_prefix_install}/lib/cmake/absl/abslConfigVersion.cmake"; do | ||
| if [[ -f "${absl_version_file_default}" ]]; then | ||
| absl_version_file="${absl_version_file_default}" | ||
| break | ||
| fi | ||
| done |
There was a problem hiding this comment.
This loop attempts to find and set absl_version_file after Abseil is built and installed. However, absl_version_file is a local variable that is never read or used again after this point, making this entire loop redundant. Removing it simplifies the code and avoids unnecessary file system checks.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:
- Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/contributing.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
Signed-off-by: Jimmy Situ <web@jimmystone.cn>
|
@mguthaus does this solve your issue? |
2 participants
Summary
Fix Abseil conflict with or-tools' in OpenROAD
Type of Change
Change DependencyInstaller.sh, use or-tools' Abseil when or-tools already installed one
This change can fix the isssue #10651
Impact
Abseil is changed to ABSL_VERSION="20250512.0", which is snyced with or-tools'
Verification
./etc/Build.sh).Related Issues
[#10651]