Feature/685 refactor manifest format #765

pnoltes · 2024-07-27T10:03:40Z

Refactor Manifest Format to JSON

This PR refactors the MANIFEST format to JSON. Consequently, the custom manifest parsing is removed, and the properties JSON format is now used.

Additional Changes

This PR has grown considerably larger than initially intended due to the following additional changes:

Deprecation Clean-Up: Some of the framework deprecated APIs have been moved to the framework src directory, and deprecated code that was no longer used has been removed. This cleanup primarily focused on bundle and module api.
Source File Updates: Many source files were updated, mostly to adjust include statements. In some cases, usage of deprecated bundle api has been replaced.

Manifest Attribute Definitions

Private Defines: Mandatory and recognized optional manifest attribute names are no longer defined in celix_constants.h. Instead, they are private defines within the bundle manifest source file.
New Functions: Functions were added to explicitly retrieve the values of mandatory or recognized optional manifest attributes.
Manifest Attribute Names: The used manifest attribute names has changed to be more aligned with the naming scheme of config properties.

CMake Function Names

The CMake function celix_bundle_headers remains unchanged, ensuring no breaking changes. i.e.:

celix_bundle_headers(my_bundle "Attribute1: Value1" "Attribute2: Value2")

still works.

This is the same approach was done for the container config properties (e.g., celix_container_runtime_properties and celix_container_embedded_properties are backwards compatible). However, because celix_bundle_headers is not as heavily used, it might be beneficial to introduce a new CMake function that accepts separated arguments for attribute names and values, such as:

celix_bundle_manifest(my_bundle "Attribute1" "Value1")

Bundle Packaging

Currently, a bundle zip can be created with jar if the jar executable is available. This was beneficial for ensuring the MANIFEST.MF file was the first file in the zip.
With the JSON format, this benefit is no longer relevant.
We can keep the support for using jar to package a bundle, but we could also drop jar support.

Future Work

After this PR is merged, my plan is to continue with #509 by:

Moving all deprecated API headers (except those used in remote services) to the framework src directory.
Renaming, refactoring, or removing the deprecated functions.

In a subsequent pull request, the remote services implementation will be refactored to remove the usage of the remaining deprecated APIs.

… some usage of deprecated api

…IFEST.MF

…r-manifest-format

…ormat

codecov-commenter · 2024-07-27T10:12:24Z

Codecov Report

Attention: Patch coverage is 96.72727% with 9 lines in your changes missing coverage. Please review.

Project coverage is 90.77%. Comparing base (bba867a) to head (d10a983).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
libs/framework/src/module.c	89.55%	7 Missing ⚠️
libs/framework/src/dm_dependency_manager_impl.c	66.66%	1 Missing ⚠️
libs/framework/src/framework.c	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
+ Coverage   90.27%   90.77%   +0.50%     
==========================================
  Files         226      226              
  Lines       26323    26604     +281     
==========================================
+ Hits        23762    24149     +387     
+ Misses       2561     2455     -106

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

PengZheng

Another nice cleanup of the module layer! I am glab to see the reduction of LOC.

Overall LGTM. There are only some minor issues that need fixing.
I also left some remarks about some further cleanup, which may or may not happen within this PR.

PengZheng · 2024-07-30T07:20:09Z

libs/utils/include/celix_err.h

@@ -94,6 +94,28 @@ CELIX_UTILS_EXPORT void celix_err_printErrors(FILE* stream, const char* prefix,
 */
 CELIX_UTILS_EXPORT int celix_err_dump(char* buf, size_t size, const char* prefix, const char* postfix);

+/*!


I think the two macros, which represent a very specific error handling style, does not belong to celix_err.h. Moreover, including them makes celix_err.h not self-contained any longer (missing errno.h).

Or we include celix_errno.h, or we have a dedicate header including all these error handling macros.

WDYT?

I just noticed that all usages of these macro are within celix_bundle_manifest.c. If we remove unnecessary duplication of manifest information (see below for more detail), then all these usages are not needed any more.

The marcro were in the wrong header, but will now be removed with the changes for #765 (comment)

PengZheng · 2024-07-30T07:25:38Z

libs/utils/include/celix_err.h

+    do {                                                                                                               \
+        if ((arg) == NULL) {                                                                                           \
+            celix_err_pushf("%s: Out of memory", __func__);                                                            \
+            return status;                                                                                             \


Suggested change

return status; \

return ENOMEM; \

Otherwise, all usages of this macros are incorrect.

PengZheng · 2024-07-30T08:47:52Z

libs/utils/include/celix_properties.h

@@ -450,7 +450,8 @@ CELIX_UTILS_EXPORT const celix_version_t* celix_properties_getVersion(const celi
 * @brief Get a value of a property as a copied Celix version.
 *
 * If the property value is a Celix version, a copy of the found version will be returned.
- * If the property value is a string, this function will attempt to convert it to a new Celix version.
+ * If the property value is present, but not a Celix version, this function will attempt to convert the property value


We need a test case for non-string value type.

PengZheng · 2024-07-30T08:51:20Z

libs/framework/include_deprecated/bundle_context.h


+#include "bundle_context.h"


Why include itself?

removed self include

PengZheng · 2024-08-02T06:36:16Z

libs/framework/src/bundle.c

-    }
+celix_status_t bundle_createModule(bundle_pt bundle, celix_module_t** moduleOut) {
+    celix_status_t status = CELIX_SUCCESS;
+    long bundleId = celix_bundle_getId(bundle);


Nice cleanup

PengZheng · 2024-08-02T14:25:57Z

libs/framework/src/module.c

-    char* symbolicName;
-    char* group;
-    char* name;
-    char* description;


Nice cleanup.

PengZheng · 2024-08-02T14:34:46Z

libs/framework/src/module.c


-            manifestParser_destroy(mp);
-        }
+celix_module_t* module_createFrameworkModule(celix_framework_t* fw, bundle_pt bundle) {


It seems identical to module_create so that we can combine them into one.

nicely spotted, I had not yet noticed that. Removed createFrameworkModule.

PengZheng · 2024-08-02T14:53:09Z

libs/framework/src/module.c

-        activator = manifest_getValue(manifest, CELIX_FRAMEWORK_BUNDLE_ACTIVATOR);
-
-        if (exportLibraries != NULL) {
-            status = CELIX_DO_IF(status, celix_module_loadLibrariesInManifestEntry(module, exportLibraries, activator, archive, &activatorHandle));


Now that we remove this and that we have already concluded export library does not work for C/C++, shall we remove it together with corresponding documentation and cmake commands?

PengZheng · 2024-08-02T15:06:21Z

cmake/cmake_celix/templates/MANIFEST.json.in

+    >$<$<BOOL:$<TARGET_PROPERTY:${BUNDLE_TARGET_NAME},BUNDLE_HEADERS>>:$<COMMA>>
+    "CELIX_BUNDLE_MANIFEST_VERSION" : "2.0.0",
+    "CELIX_BUNDLE_SYMBOLIC_NAME" : "$<TARGET_PROPERTY:${BUNDLE_TARGET_NAME},BUNDLE_SYMBOLIC_NAME>",
+    "CELIX_BUNDLE_VERSION" : "$<TARGET_PROPERTY:${BUNDLE_TARGET_NAME},BUNDLE_VERSION>",


To make strict typing work, this need to be a version string (celix_properties_isVersionString).

PengZheng · 2024-08-02T15:07:03Z

cmake/cmake_celix/templates/MANIFEST.json.in

+{
+    $<JOIN:$<TARGET_PROPERTY:${BUNDLE_TARGET_NAME},BUNDLE_HEADERS>,$<COMMA>
+    >$<$<BOOL:$<TARGET_PROPERTY:${BUNDLE_TARGET_NAME},BUNDLE_HEADERS>>:$<COMMA>>
+    "CELIX_BUNDLE_MANIFEST_VERSION" : "2.0.0",


Need to be a version string.

pnoltes · 2024-10-06T17:39:55Z

I’ve been busy with other commitments, but I plan to pick this up again in the coming weeks. Thanks for the patience!

pnoltes · 2024-11-10T18:54:33Z

I updated the PR and processed al comments, except the remark that the revision can be removed. That last one I would like to do in separate PR.

pnoltes added 18 commits June 8, 2024 20:37

gh-685: Add new manifest impl

afc02be

gh-685: Add manifest validation

a409230

gh-685: Add bundle manifest getter functions

f79f229

gh-685: Update usage of manifest to celix_bundle_manifest and removes…

3759c1d

… some usage of deprecated api

gh-685: Update bundle zip generation for MANIFEST.json instead of MAN…

c44bbe6

…IFEST.MF

gh-685: Add cmake generated celix_framework_version.h

4678197

gh-685: Refactor bundle manifest storage

26d14ff

Merge remote-tracking branch 'origin/master' into feature/685-refacto…

203cbfc

…r-manifest-format

gh-685: Add missing includes

75aafbc

gh-685: Add additional manifest tests

7b4b0cc

Merge branch 'refs/heads/master' into feature/685-refactor-manifest-f…

89bafcb

…ormat

gh-685: Removing manifest attribute name from celix_constants.h

4822eed

gh-685: Remove commented out code

150c3dc

gh-685: Fix tests which were broken after the manifest format refactor

a123d96

gh-685: Add celix_bundle_getVersion error injection wrapper

28ee4a1

gh-685: Add missing sys/select.h include

297a437

gh-685: Remove unused deprecated bundle api

dc90776

gh-685: Adjust documentation for MANIFEST.MF to MANIFEST.json refactor

3048050

PengZheng self-requested a review July 28, 2024 03:16

PengZheng requested changes Aug 2, 2024

View reviewed changes

gh-685: Improve manifest json impl based on review comments

d10a983

pnoltes requested a review from PengZheng November 10, 2024 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/685 refactor manifest format #765

Feature/685 refactor manifest format #765

pnoltes commented Jul 27, 2024

codecov-commenter commented Jul 27, 2024 •

edited

Loading

PengZheng left a comment

PengZheng Jul 30, 2024

PengZheng Aug 2, 2024 •

edited

Loading

pnoltes Nov 10, 2024

PengZheng Jul 30, 2024

PengZheng Jul 30, 2024

PengZheng Jul 30, 2024

pnoltes Nov 10, 2024

PengZheng Aug 2, 2024

PengZheng Aug 2, 2024

PengZheng Aug 2, 2024

pnoltes Nov 10, 2024 •

edited

Loading

PengZheng Aug 2, 2024

PengZheng Aug 2, 2024

PengZheng Aug 2, 2024

pnoltes commented Oct 6, 2024

pnoltes commented Nov 10, 2024


		#include "bundle_context.h"

Feature/685 refactor manifest format #765

Are you sure you want to change the base?

Feature/685 refactor manifest format #765

Conversation

pnoltes commented Jul 27, 2024

Refactor Manifest Format to JSON

Additional Changes

Manifest Attribute Definitions

CMake Function Names

Bundle Packaging

Future Work

codecov-commenter commented Jul 27, 2024 • edited Loading

Codecov Report

PengZheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PengZheng Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnoltes Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnoltes commented Oct 6, 2024

pnoltes commented Nov 10, 2024

codecov-commenter commented Jul 27, 2024 •

edited

Loading

PengZheng Aug 2, 2024 •

edited

Loading

pnoltes Nov 10, 2024 •

edited

Loading