bazel: stub InitRunFiles, fix RUNFILES_* env leak in tcl_library_init#10100
bazel: stub InitRunFiles, fix RUNFILES_* env leak in tcl_library_init#10100oharboe wants to merge 5 commits intoThe-OpenROAD-Project:masterfrom
Conversation
InitRunFiles.cpp used a global constructor to set TCL_LIBRARY via Bazel runfiles -- the same job tcl_library_init.cc already does with a proper Tcl interp. Stub the file (kept for src/sta compatibility) and drop its deps. Also clear inherited RUNFILES_DIR and RUNFILES_MANIFEST_FILE before Runfiles::Create() in tcl_library_init.cc. When OpenROAD is invoked by a build system that previously ran another Bazel binary, those env vars point to the wrong runfiles tree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@hzeller Thoughts? |
![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 stubs out the legacy runfiles library and migrates Tcl initialization logic to bazel/tcl_library_init.cc. It also introduces logic to unset RUNFILES_DIR and RUNFILES_MANIFEST_FILE environment variables to ensure correct runfiles resolution when the binary is invoked by other Bazel processes. A portability issue was identified in bazel/tcl_library_init.cc where the use of unsetenv will cause compilation errors on Windows; a suggestion was provided to use platform-specific guards and _putenv_s for MSVC compatibility.
|
clang-tidy review says "All clean, LGTM! 👍" |
Use _putenv_s on Windows instead of POSIX unsetenv to clear RUNFILES_DIR and RUNFILES_MANIFEST_FILE. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
/gemini review |
|
#10095 already removes the runfile stuff, so don't do this here to avoid merge conflicts. |
| #else | ||
| using rules_cc::cc::runfiles::Runfiles; | ||
|
|
||
| // Clear inherited RUNFILES_* env vars: when OpenROAD is invoked by |
There was a problem hiding this comment.
Is there a test that can show that this is necessary ?
There was a problem hiding this comment.
Code Review
This pull request stubs out the runfiles library and migrates Tcl initialization logic to bazel/tcl_library_init.cc. The implementation now clears RUNFILES_DIR and RUNFILES_MANIFEST_FILE environment variables to ensure Runfiles::Create correctly resolves paths based on the executable location. I have no feedback to provide.
When OpenROAD is invoked by a Bazel-built process (e.g. a Python wrapper), the inherited RUNFILES_DIR points to the caller's runfiles tree. Without the unsetenv() fix in tcl_library_init.cc, Runfiles::Create() resolves paths in the wrong tree and OpenROAD fails to find its TCL library. This sh_test reproduces the scenario: as a Bazel test it has RUNFILES_DIR set to its own runfiles, then invokes OpenROAD which must find its own TCL library despite the misleading env var. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
The previous test used -version which exits before Tcl initialization, so it could never detect the RUNFILES_DIR env leak. Fix the test to: - Set RUNFILES_DIR to an empty temp dir (simulating cross-binary leak) - Run a real Tcl command (-no_splash -exit) that exercises tcl_library_init - Check for "application-specific initialization failed" in output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
InitRunFiles.cpp used a global constructor to set TCL_LIBRARY via Bazel runfiles -- the same job tcl_library_init.cc already does with a proper Tcl interp. Stub the file (kept for src/sta compatibility) and drop its deps.
Also clear inherited RUNFILES_DIR and RUNFILES_MANIFEST_FILE before Runfiles::Create() in tcl_library_init.cc. When OpenROAD is invoked by a build system that previously ran another Bazel binary, those env vars point to the wrong runfiles tree.