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/509 remove install of v2 headers #666

Merged
merged 16 commits into from
Oct 8, 2023

Conversation

pnoltes
Copy link
Contributor

@pnoltes pnoltes commented Oct 1, 2023

This PR removes the installation of the deprecated headers (those without the celix_ prefix) from the utils and framework libraries.
This is a task in #509.

Additionally, the PR removes the installation and usage of the v2 headers for the shell_api, log_service_api, and pubsub_spi libraries.
As a consequence, the shell_bonjour and psa_udp_mc bundles, which relied respectively on the v2 shell_api and v2 pubsub serializer api and both hasn't been maintained, are also removed.

Lastly, the remote service admin bundles/libraries have been using the service_reference and hashmap in their API/SPI. As a result, they are not currently usable downstream. Addressing this requires a significant effort; thus, for this PR, I've disabled the Conan test package test for RSA. This will be addressed in a future PR.

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2023

Codecov Report

Merging #666 (e38ee62) into master (d61be60) will increase coverage by 1.45%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
+ Coverage   81.55%   83.01%   +1.45%     
==========================================
  Files         260      252       -8     
  Lines       34677    32789    -1888     
==========================================
- Hits        28281    27219    -1062     
+ Misses       6396     5570     -826     
Files Coverage Δ
...les/pubsub/pubsub_admin_tcp/src/pubsub_tcp_admin.c 50.50% <ø> (ø)
...b/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c 68.95% <ø> (ø)
...sub/pubsub_admin_tcp/src/pubsub_tcp_topic_sender.c 84.01% <ø> (ø)
.../pubsub/pubsub_admin_websocket/src/psa_activator.c 100.00% <ø> (ø)
...ubsub_admin_websocket/src/pubsub_websocket_admin.c 42.47% <ø> (ø)
...in_websocket/src/pubsub_websocket_topic_receiver.c 67.56% <ø> (ø)
...dmin_websocket/src/pubsub_websocket_topic_sender.c 83.22% <ø> (-0.68%) ⬇️
...les/pubsub/pubsub_admin_zmq/src/pubsub_zmq_admin.c 52.96% <ø> (ø)
...b/pubsub_admin_zmq/src/pubsub_zmq_topic_receiver.c 76.71% <ø> (ø)
...sub/pubsub_admin_zmq/src/pubsub_zmq_topic_sender.c 84.05% <ø> (ø)
... and 5 more

... and 2 files with indirect coverage changes

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

@pnoltes pnoltes linked an issue Oct 1, 2023 that may be closed by this pull request
42 tasks
@PengZheng PengZheng self-requested a review October 6, 2023 08:44
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 used Celix in my day job. IIRC, we used some deprecated APIs for some reasons.
Before merging this PR, I would like to have a case-by-case analysis of these usages to make sure our new API can still satisfy these needs. For example, why service_reference is needed for RSA? Without service_reference, how to implement the same functionality with 3.0 API? More deprecated API usage scenarios will be added next week.

If the current 3.0 API can not address some requirements, it is a good chance to have some discussions before actually removing deprecated 2.0 APIs.

@xuzhenbao Please have a look at the RSA part.

@PengZheng PengZheng requested a review from xuzhenbao October 6, 2023 09:50
@pnoltes
Copy link
Contributor Author

pnoltes commented Oct 7, 2023

If the current 3.0 API can not address some requirements, it is a good chance to have some discussions before actually removing deprecated 2.0 APIs.

I understand. My proposal is to reintroduce the framework v2 headers install and RSA conan create test in PR.
We can then discuss the removal of service_reference, etc and the future of RSA, before removing all v2 headers.

@PengZheng
Copy link
Contributor

I understand. My proposal is to reintroduce the framework v2 headers install and RSA conan create test in PR.
We can then discuss the removal of service_reference, etc and the future of RSA, before removing all v2 headers.

It's a good idea to temporarily reintroduce framework v2 headers.

Now that 2.4.0 is available, I am working on introducing it into my day job projects.
After that I will be able to provide precise feedback about the current usage of deprecated APIs in these projects.

@pnoltes pnoltes requested a review from PengZheng October 7, 2023 21:00
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.

LGTM

@pnoltes
Copy link
Contributor Author

pnoltes commented Oct 8, 2023

@xuzhenbao Please have a look at the RSA part.

Given that the removal of the deprecated headers and therefore the refactor RSA will be done later, I will merge this PR.

@pnoltes pnoltes merged commit f8e95a5 into master Oct 8, 2023
28 checks passed
@pnoltes pnoltes deleted the feature/509-remove-install-of-v2-headers branch October 8, 2023 15:15
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.

Prepare Celix 3.0.0 release
3 participants