[Python] Fix absl sources in setup.py for system absl#41885
[Python] Fix absl sources in setup.py for system absl#41885sreenithi wants to merge 1 commit intogrpc:masterfrom
Conversation
|
/gemini review |
| system_absl_path = os.path.join("/usr", "include") | ||
| ABSL_INCLUDE = (system_absl_path,) | ||
| ABSL_SOURCES = [ | ||
| p.replace("third_party/abseil-cpp", system_absl_path) |
There was a problem hiding this comment.
| system_absl_path = os.path.join("/usr", "include") | |
| ABSL_INCLUDE = (system_absl_path,) | |
| ABSL_SOURCES = [ | |
| p.replace("third_party/abseil-cpp", system_absl_path) | |
| _system_absl_path = os.path.join("/usr", "include") | |
| ABSL_INCLUDE = (_system_absl_path,) | |
| ABSL_SOURCES = [ | |
| p.replace("third_party/abseil-cpp", _system_absl_path) |
Discussed f2f with @asheshvidyut
There was a problem hiding this comment.
Code Review
This pull request modifies setup.py to introduce an ABSL_SOURCES variable for managing Abseil source file paths, enabling conditional use of system-installed Abseil. A critical issue was identified: when building with system Abseil, the ABSL_SOURCES list should be cleared to prevent incorrect compilation of source files from the system include path, as the system library should be linked directly.
| ABSL_SOURCES = [ | ||
| p.replace("third_party/abseil-cpp", system_absl_path) | ||
| for p in ABSL_SOURCES | ||
| ] |
There was a problem hiding this comment.
When building with system Abseil, we should be linking against the pre-compiled system library, not compiling its source files. The current logic attempts to locate and compile an Abseil source file from the system include path (/usr/include), which is incorrect.
The ABSL_SOURCES list should be cleared when BUILD_WITH_SYSTEM_ABSL is enabled.
ABSL_SOURCES = []There was a problem hiding this comment.
Yes, this is a fair point.
Lines 161 to 164 in 4f623df
We don't expect sources to be present when the system absl is used, only headers.
sergiitk
left a comment
There was a problem hiding this comment.
Gemini caught something important
Fixes #41696