You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/06/20 13:39:39 UTC

[GitHub] [qpid-dispatch] jiridanek opened a new pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

jiridanek opened a new pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267


   The change would've been nice and easy in CMake 3.12+. Without support for using OBJECT library in `target_link_libraries`, it is necessary to 1) use a STATIC library and then handle the `-Wl,--whole-archive` flag (so that symbols used only from Python are not eliminated by linker) or 2) propagate the dependent libraries through the build some other way. I choose option 2) here.
   
   @astitcher Is this what you had in mind, or are there some improvements that I've missed?


-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#issuecomment-864801630


   MacOS seems to have failed on the empty STATIC library https://travis-ci.com/github/apache/qpid-dispatch/jobs/516966091
   
   ```
   ar: no archive members specified
   usage:  ar -d [-TLsv] archive file ...
   	ar -m [-TLsv] archive file ...
   	ar -m [-abiTLsv] position archive file ...
   	ar -p [-TLsv] archive [file ...]
   	ar -q [-cTLsv] archive file ...
   	ar -r [-cuTLsv] archive file ...
   	ar -r [-abciuTLsv] position archive file ...
   	ar -t [-TLsv] archive [file ...]
   	ar -x [-ouTLsv] archive [file ...]
   make[2]: *** [src/libqpid-dispatch-libraries.a] Error 1
   ```
   
   Other than that, it looks good (so far).
   
   As a quick fix, I can add some dummy symbol into the library which will be then eliminated by linker during linking anyways, so the effect will be the same, and it should work even on macOS.


-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] astitcher commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r655407202



##########
File path: src/server.c
##########
@@ -1738,7 +1738,7 @@ void qd_connector_decref(qd_connector_t* connector)
     }
 }
 
-__attribute__((noinline)) // permit replacement by dummy implementation in unit_tests
+__attribute__((weak)) // permit replacement by dummy implementation in unit_tests

Review comment:
       [Not a comment on this PR really!]
   This whole replacing a function by redefining in the unit tests without an injection API makes me queazy! I'm not entirely sure why - it just seems error-prone.




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r656172968



##########
File path: src/empty.c
##########
@@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+typedef int qd_make_this_translation_unit_not_be_empty;

Review comment:
       This translation unit is deliberately left blank ;P Needed for macOS, where a static library consisting of `/dev/null` source file fails to build.




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r659838422



##########
File path: src/dispatch.c
##########
@@ -122,9 +122,9 @@ STATIC_ASSERT(sizeof(long) >= sizeof(void*), pointer_is_bigger_than_long);
 
 qd_error_t qd_dispatch_load_config(qd_dispatch_t *qd, const char *config_path)
 {
-    qd->dl_handle = dlopen(QPID_DISPATCH_LIB, RTLD_LAZY | RTLD_NOLOAD);
+    qd->dl_handle = dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD);

Review comment:
       I was thinking whether I could move the `dlopen`  to the python side, in `configure_dispatch`, but I was not exactly sure I can make the things work exactly the way they are now.




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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r656172968



##########
File path: src/empty.c
##########
@@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+typedef int qd_make_this_translation_unit_not_be_empty;

Review comment:
       This translation unit is deliberately left blank ;P




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r659761836



##########
File path: src/empty.c
##########
@@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+typedef int qd_make_this_translation_unit_not_be_empty;

Review comment:
       Can you add that comment about macOS support here as well?




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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r659760829



##########
File path: src/dispatch.c
##########
@@ -122,9 +122,9 @@ STATIC_ASSERT(sizeof(long) >= sizeof(void*), pointer_is_bigger_than_long);
 
 qd_error_t qd_dispatch_load_config(qd_dispatch_t *qd, const char *config_path)
 {
-    qd->dl_handle = dlopen(QPID_DISPATCH_LIB, RTLD_LAZY | RTLD_NOLOAD);
+    qd->dl_handle = dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD);

Review comment:
       This dlopen is done for backward compatibility, right?  Can you drop a comment here explaining that?




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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r659760829



##########
File path: src/dispatch.c
##########
@@ -122,9 +122,9 @@ STATIC_ASSERT(sizeof(long) >= sizeof(void*), pointer_is_bigger_than_long);
 
 qd_error_t qd_dispatch_load_config(qd_dispatch_t *qd, const char *config_path)
 {
-    qd->dl_handle = dlopen(QPID_DISPATCH_LIB, RTLD_LAZY | RTLD_NOLOAD);
+    qd->dl_handle = dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD);

Review comment:
       This dlopen is done for backward compatibility, right?  Can you drop a comment here explaining that?

##########
File path: src/empty.c
##########
@@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+typedef int qd_make_this_translation_unit_not_be_empty;

Review comment:
       Can you add that comment about macOS support here as well?




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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-commenter edited a comment on pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#issuecomment-864813959


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?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 [#1267](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d7cf8e) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/3e87d3241243482d45ecc51187b5a9976a7fc471?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e87d32) will **increase** coverage by `0.01%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1267      +/-   ##
   ==========================================
   + Coverage   84.71%   84.72%   +0.01%     
   ==========================================
     Files         113      113              
     Lines       28175    28175              
   ==========================================
   + Hits        23869    23872       +3     
   + Misses       4306     4303       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3NlcnZlci5j) | `86.57% <ø> (ø)` | |
   | [src/timer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3RpbWVyLmM=) | `96.85% <ø> (ø)` | |
   | [src/dispatch.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2Rpc3BhdGNoLmM=) | `83.80% <50.00%> (ø)` | |
   | [...router\_core/modules/edge\_router/link\_route\_proxy.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvbGlua19yb3V0ZV9wcm94eS5j) | `78.69% <0.00%> (-4.15%)` | :arrow_down: |
   | [src/router\_core/modules/edge\_router/edge\_mgmt.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvZWRnZV9tZ210LmM=) | `84.15% <0.00%> (-1.00%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_adaptor.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX2FkYXB0b3IuYw==) | `82.84% <0.00%> (-0.33%)` | :arrow_down: |
   | [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL21lc3NhZ2UuYw==) | `87.33% <0.00%> (-0.15%)` | :arrow_down: |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9ub2RlLmM=) | `93.57% <0.00%> (+0.09%)` | :arrow_up: |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `90.73% <0.00%> (+0.19%)` | :arrow_up: |
   | [src/router\_core/delivery.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2RlbGl2ZXJ5LmM=) | `93.70% <0.00%> (+0.37%)` | :arrow_up: |
   | ... and [2 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3e87d32...2d7cf8e](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r654937669



##########
File path: tests/c_unittests/CMakeLists.txt
##########
@@ -22,8 +22,8 @@ file(GLOB unittest_SOURCES
 )
 
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CXX_STANDARD_FLAGS} ${SANITIZE_FLAGS}")
-add_executable(c_unittests ${unittest_SOURCES})
-target_link_libraries(c_unittests pthread qpid-dispatch)
+add_executable(c_unittests ${unittest_SOURCES}  $<TARGET_OBJECTS:qpid-dispatch>)

Review comment:
       ```suggestion
   add_executable(c_unittests ${unittest_SOURCES} $<TARGET_OBJECTS:qpid-dispatch>)
   ```




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek merged pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek merged pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267


   


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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r655430987



##########
File path: src/server.c
##########
@@ -1738,7 +1738,7 @@ void qd_connector_decref(qd_connector_t* connector)
     }
 }
 
-__attribute__((noinline)) // permit replacement by dummy implementation in unit_tests
+__attribute__((weak)) // permit replacement by dummy implementation in unit_tests

Review comment:
       I'd love to do something about it, but there isn't really an existing universal solution (in the form of a library) that I'd be happy with. And I never spent the time.
   
   Doing linking substitution like this actually is not so bad. It is even recommended (in it's original version, with a shared library) in http://blog.wingman-sw.com/linker-substitution-in-c-limitations-and-workarounds. When switching from shared library to a static one, I could've either kept building the so in addition, just for use in tests (that is a good idea, AFAIK, for the future), or change the tests to use the other technique mentioned in the article (the pointer substitution one). Or use weak symbols, which produces the smallest diff.
   
   I'm actually happy that we can get rid of `noinline`, that was bugging me. Clang by default wants to inline one of the functions on -O2 and we have to prevent that because of our tests. I feel better about having weak symbols around, but that might be just me. Maybe there is some harm in them that I am just not seeing. (Besides the obvious: you might redefine it when you don't mean to.) Noinline is btw necessary with the runtime injection APIs. Or compile for tests extra and turn off optimizations altogether.
   
   Regarding the function pointer substitution technique from the blog. It is the first time I saw it and it is actually a very good solution, also it uses standard C without any platform magic (either linker trickery or ELF file manipulation). It looks really simple to understand, but it's actually quite clever. I wouldn't 've come up with it myself.




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-commenter commented on pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#issuecomment-864813959


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?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 [#1267](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bc958a6) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/3e87d3241243482d45ecc51187b5a9976a7fc471?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e87d32) will **increase** coverage by `0.06%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1267      +/-   ##
   ==========================================
   + Coverage   84.71%   84.77%   +0.06%     
   ==========================================
     Files         113      113              
     Lines       28175    28175              
   ==========================================
   + Hits        23869    23886      +17     
   + Misses       4306     4289      -17     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3NlcnZlci5j) | `86.57% <ø> (ø)` | |
   | [src/timer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3RpbWVyLmM=) | `96.85% <ø> (ø)` | |
   | [src/dispatch.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2Rpc3BhdGNoLmM=) | `83.80% <50.00%> (ø)` | |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `89.45% <0.00%> (-1.09%)` | :arrow_down: |
   | [src/router\_core/delivery.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2RlbGl2ZXJ5LmM=) | `93.14% <0.00%> (-0.19%)` | :arrow_down: |
   | [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL21lc3NhZ2UuYw==) | `87.33% <0.00%> (-0.15%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX3NlcnZlci5j) | `85.85% <0.00%> (+0.28%)` | :arrow_up: |
   | [src/router\_core/forwarder.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2ZvcndhcmRlci5j) | `93.83% <0.00%> (+0.79%)` | :arrow_up: |
   | [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `94.61% <0.00%> (+0.86%)` | :arrow_up: |
   | [src/iterator.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2l0ZXJhdG9yLmM=) | `93.17% <0.00%> (+3.87%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3e87d32...bc958a6](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#issuecomment-864801630


   MacOS seems to have failed on the empty STATIC library https://travis-ci.com/github/apache/qpid-dispatch/jobs/516966091
   
   ```
   ar: no archive members specified
   usage:  ar -d [-TLsv] archive file ...
   	ar -m [-TLsv] archive file ...
   	ar -m [-abiTLsv] position archive file ...
   	ar -p [-TLsv] archive [file ...]
   	ar -q [-cTLsv] archive file ...
   	ar -r [-cuTLsv] archive file ...
   	ar -r [-abciuTLsv] position archive file ...
   	ar -t [-TLsv] archive [file ...]
   	ar -x [-ouTLsv] archive [file ...]
   make[2]: *** [src/libqpid-dispatch-libraries.a] Error 1
   ```
   
   Other than that, it looks good (so far).
   
   As a quick fix, I can add some dummy symbol into the library which will be then eliminated by linker during linking anyways, so the effect will be the same, and it should work even on macOS.


-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r655416842



##########
File path: tests/CMakeLists.txt
##########
@@ -40,8 +40,8 @@ set(unit_test_SOURCES
     thread_test.c
     )
 
-add_executable(unit_tests ${unit_test_SOURCES})
-target_link_libraries(unit_tests  qpid-dispatch)
+add_executable(unit_tests ${unit_test_SOURCES} $<TARGET_OBJECTS:qpid-dispatch>)
+target_link_libraries(unit_tests qpid-dispatch-libraries)

Review comment:
       The static library is empty, it is only CMake target that carries dependencies of qpid-dispatch. Like INTERFACE library, but that is not supported in CMake 3.8.12.2. I have to propagate the PUBLIC/INTERFACE dependencies of the object files in `$<TARGET_OBJECTS:qpid-dispatch>` somehow.
   
   If I had CMake 3.12, I could do `target_link_libraries(unit_tests qpid-dispatch)` and be done with it. For older CMake, I have to add the object files into `add_executable`, but that does nothing about link dependencies. So I'd have to have e.g. a variable.... I thought that a `-libraries` static library containing no symbols of its own is cleaner. (The library will actually have to contain something due to the macOS error I mentioned above.)

##########
File path: src/server.c
##########
@@ -1738,7 +1738,7 @@ void qd_connector_decref(qd_connector_t* connector)
     }
 }
 
-__attribute__((noinline)) // permit replacement by dummy implementation in unit_tests
+__attribute__((weak)) // permit replacement by dummy implementation in unit_tests

Review comment:
       I'd love to do something about it, but there isn't really an existing universal solution (in the form of a library) that I'd be happy with. And I never spent the time.
   
   Doing linking substitution like this actually is not so bad. It is even recommended (in it's original version, with a shared library) in http://blog.wingman-sw.com/linker-substitution-in-c-limitations-and-workarounds. When switching from shared library to a static one, I could've either kept building the so in addition, just for use in tests (that is a good idea, AFAIK, for the future), or change the tests to use the other technique mentioned in the article (the pointer substitution one). Or use weak symbols, which produces the smallest diff.
   
   I'm actually happy that we can get rid of `noinline`, that was bugging me. Clang by default wants to inline one of the functions on -O2 and we have to prevent that because of our tests. I feel better about having weak symbols around, but that might be just me. Maybe there is some harm in them that I am just not seeing. (Besides the obvious: you might redefine it when you don't mean to.) Noinline is btw necessary with the runtime injection APIs. Or compile for tests extra and turn off optimizations altogether.
   
   Regarding the function pointer substitution technique from the blog. It is the first time I saw it and it is actually a very good solution, also it uses standard C without any platform magic (either linker trickery or ELF file manipulation). It looks really simple to understand, but it's actually quite clever. I wouldn't 've come up with it myself.




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r659838422



##########
File path: src/dispatch.c
##########
@@ -122,9 +122,9 @@ STATIC_ASSERT(sizeof(long) >= sizeof(void*), pointer_is_bigger_than_long);
 
 qd_error_t qd_dispatch_load_config(qd_dispatch_t *qd, const char *config_path)
 {
-    qd->dl_handle = dlopen(QPID_DISPATCH_LIB, RTLD_LAZY | RTLD_NOLOAD);
+    qd->dl_handle = dlopen(NULL, RTLD_LAZY | RTLD_NOLOAD);

Review comment:
       I was thinking whether I could move the `dlopen`  to the python side, in `configure_dispatch`, but I was not exactly sure I can make the things work exactly the way they are now.




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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r655416842



##########
File path: tests/CMakeLists.txt
##########
@@ -40,8 +40,8 @@ set(unit_test_SOURCES
     thread_test.c
     )
 
-add_executable(unit_tests ${unit_test_SOURCES})
-target_link_libraries(unit_tests  qpid-dispatch)
+add_executable(unit_tests ${unit_test_SOURCES} $<TARGET_OBJECTS:qpid-dispatch>)
+target_link_libraries(unit_tests qpid-dispatch-libraries)

Review comment:
       The static library is empty, it is only CMake target that carries dependencies of qpid-dispatch. Like INTERFACE library, but that is not supported in CMake 3.8.12.2. I have to propagate the PUBLIC/INTERFACE dependencies of the object files in `$<TARGET_OBJECTS:qpid-dispatch>` somehow.
   
   If I had CMake 3.12, I could do `target_link_libraries(unit_tests qpid-dispatch)` and be done with it. For older CMake, I have to add the object files into `add_executable`, but that does nothing about link dependencies. So I'd have to have e.g. a variable.... I thought that a `-libraries` static library containing no symbols of its own is cleaner. (The library will actually have to contain something due to the macOS error I mentioned above.)




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] astitcher commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r655407202



##########
File path: src/server.c
##########
@@ -1738,7 +1738,7 @@ void qd_connector_decref(qd_connector_t* connector)
     }
 }
 
-__attribute__((noinline)) // permit replacement by dummy implementation in unit_tests
+__attribute__((weak)) // permit replacement by dummy implementation in unit_tests

Review comment:
       [Not a comment on this PR really!]
   This whole replacing a function by redefining in the unit tests without an injection API makes me queazy! I'm not entirely sure why - it just seems error-prone.

##########
File path: tests/CMakeLists.txt
##########
@@ -40,8 +40,8 @@ set(unit_test_SOURCES
     thread_test.c
     )
 
-add_executable(unit_tests ${unit_test_SOURCES})
-target_link_libraries(unit_tests  qpid-dispatch)
+add_executable(unit_tests ${unit_test_SOURCES} $<TARGET_OBJECTS:qpid-dispatch>)
+target_link_libraries(unit_tests qpid-dispatch-libraries)

Review comment:
       In this and the subsequent similar changes I don't understand why you need both to add the object files and to add a static library. Surely you are just effectively adding the same object twice like this? I think you only need to do the add_executable() change and remove the target_link_libraries() unless there is another library to add.




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-commenter commented on pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#issuecomment-864813959


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?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 [#1267](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bc958a6) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/3e87d3241243482d45ecc51187b5a9976a7fc471?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e87d32) will **increase** coverage by `0.06%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1267      +/-   ##
   ==========================================
   + Coverage   84.71%   84.77%   +0.06%     
   ==========================================
     Files         113      113              
     Lines       28175    28175              
   ==========================================
   + Hits        23869    23886      +17     
   + Misses       4306     4289      -17     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3NlcnZlci5j) | `86.57% <ø> (ø)` | |
   | [src/timer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3RpbWVyLmM=) | `96.85% <ø> (ø)` | |
   | [src/dispatch.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2Rpc3BhdGNoLmM=) | `83.80% <50.00%> (ø)` | |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `89.45% <0.00%> (-1.09%)` | :arrow_down: |
   | [src/router\_core/delivery.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2RlbGl2ZXJ5LmM=) | `93.14% <0.00%> (-0.19%)` | :arrow_down: |
   | [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL21lc3NhZ2UuYw==) | `87.33% <0.00%> (-0.15%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX3NlcnZlci5j) | `85.85% <0.00%> (+0.28%)` | :arrow_up: |
   | [src/router\_core/forwarder.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2ZvcndhcmRlci5j) | `93.83% <0.00%> (+0.79%)` | :arrow_up: |
   | [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `94.61% <0.00%> (+0.86%)` | :arrow_up: |
   | [src/iterator.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2l0ZXJhdG9yLmM=) | `93.17% <0.00%> (+3.87%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3e87d32...bc958a6](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-commenter edited a comment on pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#issuecomment-864813959


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?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 [#1267](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d7cf8e) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/3e87d3241243482d45ecc51187b5a9976a7fc471?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e87d32) will **increase** coverage by `0.01%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1267      +/-   ##
   ==========================================
   + Coverage   84.71%   84.72%   +0.01%     
   ==========================================
     Files         113      113              
     Lines       28175    28175              
   ==========================================
   + Hits        23869    23872       +3     
   + Misses       4306     4303       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3NlcnZlci5j) | `86.57% <ø> (ø)` | |
   | [src/timer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3RpbWVyLmM=) | `96.85% <ø> (ø)` | |
   | [src/dispatch.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2Rpc3BhdGNoLmM=) | `83.80% <50.00%> (ø)` | |
   | [...router\_core/modules/edge\_router/link\_route\_proxy.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvbGlua19yb3V0ZV9wcm94eS5j) | `78.69% <0.00%> (-4.15%)` | :arrow_down: |
   | [src/router\_core/modules/edge\_router/edge\_mgmt.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvZWRnZV9tZ210LmM=) | `84.15% <0.00%> (-1.00%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_adaptor.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX2FkYXB0b3IuYw==) | `82.84% <0.00%> (-0.33%)` | :arrow_down: |
   | [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL21lc3NhZ2UuYw==) | `87.33% <0.00%> (-0.15%)` | :arrow_down: |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9ub2RlLmM=) | `93.57% <0.00%> (+0.09%)` | :arrow_up: |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `90.73% <0.00%> (+0.19%)` | :arrow_up: |
   | [src/router\_core/delivery.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/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-c3JjL3JvdXRlcl9jb3JlL2RlbGl2ZXJ5LmM=) | `93.70% <0.00%> (+0.37%)` | :arrow_up: |
   | ... and [2 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1267/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3e87d32...2d7cf8e](https://codecov.io/gh/apache/qpid-dispatch/pull/1267?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267#discussion_r655416842



##########
File path: tests/CMakeLists.txt
##########
@@ -40,8 +40,8 @@ set(unit_test_SOURCES
     thread_test.c
     )
 
-add_executable(unit_tests ${unit_test_SOURCES})
-target_link_libraries(unit_tests  qpid-dispatch)
+add_executable(unit_tests ${unit_test_SOURCES} $<TARGET_OBJECTS:qpid-dispatch>)
+target_link_libraries(unit_tests qpid-dispatch-libraries)

Review comment:
       The static library is empty, it is only a CMake target that carries dependencies of qpid-dispatch. Like INTERFACE library, but that is not supported in CMake 3.8.12.2. I have to propagate the PUBLIC/INTERFACE dependencies of the object files in `$<TARGET_OBJECTS:qpid-dispatch>` somehow.
   
   If I had CMake 3.12, I could do `target_link_libraries(unit_tests qpid-dispatch)` and be done with it. For older CMake, I have to add the object files into `add_executable`, but that does nothing about link dependencies. So I'd have to have e.g. a variable.... I thought that a `-libraries` static library containing no symbols of its own is cleaner. (The library will actually have to contain something due to the macOS error I mentioned above.)

##########
File path: tests/CMakeLists.txt
##########
@@ -40,8 +40,8 @@ set(unit_test_SOURCES
     thread_test.c
     )
 
-add_executable(unit_tests ${unit_test_SOURCES})
-target_link_libraries(unit_tests  qpid-dispatch)
+add_executable(unit_tests ${unit_test_SOURCES} $<TARGET_OBJECTS:qpid-dispatch>)
+target_link_libraries(unit_tests qpid-dispatch-libraries)

Review comment:
       The static library is empty, it is only a CMake target that carries dependencies of qpid-dispatch. Like INTERFACE library, but that is not supported in CMake 2.8.12.2. I have to propagate the PUBLIC/INTERFACE dependencies of the object files in `$<TARGET_OBJECTS:qpid-dispatch>` somehow.
   
   If I had CMake 3.12, I could do `target_link_libraries(unit_tests qpid-dispatch)` and be done with it. For older CMake, I have to add the object files into `add_executable`, but that does nothing about link dependencies. So I'd have to have e.g. a variable.... I thought that a `-libraries` static library containing no symbols of its own is cleaner. (The library will actually have to contain something due to the macOS error I mentioned above.)




-- 
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek merged pull request #1267: DISPATCH-2178: Do not build separate libqpid-dispatch.so

Posted by GitBox <gi...@apache.org>.
jiridanek merged pull request #1267:
URL: https://github.com/apache/qpid-dispatch/pull/1267


   


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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org