pnoltes commented on a change in pull request #353:
URL: https://github.com/apache/celix/pull/353#discussion_r681900979



##########
File path: bundles/pubsub/examples/CMakeLists.txt
##########
@@ -221,10 +221,10 @@ if (BUILD_PUBSUB_PSA_ZMQ)
         BUNDLES
             Celix::shell
             Celix::shell_tui
-            Celix::pubsub_serializer_json
-            Celix::pubsub_discovery_etcd
-            Celix::pubsub_topology_manager
-            Celix::pubsub_admin_zmq
+            Celix::celix_pubsub_serializer_json
+            Celix::celix_pubsub_discovery_etcd
+            Celix::celix_pubsub_topology_manager
+            Celix::celix_pubsub_admin_zmq

Review comment:
       Because this reflects how the bundles are actually named when using 
Celix (e.g. the imported cmake target names).
   Ideally the celix_ should not be part of this, but if we change this now 
this will be a backwards incompatible update. 
   
   For example:
   ``` 
   add_library(foo SHARED ...)
   install(TARGETS foo EXPORT ProjectTargets)
   install(EXPORT ProjectTargets NAMESPACE MyProject ...)
   ```
   will lead to a library file `libfoo.so` and a imported target with the name 
`MyProject::foo` 
   
   And 
   ``` 
   add_library(myproject_foo SHARED ...)
   install(TARGETS myproject_foo EXPORT ProjectTargets)
   install(EXPORT ProjectTargets NAMESPACE MyProject ...)
   ```
   will lead to a library file `libmyproject_foo.so` and a imported target with 
the name `MyProject::myproject_foo` 
   
   To really get what you want you need:
   ``` 
   add_library(foo SHARED ...)
   set_target_properties(foo PROPERTIES OUTPUT_NAME "myproject_foo")
   install(TARGETS foo EXPORT ProjectTargets)
   install(EXPORT ProjectTargets NAMESPACE MyProject ...)
   ```
   which will lead to a library file `libmyproject_foo.so` and a imported 
target with the name `MyProject::foo` 
   
   What I prefer to do for the pubsub bundles was to rename the bundle target 
and a FILENAME attribute to the `add_bundle` so that the bundle filename 
contains a celix_ prefix to ensure uniqueness, but the imported target did not 
have a  Celix::celix_  usage. But again, this would be a backwards incompatible 
update. 
   
   e.g.
   ```
   add_celix_bundle(pubsub_admin_tcp_v2
      ...
     FILENAME celix_pubsub_admin_tcp_v2
   )
   ...
   ```




-- 
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-unsubscr...@celix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to