Repository navigation
configure: check if "-lrt" is required for using the Dasynq timers - #588
Conversation
Specify the linker flags at the end of the compiler driver invocation in try_link_object(), like how Makefiles do. The ordering of compiler driver flags is important, for example if the linker is invoked with '--as-needed' flag, it would discard any library that doesn't satisfiy a non-weak symbol at the time of being loaded and because 'configure' specified linker flags (and potentially libraries) before the object, it would make the linker discard the specified library, leading to false "undefined symbols" when the linker tries to link the object. See ld(1) for more information.
davmac314
left a comment
davmac314
left a comment
There was a problem hiding this comment.
Thanks, this is pretty good. I do have some minor comments.
| t1.add_timer(l1); | ||
| l1.run(); |
There was a problem hiding this comment.
For completeness this really should arm the timer also (arm_timer_rel(l1, {1,0}); or similar).
| if [ "$AUTO_LDFLAGS_BASE" = true ] && [ "$PLATFORM" = FreeBSD ]; then | ||
| check_link_argument LDFLAGS_BASE -lrt | ||
| if [ "$AUTO_LDFLAGS_BASE" = true ]; then | ||
| info "Checking whether using timers require linking with librt to work..." |
There was a problem hiding this comment.
"to work" is redundant, and "require" should be "requires".
I actually think:
"Checking whether linking with librt is required"
... would be better. No real need to mention timers here; that's really just an implementation detail, and explicitly mentioning timers sort of implies that maybe it would be possible to build without support for timers.
There was a problem hiding this comment.
I prefer "Checking whether linking with librt is required".
| # Account for the CPPFLAGS in this check. Unlike the other checks, this one uses the Dasynq | ||
| # library which can be configured with a number of macro definitions. For example, an user | ||
| # under Linux may specify "-DDASYNQ_HAVE_EPOLL=0" to disable usage of epoll system calls which | ||
| # also makes Dasynq use POSIX timers instead of timerfd, potentially changing the test result. |
There was a problem hiding this comment.
Nit: good comment in that it is explanatory and useful, but it is quite verbose. You could probably trim this down to the essential details, i.e. Dasynq can be configured by compile-time macros, which can influence the result of this test.
You can leave mostly as-is if you prefer, but in that case it's "a user" not "an user" and "disable usage of the epoll system call" not "disable usage of epoll system calls".
There was a problem hiding this comment.
Nit: good comment in that it is explanatory and useful, but it is quite verbose. You could probably trim this down to the essential details, i.e. Dasynq can be configured by compile-time macros, which can influence the result of this test.
This essential part of this is the Account for the CPPFLAGS in this check. Unlike the other checks, this one uses the Dasynq library which can be configured with a number of macro definitions. and the rest is just an example of how this affects this test. I'm going to reorder it a little bit to make the essential part complete without reading the example (i.e. moving potentially changing the test result. to the essential part.)
You can leave mostly as-is if you prefer, but in that case it's "a user" not "an user" and "disable usage of the epoll system call" not "disable usage of epoll system calls".
Fixed.
There was a problem hiding this comment.
This essential part of this is the Account for the CPPFLAGS in this check. Unlike the other checks,
The details I pointed out in my comment are what I believe to to be the essential part.
"Account for the CPPFLAGS in this check" is just something that the line below is doing. It should be something more like "CPPFLAGS is included in the following check, because ..." if this is important.
"Unlike the other checks" is just extraneous - you don't need to say it.
There was a problem hiding this comment.
I've trimmed it down a little bit.
4a8d76f to
33f13a8
Compare
| # a user under Linux may specify "-DDASYNQ_HAVE_EPOLL=0" to disable usage of the epoll system | ||
| # calls and make Dasynq use POSIX timers instead of timerfd. |
There was a problem hiding this comment.
disable usage of the epoll system calls
As I said in the previous review, it's "disable usage of the epoll system call". It's an interface, singular - call, not calls.
This would be better, IMO:
"... to disable the epoll backend, which currently also causes Dasynq to use POSIX timers."
But at least fix "calls" to "call" as I already pointed that out, please. The rest looks ok.
There was a problem hiding this comment.
Sorry. I used the string in the Dasynq config.h (If the epoll family of system calls are available:) which says family of and I only saw the the in your requested change.
Anyway, changed to your suggestion.
There was a problem hiding this comment.
I used the string in the Dasynq config.h
You're right actually that it is a set of calls, that slipped my mind. Still, keeping "family of" would have been more clear. (I think what it is now is better though).
Check if using a Dasynq timer in a test program requires linking with librt, then supply "-lrt" in the LDFLGAS_BASE of the build if it does. Perviously 'configure' only added the "-lrt" flag for the FreeBSD, which was known for requiring it.
33f13a8 to
8062e5b
Compare
2 participants
Addresses #551 and also includes a fix for linker flags ordering issue that was found when testing it with Ubuntu 21.04.
Tested on an Ubuntu 21.04 VM with
CPPFLAGS="-DDASYNQ_HAVE_EPOLL=0" ./configure(does require linking with librt) and my Artix (does not require linking with librt).