-
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/579 automatic dependency deduction #589
Feature/579 automatic dependency deduction #589
Conversation
…ake dependency specification.
Improve dependency specification of RSA.
# Conflicts: # libs/framework/CMakeLists.txt # libs/utils/CMakeLists.txt
Fix indentation.
…celix_subproject.
This reverts commit 9ced95b.
Codecov Report
@@ Coverage Diff @@
## master #589 +/- ##
==========================================
+ Coverage 78.50% 78.54% +0.03%
==========================================
Files 234 234
Lines 35389 35431 +42
==========================================
+ Hits 27782 27829 +47
+ Misses 7607 7602 -5
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
# avoid unknown export "celix" error when building nothing | ||
add_library(celix INTERFACE) | ||
add_library(Celix::celix ALIAS celix) | ||
install(TARGETS celix EXPORT celix DESTINATION ${CMAKE_INSTALL_LIBDIR} COMPONENT 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.
I assume this will generate a imported target?
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.
Yes, this only serves to define the Celix export.
@@ -16,10 +16,11 @@ | |||
# under the License. | |||
|
|||
set(PUSHSTREAMS_DEFAULT_ON ${CELIX_CXX17}) | |||
celix_subproject(PUSHSTREAMS "Option to build the PushStreams library" ${PUSHSTREAMS_DEFAULT_ON} DEPS PROMISES) | |||
celix_subproject(PUSHSTREAMS "Option to build the PushStreams library" ${PUSHSTREAMS_DEFAULT_ON}) |
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.
Does this mean that the celix cmake function celix_subproject
is no longer used to configure dependecies, but this is now done using the Conanfile?
So for pushstream the lines :
if self.options.build_pushstreams:
self.options.build_promises = 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.
Yes. Previously celix_subproject
will only hint our users to enable missing components without automatic configuration.
For a long chain of dependencies, users have to try several times to work out a working configuration for Celix.
Imagine that we have several Conan packages, each of them has intra-package dependencies like Celix, and they also have inter-package dependencies. Then working out a working configuration for a downstream user can be daunting.
With this in place, our users only specify what they need directly, everything else is automatically configured like charm.
# Conflicts: # .github/workflows/ubuntu.yml
This PR fixes #579 by implementing automatic dependency deduction among components within the Celix package.
Now we can specify only what we need, and a minimal Conan package with corresponding feature (and its internal dependencies, but nothing more) will be built for us automatically.
For example, if we only need PushStreams, the following snippet should be enough for any downstream project:
Or we can create a minimal Conan package with only PushStreams and Promises support by the following command line:
conan create . -b missing -o celix:build_pushstreams=True
To this end:
build_
options default to beFalse
. Please note that CMake caches' defaults are not changed.build_
option. This is necessary to fix https://github.com/apache/celix/actions/runs/5560191654/jobs/10156995210CodeCoverage.cmake
has been updated to exclude them from Coverage statistics.Celix::celix
dummy target is introduced. Now all components can be turned off, which will result in empty package. Without this target,install(TARGETS ...)
will report unknown export "celix" error. Note that creating an empty package is the fastest way of publishing a new conan package (without actually building anything). In my day job, I normally publish package this way (CI has already guaranteed that the package can be built correctly).build_
option enabled.conan_test_package
has been updated to discard usage of deprecated headers, and more tests are added.Documentation has not been updated to reflect the above. We may need more usage experience.
@xuzhenbao I noticed if
mdnsresponder/1790.80.10
were used, tests failed. Is that normal?