-
Notifications
You must be signed in to change notification settings - Fork 89
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/99 update documentation #640
Conversation
Codecov Report
@@ Coverage Diff @@
## master #640 +/- ##
=======================================
Coverage 81.61% 81.61%
=======================================
Files 260 260
Lines 34680 34679 -1
=======================================
Hits 28303 28303
+ Misses 6377 6376 -1
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with some minor remarks attached.
@@ -1299,7 +1299,6 @@ int pubsub_tcpHandler_acceptHandler(pubsub_tcpHandler_t *handle, psa_tcp_connect | |||
#endif | |||
if (rc < 0) { | |||
pubsub_tcpHandler_freeEntry(entry); | |||
free(entry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch.
conan create . --build missing -o build_all=True | ||
#Optionally build with CMake->Ninja, instead of CMake->Make. Note this includes building dependencies with Ninja. | ||
conan create . --build missing -o build_all=True -c tools.cmake.cmaketoolchain:generator=Ninja | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One user requires a minimal build consists of framework
and utils
, see #635
Conan allows users to specify only what they need, and guarantees to provide a minimal build satisfying the requirements. It will be beneficial to mention this.
Create Apache Celix package - and build the dependencies - in the Conan cache: | ||
```bash | ||
cd <celix_source_dir> | ||
conan create . --build missing -o build_all=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying this on a brand-new machine (no jave pre-installed), I encountered the following error:
-- Installing: /home/peng/.conan2/p/b/celixb9d5adbea8041/b/celix/gen/bundles/http_admin/content_install/libhttp_admin.so.0
zip error: Nothing to do! (try: zip -rq /home/peng/.conan2/p/b/celixb9d5adbea8041/b/bundles/http_admin/http_admin/celix_http_admin.zip.install . -i *)
CMake Error at bundles/http_admin/http_admin/cmake_install.cmake:111 (file):
file INSTALL cannot find
"/home/peng/.conan2/p/b/celixb9d5adbea8041/b/bundles/http_admin/http_admin/celix_http_admin.zip.install":
It turns out that bundle packaging using zip (instead of jar) does not work.
It should be fixed by #641
documents/subprojects.md
Outdated
* [Promises library](../libs/promises/README.md) - A C++17 header only adaption and implementation of the OSGi Promise specification. | ||
* [Push Streams Library](../libs/pushstreams/README.md) - A C++17 header adaption and only implementation of the OSGi Push Stream specification. | ||
* [Push Streams Library](../libs/pushstreams/README.md) - A C++17 header adaption and only implementation of the OSGi Push Stream specification. | ||
* [Error Injector Library](../libs/error_injector/README.md) - A C library which can be used to inject errors in a running process, for testing purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have not made it installable yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will removed the mention of Error Injector lib as standalone library
documents/building/README.md
Outdated
The following packages (libraries + headers) should be installed on your system: | ||
|
||
* Development Environment | ||
* build-essentials (gcc/g++ or clang/clang++) | ||
* java or zip (for packaging bundles) | ||
* make (3.14 or higher) | ||
* git | ||
* cmake (3.18 or higher) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no less than 3.19 for Rust support ;p
The following packages (libraries + headers) should be installed on your system: | ||
|
||
* Development Environment | ||
* build-essentials (gcc/g++ or clang/clang++) | ||
* java or zip (for packaging bundles) | ||
* make (3.14 or higher) | ||
* git | ||
* cmake (3.18 or higher) | ||
* Conan (2 or higher) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still support Conan 1 in our CI.
Indeed Conan 2 should be recommended to our users for way better IDE integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I think it is good to promote conan 2.
documents/building/README.md
Outdated
* cmake (3.18 or higher) | ||
* Conan (2 or higher) | ||
|
||
For Ubuntu 20.04, use the following commands: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked linux-build-apt
CI build, which uses Ubuntu 22.04.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct and conan-create.yml also uses 22.04. The linux-build-conan in ubuntu still uses 20.04.
I will update the documentation to 22.04.
|
||
Standalone libraries: | ||
## Standalone Libraries | ||
Apache Celix also provides several standalone libraries which can be used without the framework: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be helpful to mention that building them with Conan involves only turning on one option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add this
This PR corrects and updates the Apache Celix documentation, mainly for subprojects and building with Conan.
Will PR will close #99