Introducing report_running()#239
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
| @@ -0,0 +1,23 @@ | |||
| /******************************************************************************** | |||
| * Copyright (c) 2025 Contributors to the Eclipse Foundation | |||
There was a problem hiding this comment.
Maybe new code should use 2026 copyright year
| tags = ["FUSA"], | ||
| deps = [ | ||
| ":lifecycle_client", | ||
| "@score_baselibs//score/mw/log", |
There was a problem hiding this comment.
I don't see a direct dependency to mw::log in the report_running impl.
|
|
||
| #include "score/mw/lifecycle/lifecycle_client/report_running.h" | ||
|
|
||
| #include "score/mw/log/logging.h" |
There was a problem hiding this comment.
header to be removed
| #include "score/mw/lifecycle/lifecycle_client/report_running.h" | ||
|
|
||
| #include "score/mw/log/logging.h" | ||
| #include <score/mw/lifecycle/lifecycle_client.h> |
There was a problem hiding this comment.
Should this be:
| #include <score/mw/lifecycle/lifecycle_client.h> | |
| #include "score/mw/lifecycle/lifecycle_client.h" |
| srcs = ["src/report_running.cpp"], | ||
| hdrs = ["src/report_running.h"], | ||
| features = COMPILER_WARNING_FEATURES, | ||
| include_prefix = "score/mw/lifecycle/lifecycle_client", |
There was a problem hiding this comment.
Should this be ?
| include_prefix = "score/mw/lifecycle/lifecycle_client", | |
| include_prefix = "score/mw/lifecycle", |
Consumer should include the file via
#include "score/mw/lifecycle/report_running.h"
| features = COMPILER_WARNING_FEATURES, | ||
| include_prefix = "score/mw/lifecycle/lifecycle_client", | ||
| strip_include_prefix = "/score/launch_manager/lifecycle_client/src", | ||
| visibility = ["//visibility:public"], |
There was a problem hiding this comment.
I think this should be restricted visibility
| visibility = ["//visibility:public"], | |
| visibility = ["//score:__subpackages__"], |
only the targets in score/launch_manager/BUILD are the public targets.
If I remember correctly, the report_running should just become part of the lifecycle_cc target
250bb36 to
9a9be83
Compare
| ], | ||
| ) | ||
|
|
||
| filegroup( |
There was a problem hiding this comment.
We should check if this can be removed. I don't see any usage
There was a problem hiding this comment.
Yes, can be removed.
There was a problem hiding this comment.
We should check if this entire thing can be removed. I don't see any usage of it.
There was a problem hiding this comment.
As github can not show me to which line of code this comment belongs, please check yourself if this has been fixed.
There was a problem hiding this comment.
I wonder why there is a target that compiles again all these sources that already exist in individual libraries.
Ideally, there are individual private libraries and one target that only lists them as dependencies
There was a problem hiding this comment.
Yes. This was needed in first place to build the rust supervised app, however as the rust supervised app only uses the old internal lifecycle client, I could adapt it accordingly. Now it is no longer needed.
681c575 to
40c0ae2
Compare
| @@ -32,21 +32,6 @@ cc_library( | |||
|
|
|||
| cc_library( | |||
There was a problem hiding this comment.
This still exposes multiple includes paths.
For client code both include paths are working:
#include <score/mw/lifecycle/lifecycle_client/application.h>
#include <score/mw/lifecycle/lifecycle_client/runapplication.h>
#include <score/mw/lifecycle/application.h>
#include <score/mw/lifecycle/runapplication.h>Can we restrict this only to the latter one?
| use std::ffi::CString; | ||
| use std::marker::PhantomData; | ||
|
|
||
| #[link(name = "lifecycle_client")] |
There was a problem hiding this comment.
should this be corrected instead of being removed completely?
Introduces a low-level
void report_running()function so applications can report a Running state without requiring usage of the full mw::Lifecycle API.This was decided in one of the weekly lifecycle meetings and documented in ticket #140
This PR introduces the API.
Follow-up PR will then remove the existing LifecycleClient class from public visibility.