Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/refactor bundle cache #476

Merged
merged 103 commits into from
Apr 6, 2023
Merged

Feature/refactor bundle cache #476

merged 103 commits into from
Apr 6, 2023

Conversation

pnoltes
Copy link
Contributor

@pnoltes pnoltes commented Feb 1, 2023

This PR refactors the bundle cache handling so that bundles zips are not extracted if there is already a cache dir for that bundle.

This PR does not implement or replace the ideas mentioned in #441, but instead is just a step to make extracted bundle cache dirs reusable.

The bundle archive, bundle cache and celix launcher functionality is tested with additional gtest code.

For the bundle update functionality, no additional test are added and IMO this functionality is still "work in progress" / "use at own risk".
Mainly because IMO we need to test bundle update with really different bundle libs (including new and removed symbols) to ensure that a dlopen/dlsym on the same path, but different libs really works as expected. I considered this out of scope for this PR.

This PR also introduces:

  • Split bundle cache in a resource cache and storage cache and introduces a celix_bundle_getDataFile next to celix_bundle_getEntry, specifically to access files from the bundle storage instead of the bundle resource cache. This is more conform the OSGi spec, see https://docs.osgi.org/specification/osgi.core/8.0.0./framework.api.html#org.osgi.framework.Bundle getEntry and getDataFile.
  • Remove usage of -Og, because -Og actual does not really work when debugging (optimized out results).
  • Introduce some additional utils functions for string to long,bool,etc conversion, file touch and get last modified from file functions.
  • Move (but do not change) the lib loading functionality from bundle archive to (bundle) module.
  • Refactor some cpputest utils test to gtest.
  • Small refactoring in start, stop and update shell command.
  • Refactor http_admin to use the storage cache instead of resource cache for the html files handling.
  • Add support for --create-bundle-cache option in the celix launcher to create a bundle cache and exit (i.e. prepare a application to startup more quickly).

This PR has become large than I expected and also took more time than I expected. I think this is mainly, because I had some trouble understanding the bundle archive, revision mechanism and added quite some refactoring to (IMO) make this a bit cleaner. I would have preferred some more refactoring, but that breaks the current public api.

@PengZheng
Copy link
Contributor

Thanks for making this happen. Please give me one or two weeks to review it.

@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2023

Codecov Report

Merging #476 (1802798) into master (42e9669) will decrease coverage by 0.95%.
The diff coverage is 80.72%.

❗ Current head 1802798 differs from pull request most recent head 0f0ab31. Consider uploading reports for the commit 0f0ab31 to get more accurate results

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
- Coverage   76.48%   75.54%   -0.95%     
==========================================
  Files         226      225       -1     
  Lines       34133    34439     +306     
==========================================
- Hits        26108    26016      -92     
- Misses       8025     8423     +398     
Impacted Files Coverage Δ
...undles/http_admin/http_admin/src/websocket_admin.c 91.86% <ø> (-1.00%) ⬇️
bundles/shell/shell/src/std_commands.c 100.00% <ø> (ø)
libs/framework/include/celix/Exception.h 0.00% <0.00%> (ø)
libs/framework/src/manifest.c 44.93% <12.50%> (-4.72%) ⬇️
libs/framework/include/celix/FrameworkUtils.h 84.21% <40.00%> (-15.79%) ⬇️
...framework/src/framework_bundle_lifecycle_handler.c 55.67% <45.00%> (-15.42%) ⬇️
bundles/shell/shell/src/update_command.c 50.00% <50.00%> (+20.68%) ⬆️
bundles/http_admin/http_admin/src/activator.c 82.60% <52.63%> (-8.62%) ⬇️
libs/framework/src/bundle_revision.c 61.70% <60.86%> (-23.30%) ⬇️
libs/framework/src/framework.c 77.68% <63.27%> (-3.15%) ⬇️
... and 26 more

... and 155 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

PengZheng and others added 7 commits February 7, 2023 15:08
# Conflicts:
#	bundles/http_admin/http_admin/CMakeLists.txt
Also introduce an error injector lib for celix_utils
Also introduces the CELIX_EI_IMPL macro which returns the configured return type of the wrapped
function and adds some mimimal documentation about the bundle cache.
Also framework documentation for the bundle cache.
…ndle_cache

# Conflicts:
#	libs/framework/CMakeLists.txt
@PengZheng PengZheng self-requested a review February 20, 2023 03:02
Copy link
Contributor

@PengZheng PengZheng left a comment

Choose a reason for hiding this comment

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

I'm still looking at this PR, leaving some partial comments here.

bundles/deployment_admin/src/deployment_admin.c Outdated Show resolved Hide resolved
bundles/http_admin/http_admin/src/activator.c Outdated Show resolved Hide resolved
bundles/http_admin/http_admin/src/activator.c Outdated Show resolved Hide resolved
bundles/http_admin/http_admin/src/activator.c Show resolved Hide resolved
bundles/http_admin/http_admin/src/http_admin.c Outdated Show resolved Hide resolved
bundles/shell/shell/src/install_command.c Show resolved Hide resolved
bundles/shell/shell/src/start_command.c Show resolved Hide resolved
@PengZheng PengZheng mentioned this pull request Feb 21, 2023
This was linked to issues Apr 3, 2023
@PengZheng PengZheng mentioned this pull request Apr 4, 2023
42 tasks
@pnoltes pnoltes requested a review from PengZheng April 6, 2023 13:03
@pnoltes
Copy link
Contributor Author

pnoltes commented Apr 6, 2023

I would like to move towards merging this PR.

I am quite happy with the overall changes this PR introduces. This includes the bundle cache changes, but certainly also the error handling and file utils improvements and I think it is good to merge this in master.

@PengZheng and @stegemr: Are there any other changes that needs to be done and if so, can this be done in a separate PR? I prefer a separate PR also to keep overview of the changes more manageable.

Copy link
Contributor

@PengZheng PengZheng left a comment

Choose a reason for hiding this comment

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

I apologize for the terribly slow review.
We can merge it now, but please wait a week or two before making a new release.

@pnoltes
Copy link
Contributor Author

pnoltes commented Apr 6, 2023

I apologize for the terribly slow review.
We can merge it now, but please wait a week or two before making a new release.

No worries, as said I am quite happy with the - joined effort - changes. But I think is time now to merge this and move forward in - if possible - smaller PRs.

Also no problem to wait a a week (or longer) for a new release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants