repo: rework fix for logging build issues#105
Conversation
- Use `logging` where applicable. - Revert previous (partially invalid) change. - Fix configuration in `.bazelrc`.
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
@ramceb |
|
The created documentation from the pull request is available at: docu-html |
There was a problem hiding this comment.
Pull request overview
This PR aims to address Bazel build issues related to score/mw/log by reworking which external Bazel module provides the logging dependency and by adjusting the .bazelrc configuration used in the repo’s standard build configs.
Changes:
- Switched multiple C++ Bazel targets’ deps from
@score_logging//score/mw/logto@score_baselibs//score/mw/log. - Updated
.bazelrcstub config to also setKRemote_Logging=Falsefor the@score_baselibs//score/mw/logflag target (and removed the prior globaltestflag line for@score_logging). - Added an explicit
@score_logging//score/mw/logdep to theexamples/cpp_lifecycle_appbinary.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/lifecycle_client_lib/BUILD |
Swaps logging dep from @score_logging to @score_baselibs. |
src/launch_manager_daemon/process_state_client_lib/BUILD |
Swaps logging dep in process_state_client to @score_baselibs. |
src/launch_manager_daemon/lifecycle_client_lib/BUILD |
Swaps logging dep from @score_logging to @score_baselibs. |
src/launch_manager_daemon/health_monitor_lib/BUILD |
Swaps multiple logging deps from @score_logging to @score_baselibs. |
src/launch_manager_daemon/BUILD |
Swaps logging dep from @score_logging to @score_baselibs. |
src/control_client_lib/BUILD |
Swaps logging dep from @score_logging to @score_baselibs. |
examples/cpp_lifecycle_app/BUILD |
Adds explicit logging dep (currently @score_logging//score/mw/log). |
.bazelrc |
Moves/extends remote logging disablement for stub config to include @score_baselibs logging flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| "//src/lifecycle_client_lib", | ||
| "@score_logging//score/mw/log", |
There was a problem hiding this comment.
cpp_lifecycle_app now directly depends on @score_logging//score/mw/log while //src/lifecycle_client_lib already pulls in @score_baselibs//score/mw/log transitively. Unless the example truly needs both implementations, this extra dep is likely redundant and may create inconsistent linking/flags compared to the rest of the repo. Prefer relying on the transitive dep or switching the example to the same @score_baselibs//score/mw/log target used elsewhere (whichever is intended).
| "@score_logging//score/mw/log", |
| deps = [ | ||
| "//externals/ipc_dropin", | ||
| "//src/launch_manager_daemon/common", | ||
| "//src/launch_manager_daemon/common:identifier_hash", | ||
| "//src/launch_manager_daemon/common:lifecycle_error", | ||
| "//src/launch_manager_daemon/common:log", | ||
| "@score_baselibs//score/mw/log", | ||
| "@score_baselibs//score/result", | ||
| "@score_logging//score/mw/log", | ||
| ], |
There was a problem hiding this comment.
process_state_client now depends on @score_baselibs//score/mw/log, but the headers target still omits it even though the comment says its deps must match process_state_client. This can break builds that compile headers-only consumers (especially when LC_LOG_SCORE_MW_LOG is enabled) and makes the two targets diverge. Add the same @score_baselibs//score/mw/log dep to the headers rule (or adjust the comment/structure so the invariant stays true).
| deps = [ | ||
| "//src/launch_manager_daemon/lifecycle_client_lib:lifecycle_client", | ||
| "@score_baselibs//score/language/futurecpp", | ||
| "@score_baselibs//score/memory:string_literal", | ||
| "@score_baselibs//score/mw/log", | ||
| "@score_baselibs//score/os:stdlib", | ||
| "@score_baselibs//score/os/utils:signal", |
There was a problem hiding this comment.
The PR description says "Use logging where applicable", but this BUILD file switches deps from @score_logging//score/mw/log to @score_baselibs//score/mw/log. At the same time, the example app adds @score_logging//score/mw/log. Please reconcile which repository should provide score/mw/log in this repo (and update either the description or the deps) to avoid inconsistent linkage/configuration across targets.
loggingwhere applicable..bazelrc.