You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2022/12/04 18:58:01 UTC
[GitHub] [celix] pnoltes opened a new pull request, #451: Feature/split celix cxx option
pnoltes opened a new pull request, #451:
URL: https://github.com/apache/celix/pull/451
This PR splits up the CELIX_CXX into a CELIX_CXX14 and CELIX_CXX17 option.
The default C++ language is also updated to C++14 to ensure libs/bundles using C++ are default complaint with C++14.
Also:
- Fix an uncaught exception coverity issue (CID 236044) by making the promise::resolve promise::fail implementation not thrown an exception if the promise is already done. Note that throwing an exception is part of the OSGi specification, but IMO throwing exception for a lib that should make multi threading programming more easy is not a good design. For the Celix implementation the tryFail and tryResolve can be used to atomically try to fail/resolve a promise and check whether it was already done.
- Remove unused/dead C++ RSA Topology Manager code. If this is needed, the code can be reintroduced using git history.
-Refactor the C++ RSA integration test. Originally the promise was tested first, but I think there was a race condition that this could fail if the pubsub connection was not fully established yet. The remote promise call in now retried a couple of times. This should fix #397
- Fix cv-qualifiers issue with template deduction for the Promise class.
- Made C++ RSA default on. IMO the C++ RSA is proven to be stable enough (in my day-time job).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a diff in pull request #451: Feature/split celix cxx option
Posted by GitBox <gi...@apache.org>.
pnoltes commented on code in PR #451:
URL: https://github.com/apache/celix/pull/451#discussion_r1045278039
##########
libs/framework/gtest/CMakeLists.txt:
##########
@@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
correct, added the check in the framework CMakeLists.txt
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on pull request #451: Feature/split celix cxx option
Posted by GitBox <gi...@apache.org>.
pnoltes commented on PR #451:
URL: https://github.com/apache/celix/pull/451#issuecomment-1345625507
Note that I revert the changes for #397, this is done in a different PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] codecov-commenter commented on pull request #451: Feature/split celix cxx option
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #451:
URL: https://github.com/apache/celix/pull/451#issuecomment-1336493620
# [Codecov](https://codecov.io/gh/apache/celix/pull/451?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#451](https://codecov.io/gh/apache/celix/pull/451?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (66b4ec8) into [master](https://codecov.io/gh/apache/celix/commit/b371d7163ad36aed9e46f8f5578ff8f733f179f5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b371d71) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 66b4ec8 differs from pull request most recent head 6208146. Consider uploading reports for the commit 6208146 to get more accurate results
```diff
@@ Coverage Diff @@
## master #451 +/- ##
==========================================
+ Coverage 74.52% 74.54% +0.02%
==========================================
Files 199 199
Lines 29618 29616 -2
==========================================
+ Hits 22073 22078 +5
+ Misses 7545 7538 -7
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/451?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [libs/promises/api/celix/Promise.h](https://codecov.io/gh/apache/celix/pull/451/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9wcm9taXNlcy9hcGkvY2VsaXgvUHJvbWlzZS5o) | `96.15% <ø> (ø)` | |
| [libs/promises/api/celix/impl/SharedPromiseState.h](https://codecov.io/gh/apache/celix/pull/451/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9wcm9taXNlcy9hcGkvY2VsaXgvaW1wbC9TaGFyZWRQcm9taXNlU3RhdGUuaA==) | `86.91% <100.00%> (+0.42%)` | :arrow_up: |
| [libs/framework/src/framework.c](https://codecov.io/gh/apache/celix/pull/451/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL2ZyYW1ld29yay5j) | `78.71% <0.00%> (+0.06%)` | :arrow_up: |
| [libs/utils/src/hash\_map.c](https://codecov.io/gh/apache/celix/pull/451/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy91dGlscy9zcmMvaGFzaF9tYXAuYw==) | `94.38% <0.00%> (+0.56%)` | :arrow_up: |
| [libs/utils/include/celix/Properties.h](https://codecov.io/gh/apache/celix/pull/451/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy91dGlscy9pbmNsdWRlL2NlbGl4L1Byb3BlcnRpZXMuaA==) | `97.34% <0.00%> (+1.76%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng merged pull request #451: Feature/split celix cxx option
Posted by GitBox <gi...@apache.org>.
PengZheng merged PR #451:
URL: https://github.com/apache/celix/pull/451
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on pull request #451: Feature/split celix cxx option
Posted by GitBox <gi...@apache.org>.
pnoltes commented on PR #451:
URL: https://github.com/apache/celix/pull/451#issuecomment-1336516678
This does not yet solve this issue.
Looking at the logging:
```
2022-12-04T19:21:58.0374265Z 52:
2022-12-04T19:21:58.0374884Z 52: Topic Senders:
2022-12-04T19:21:58.0375433Z 52: |- Topic Sender default/test_invoke
2022-12-04T19:21:58.0375747Z 52: |- serializer type = json
2022-12-04T19:21:58.0376160Z 52: |- protocol type = envelope-v2
2022-12-04T19:21:58.0376521Z 52: |- url = ipc:///tmp/pubsub-test-return (static)
2022-12-04T19:21:58.0376753Z 52:
2022-12-04T19:21:58.0376903Z 52:
2022-12-04T19:21:58.0377061Z 52: Topic Receivers:
2022-12-04T19:21:58.0377244Z 52:
2022-12-04T19:21:58.0377403Z 52:
2022-12-04T19:21:58.0377556Z 52: Topic Senders:
2022-12-04T19:21:58.0377819Z 52: |- Topic Sender default/test_return
2022-12-04T19:21:58.0378106Z 52: |- serializer type = json
2022-12-04T19:21:58.0378376Z 52: |- protocol type = envelope-v2
2022-12-04T19:21:58.0378721Z 52: |- url = ipc:///tmp/pubsub-test-invoke (static)
2022-12-04T19:21:58.0378939Z 52:
2022-12-04T19:21:58.0379079Z 52:
2022-12-04T19:21:58.0379240Z 52: Topic Receivers:
2022-12-04T19:21:58.0379511Z 52: |- Topic Receiver default/test_invoke
2022-12-04T19:21:58.0379777Z 52: |- serializer type = json
2022-12-04T19:21:58.0380055Z 52: |- protocol type = envelope-v2
2022-12-04T19:21:58.0380402Z 52: |- connected url = ipc:///tmp/pubsub-test-return (static)
2022-12-04T19:21:58.0380625Z 52:
2022-12-04T19:21:58.0381002Z 52: [2022-12-04T19:21:58] [ info] [celix_psa_zmq_v2] PSA_ZMQ_TP: Delaying first send for late joiners...
2022-12-04T19:21:58.0381270Z 52:
2022-12-04T19:22:10.0391171Z 52: /home/runner/work/celix/celix/bundles/cxx_remote_services/integration/gtest/src/RemoteServicesIntegrationTestSuite.cc:151: Failure
2022-12-04T19:22:10.0391842Z 52: Value of: promise.isSuccessfullyResolved()
2022-12-04T19:22:10.0392185Z 52: Actual: false
2022-12-04T19:22:10.0392387Z 52: Expected: true
```
I think there is a race condition in the pubsub topology manager / pubsub zmq, because the pubsub overview is missing a TopicReceiver to receive the promise return value.
For now I would like to merge this PR for its changes in CELIX_CXX and coverity issues and create a separate PR to tackle #397
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #451: Feature/split celix cxx option
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #451:
URL: https://github.com/apache/celix/pull/451#discussion_r1040843895
##########
libs/framework/gtest/CMakeLists.txt:
##########
@@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
It seems that a `CELIX_CXX17` testing is missing.
##########
examples/celix-examples/CMakeLists.txt:
##########
@@ -19,7 +19,7 @@ if (COMMAND celix_subproject)
celix_subproject(EXAMPLES "Option to enable building the Examples" ON DEPS SHELL SHELL_TUI LOG_SERVICE)
else ()
set(EXAMPLES true) #celix_subproject is only available in the celix project -> using examples dir in other project is also supported
- option(CELIX_CXX ON)
+ option(CELIX_CXX14 ON)
Review Comment:
missing `CELIX_CXX17`?
##########
conanfile.py:
##########
@@ -160,8 +162,9 @@ def configure(self):
for opt, val in self.options.values.items():
if opt.startswith('build_'):
setattr(self.options, opt, True)
- if not self.options.celix_cxx:
+ if not self.options.celix_cxx14:
Review Comment:
```suggestion
if not self.options.celix_cxx14:
self.options.celix_cxx17 = false
```
##########
conanfile.py:
##########
@@ -160,8 +162,9 @@ def configure(self):
for opt, val in self.options.values.items():
if opt.startswith('build_'):
setattr(self.options, opt, True)
- if not self.options.celix_cxx:
+ if not self.options.celix_cxx14:
self.options.build_cxx_remote_service_admin = False
+ if not self.options.celix_cxx17:
Review Comment:
```suggestion
if not self.options.celix_cxx17:
self.options.build_cxx_remote_service_admin = False
```
According to the corresponsing CMakeLists.txt, cxx_remote_service_admin is C++17.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a diff in pull request #451: Feature/split celix cxx option
Posted by GitBox <gi...@apache.org>.
pnoltes commented on code in PR #451:
URL: https://github.com/apache/celix/pull/451#discussion_r1045277965
##########
examples/celix-examples/CMakeLists.txt:
##########
@@ -19,7 +19,7 @@ if (COMMAND celix_subproject)
celix_subproject(EXAMPLES "Option to enable building the Examples" ON DEPS SHELL SHELL_TUI LOG_SERVICE)
else ()
set(EXAMPLES true) #celix_subproject is only available in the celix project -> using examples dir in other project is also supported
- option(CELIX_CXX ON)
+ option(CELIX_CXX14 ON)
Review Comment:
added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org