We're not using runfiles anymore, don't use associated macro define. - #371
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 replaces the preprocessor macro BAZEL_CURRENT_REPOSITORY with BAZEL_BUILD in app/Main.cc and defines BAZEL_BUILD in the BUILD file. The reviewer notes that @rules_cc//cc/runfiles is still listed in the dependencies of the binary and should be removed to complete the cleanup as described in the pull request.
7bb4d4e to
c08d761
Compare
|
Can this be merged ? |
|
Ping. |
|
did this fall under the radar ? |
|
Regular ping :) |
|
Maybe 'pong' helps ? |
|
Regular ping... |
c08d761 to
379c5c3
Compare
|
taking a look right now! |
dsengupta0628
left a comment
dsengupta0628
left a comment
There was a problem hiding this comment.
Correctness ✅
- Root reason sound: remove runfiles dependency--> BAZEL_CURRENT_REPOSITORY vanishes --> guarded blocks silently drop out (readline + tcl env setup lost in bazel build). Explicit BAZEL_BUILD define restores intent.
- All 3 #ifdef sites flipped consistently. No stray BAZEL_CURRENT_REPOSITORY left.
- @rules_cc//cc/runfiles referenced only at this one dep line — safe to remove.
- Transitive deps //bazel:tcl_library_init + //:tcl_readline_setup still present, so headers bazel/tcl_library_init.h / src/tcl_readline_setup.h keep their own build inputs. Binary-level
runfiles dep was serving only the macro. Clean.
Style / conventions ✅
- BAZEL_BUILD clearer intent than repurposing a toolchain-internal macro. This is good as BAZEL_CURRENT_REPOSITORY was never meant as a build-mode flag, just happened to exist.
- Matches OpenSTA bazel patterns.
Signed-off-by: James Cherry <cherry@parallaxsw.com>
|
@maliberty I guess you need to add your write-access enabled approve as well. |
|
@codex review |
|
Merge conflict (BUILD renamed) |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
We used to use runfiles to work with tcl dependencies. Since we don't need that anymore, just add a regular `BAZEL_BUILD` define to choose the behavior in the bazel build and remove the unneeded dependency. Signed-off-by: Henner Zeller <h.zeller@acm.org>
379c5c3 to
685f006
Compare
|
Rebased. |
3 participants
We used to use runfiles to work with tcl dependencies. Since we don't need that anymore, just add a regular
BAZEL_BUILDdefine to choose the behavior in the bazel build and remove the unneeded dependency.