Skip to content

Conversation

@joshualicht
Copy link
Contributor

@joshualicht joshualicht commented Jul 30, 2025

This PR contains:

  • Namespace introduction (score::mw::per::kvs)
  • Renaming of MyErrorCode to ErrorCode
  • Split Files: Implementation and Unitests
  • Add Benchmark to workflow (e.g. to warn if compiling errors take place in the benchmark file)

Coverage Report: coverage.zip

Add score::mw::pers::kvs as a namespace for cpp implementation
Rename provisional "MyErrorCode" to "ErrorCode"
Split up files for a better code overview
@github-actions
Copy link

github-actions bot commented Jul 30, 2025

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output
[License Check Output]
2025/08/05 13:50:03 Downloading https://releases.bazel.build/7.4.0/release/bazel-7.4.0-linux-x86_64...
Extracting Bazel installation...
Starting local Bazel server and connecting to it...
INFO: Invocation ID: 7ba27b01-4aae-4ea6-86cf-7d5993b57f97
Computing main repo mapping: 
Computing main repo mapping: 
DEBUG: Rule 'rules_boost~' indicated that a canonical reproducible form can be obtained by modifying arguments integrity = "sha256-aSViuHoOdF32Ue/Cx02/mOzyEebHMFuifjZs4GNWjsE="
DEBUG: Repository rules_boost~ instantiated at:
  <builtin>: in <toplevel>
Repository rule http_archive defined at:
  /home/runner/.bazel/external/bazel_tools/tools/build_defs/repo/http.bzl:387:31: in <toplevel>
Computing main repo mapping: 
WARNING: For repository 'googletest', the root module requires module version googletest@1.14.0, but got googletest@1.15.0 in the resolved dependency graph.
WARNING: For repository 'aspect_rules_lint', the root module requires module version aspect_rules_lint@1.0.3, but got aspect_rules_lint@1.4.2 in the resolved dependency graph.
Computing main repo mapping: 
Loading: 
Loading: 1 packages loaded
Analyzing: target //:license-check (2 packages loaded, 0 targets configured)
Analyzing: target //:license-check (2 packages loaded, 0 targets configured)

Analyzing: target //:license-check (55 packages loaded, 10 targets configured)

Analyzing: target //:license-check (102 packages loaded, 10 targets configured)

Analyzing: target //:license-check (147 packages loaded, 922 targets configured)

Analyzing: target //:license-check (165 packages loaded, 1948 targets configured)

Analyzing: target //:license-check (165 packages loaded, 2810 targets configured)

Analyzing: target //:license-check (165 packages loaded, 2810 targets configured)

Analyzing: target //:license-check (169 packages loaded, 4940 targets configured)

Analyzing: target //:license-check (170 packages loaded, 5060 targets configured)

Analyzing: target //:license-check (170 packages loaded, 5060 targets configured)

INFO: Analyzed target //:license-check (171 packages loaded, 6979 targets configured).
[8 / 13] Creating runfiles tree bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/score_dash_license_checker~/tool/formatters/dash_format_converter.runfiles [for tool]; 0s local ... (2 actions, 1 running)
ERROR: /home/runner/work/inc_mw_per/inc_mw_per/BUILD:36:21: Generating Dash formatted dependency file ... failed: missing input file '//:cargo_lock'
ERROR: /home/runner/work/inc_mw_per/inc_mw_per/BUILD:36:21: Generating Dash formatted dependency file ... failed: 1 input file(s) do not exist
Target //:license.check.license_check failed to build
Use --verbose_failures to see the command lines of failed build steps.
ERROR: /home/runner/work/inc_mw_per/inc_mw_per/BUILD:36:21 Generating Dash formatted dependency file ... failed: 1 input file(s) do not exist
INFO: Elapsed time: 188.003s, Critical Path: 0.39s
INFO: 12 processes: 2 disk cache hit, 10 internal.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

@github-actions
Copy link

The created documentation from the pull request is available at: docu-html

Update BUILD Files and smaller changes
Split Code-Files for Unittests + Add kvs_check_hash tests
add benchmark to workflow (e.g. to show compiler errors)
@joshualicht joshualicht marked this pull request as ready for review July 31, 2025 10:00
@joshualicht joshualicht requested a review from umaucher July 31, 2025 11:48
Update namespace definition to C++17
Rename Extract Coverage for "kvs.cpp" to "CPP Files"
add namespace to example usage
rename namespace from score::mw::pers:kvs to score::mw::per:kvs
********************************************************************************/
#include "error.hpp"

namespace score::mw::per::kvs {
Copy link
Contributor

@vinodreddy-g vinodreddy-g Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the split of this internal folder for?

Copy link
Contributor Author

@joshualicht joshualicht Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to replicate the folder structures of the baselibs components. E.g. JSON Component: Here we also have error files in an internal folder, but they still are exposed due to bazel deps.
I dont use an internal namespace for error, because if an application wants to use the kvs errorcode (e.g. while checking for an specific errorcode after a failed operation), you would always need to write an internal namespace for this, what is (at least in my understanding) not quite ideal. Json for example also doesnt have an impl namespace for errorcodes error.h json)
For "real" internal-only operations (like kvs_helper) we can align on the usage of an internal namespace, but since this change only affects the kvs itself and is no interface change, i personally would do this in a different PR and get this merged beforehand.
Also in the last FT Meeting we talked about the structure of internal files, because error is only semi internal since it is (as you said) exposed. So the question was, if the error files should be in the internal folder or directly in the src folder. We aligned, that this question should be taken in Architecture Community and we proceed like the way its currently done in Baselibs (and in this PR).

Copy link
Contributor

@vinodreddy-g vinodreddy-g Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO
,Error should not be in internal folder . For others if you have a internal folder then it should mean that these are internal and not for public user right.

Copy link
Contributor Author

@joshualicht joshualicht Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we also thought this. But then on the other hand in more or less every baselibs component error is part of the internal folder. Thats why we moved this question to Architecture Community and change it in persistency afterwards if needed.

@vinodreddy-g vinodreddy-g merged commit 613e69b into eclipse-score:main Aug 6, 2025
11 checks passed
atarekra pushed a commit to Valeo-S-CORE-Organization/persistency that referenced this pull request Nov 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants