Mostly Autoconf cleanups #240

wferi · 2016-12-07T14:33:07Z

A couple of small things I noticed during packaging the 1.0.1 release. They don't make a big difference for a small project like libqb, but I decided to offer them for the sake of consistency and future-proofing.
Especially for pthreads, where I switched to the standard macro from the Autoconf Archive.
I couldn't fully test this on systems where gethostbyname or the socket functions aren't in libc, so some corrections might be needed if you support them for real.
Please consider incorporating these patches.

jnpkrn · 2016-12-07T19:42:14Z

Minor nitpicks:

could you please stick with common commit message prefixes
and style that has been exercised lately
(build: [configure:], Fix typos:)?
it's entirely fine to merge the single-change spelling fixes
the first commit's message contains typo on its own (defualt)

jnpkrn · 2016-12-07T19:52:55Z

configure.ac

@@ -466,10 +466,8 @@ AC_ARG_ENABLE([coverage],
 AC_ARG_ENABLE([slow-tests],
  [AS_HELP_STRING([--enable-slow-tests],[build and run slow tests])])

-if test x"$with_check" = xyes; then
 AC_ARG_ENABLE([syslog-tests],
  [AS_HELP_STRING([--enable-syslog-tests],[build and run syslog tests])])


Can you advise as to whether this conditional is sensible, please?
My original intention was to lay an egg as carefully as possible because
libtool warns that foo_LDFLAGS = -module is not portable.

Any idea of having this portability tested in some minimalistic way
and turning "syslog-tests" on automatically if positive?

Or is it more a spurious warning of libtool?

I can't see any such warning. Where and how do you get it?

Hm, cannot reproduce that easily, it might have been on FreeBSD 10.2 VM.

Reproduced on Linux box, but had to go back in history:

$ make distclean $ git co -b syslog-test 642f74d $ ./autogen.sh && ./configure --enable-syslog-tests && make check ... *** Warning: Linking the executable log.test against the loadable module *** _syslog_override.so is not portable! ...

Hard to say what resolved this.

ebcff54 did. Looks like portability is a concern for installed artifacts only. According to SO and the included link, only Mac OS X is affected.

Yep, just have discovered that commit too; nice to have this positive
side effect :-)

In the new light, I would suggest dropping --enable-syslog-tests
and enabling it unconditionally when the test suite is enabled as a whole.
I intend to do it subsequently on top of your PR, but feel free to drop
it right now without attempting to rectify anything related needlessly.

The [ default="no" ] branches were sense- and (mostly) harmless.

So let's use the more friendly syntax.

This reduces overlinking of qb-blackbox. Being a seldom used executable, the gains are mostly theoretical, but at least this silences warnings from some QA tools.

wferi · 2016-12-08T13:21:50Z

I implemented your nits and reordered that patches. The last two (GitHub doesn't show them in ancestry order) may not be necessary if you intend to drop the --enable-syslog-tests option altogether. I can take them back if you prefer.

jnpkrn · 2016-12-08T20:02:30Z

@wferi please have a look at #241 if it works for you (dropped the
last two commits, the rest is reused untouched).

wferi · 2016-12-08T21:30:17Z

#241 works for me perfectly, thanks.
Closing this.

jnpkrn · 2016-12-08T21:58:25Z

On 08/12/16 13:30 -0800, wferi wrote: #241 works for me perfectly, thanks. Closing this.

I thank you for the exemplary cleanups, good work.

…

-- Jan (Poki)

jnpkrn · 2016-12-09T17:13:43Z

configure.ac

+AX_SAVE_FLAGS
+AC_SEARCH_LIBS([dlopen],[dl],,[AC_MSG_ERROR([cannot find dlopen() function])])
+AC_SUBST([dlopen_LIBS],[$LIBS])
+AX_RESTORE_FLAGS


@wferi, discussed with @chrissie-c, she raised concerns about "keeping large(ish) chunks of other people's code in there just for simple things, it's just more stuff to keep up-to-date" and I must admit there's some grain of truth in it as all we really need here is just to backup+restore LIBS, nothing more, nothing less. Would you mind reopening this PR (if possible, I am not sure) and reflecting this comment, please? Indeed, I can do it as another follow up, but better to keep history clean.

Right there you don't even need to save LIBS, because it's empty. It's a little more interesting around the pthread function tests, where we also manipulate CFLAGS (which might be set by the user). Sure, all uses of AX_SAVE/RESTORE_FLAGS could easily be replaced by custom code. I decided to use them in this pull request, though, mostly for demo.
The main point is that they save you from thinking globally when doing local changes: you gain modularity and composability. You are free to add more involved checks (possibly requiring other flag changes) inside the save/restore pair or reorder them at will. They also reduce the room for error (I might be biased, but a couple of weeks ago I found the insidious CFLAGS="-I/usr/include/security $CLFAGS" and CPPFLAGS="$ac_save_CFLAGS" in the same configure.ac — after far too much head scratching).
Concerning the other point, all three macro files are straight from the Autoconf Archive. They seldom need updates, and when they do, it's a simple replace. Another option is requiring the archive to be installed for building libqb from Git.

jnpkrn · 2016-12-12T09:52:13Z

On 09/12/16 13:12 -0800, wferi wrote: wferi commented on this pull request. > @@ -108,7 +108,10 @@ dnl it will always be "none needed", but it is not true dnl when linking libraries. Looks like a bug. AC_SEARCH_LIBS([pthread_create], [pthread]) AC_SEARCH_LIBS([mq_open], [rt]) -AC_SEARCH_LIBS([dlopen], [dl]) +AX_SAVE_FLAGS +AC_SEARCH_LIBS([dlopen],[dl],,[AC_MSG_ERROR([cannot find dlopen() function])]) +AC_SUBST([dlopen_LIBS],[$LIBS]) +AX_RESTORE_FLAGS Right there you don't even need to save `LIBS`, because it's empty. It's a little more interesting around the pthread function tests, where we also manipulate `CFLAGS` (which might be set by the user).

Ah, next time, I will not forward anything semi-unconsciously (for some reason, I went by just the easy cases).

Sure, all uses of `AX_SAVE/RESTORE_FLAGS` could easily be replaced by custom code. I decided to use them in this pull request, though, mostly for demo. The main point is that they save you from thinking globally when doing local changes: you gain modularity and composability. You are free to add more involved checks (possibly requiring other flag changes) inside the save/restore pair or reorder them at will. They also reduce the room for error (I might be biased, but a couple of weeks ago I found the insidious `CFLAGS="-I/usr/include/security $CLFAGS"` and `CPPFLAGS="$ac_save_CFLAGS"` in the same `configure.ac` — after far too much head scratching).

Yes, the ability to offload some tasks to well-tested, self-contained solution, is appealing. No doubts about that.

Concerning the other point, all three macro files are straight from the Autoconf Archive. They seldom need updates, and when they do, it's a simple replace. Another option is requiring the archive to be installed for building libqb from Git.

That would just complicate consumption, in-tree is OK I believe. I guess I let Chrissie speak for herself. Btw. if there's anything to be done about the commits, I would have a tiny wish (feel free to ignore if the PR would eventually be accepted as is): "configure: restrict pthreads to where it's actually needed" commit could mention that mq_open is no longer relevant beyond commit 70a9623. Thanks for bearing with me (us).

…

-- Jan (Poki)

mq_open() is no longer relevant beyond 70a9623 (Remove message queues).

wferi · 2016-12-12T10:55:12Z

Jan Pokorný <[email protected]> writes:

"configure: restrict pthreads to where it's actually needed" commit could mention that mq_open is no longer relevant beyond commit 70a9623.

True; I added this note and dropped the last two commits reorganizing --enable-syslog-tests, since you'll remove that anyway.

Thanks for bearing with me (us).

Come on, I actually count on you being picky and careful. I've only got a very narrow view of the project and zero experience with maintaining it. -- Regards, Feri

wferi · 2016-12-12T11:14:43Z

... and the Travis checks failed. On code which is exactly the same as 2908242 in #241. Is this failure genuine? Or maybe the ipc.test has some race? The HPPA buildd failed differently. (Hurd and kFreeBSD also fail some tests.)

jnpkrn · 2016-12-12T11:28:41Z

On 12/12/16 03:14 -0800, wferi wrote: ... and the Travis checks failed. On code which is exactly the same as 2908242 in #241. Is this failure genuine? Or maybe the `ipc.test` has some race?

Yes, I suspect something like this: #234

The HPPA buildd failed [differently](https://buildd.debian.org/status/fetch.php?pkg=libqb&arch=hppa&ver=1.0.1-1&stamp=1481130367). (Hurd and kFreeBSD also fail some tests.)

check_ipc.c:1007:E:ipc_stress_connections_shm:test_ipc_stress_connections_shm:0: (after this point) Test timeout expired ^ just adjusting the timeout for that test might help here. Anyway, updated #241 and it passed: https://travis-ci.org/ClusterLabs/libqb/builds/183218749

…

-- Jan (Poki)

chrissie-c · 2016-12-12T11:36:19Z

The timeout for the stress test has to be HUGE on some platforms. I think it's set to an hour or so even from *BSD

PR #240 + addendum (drop syslog-tests opt-in switch)

jnpkrn · 2016-12-16T14:59:06Z

It now causes a following problem with EL6:
https://copr-be.cloud.fedoraproject.org/results/%40ClusterLabs/devel/epel-6-x86_64/00490031-libqb/build.log.gz

autoreconf: running: /usr/bin/autoconf
configure:19163: error: possibly undefined macro: AS_VAR_COPY
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
autoreconf: /usr/bin/autoconf failed with exit status: 1

Note that I already encountered something similar with pacemaker,
where solution was to use AS_VAR_GET instead:

ClusterLabs/pacemaker@36f7196#diff-a5522b90382504dc530177f63ced5209R46

@wferi, if you are OK with such a change, I'll go ahead along with
resetting serial to minimal value (currently unsure if 0 or 1).

wferi · 2016-12-18T10:01:00Z

@jnpkrn, what do you think about introducing a "legacy" M4 file like for example https://github.com/varnishcache/varnish-cache/blob/master/varnish-legacy.m4? Definitions of missing macros could be put there as needed in the future. We could start with something like

# For compatibility with autoconf < 2.63b
m4_ifndef([AS_VAR_COPY],
  [m4_define([AS_VAR_COPY],
     [AS_LITERAL_IF([$1[]$2], [$1=$$2], [eval $1=\$$2])])])

which is basically a conditionalized copy of the AS_VAR_COPY definition in /usr/share/autoconf/m4sugar/m4sh.m4.

jnpkrn · 2016-12-21T08:14:24Z

On 18/12/16 02:01 -0800, wferi wrote: @jnpkrn, what do you think about introducing a "legacy" M4 file like

Sounds reasonable, though it needs to be tested if it would be indeed the "fixed point" for ~el6 already or more and more macros would have to be introduced like this, which is something we would rather avoid. Will have a look early in 2017, holiday season for me till then (in case you have it similar, enjoy).

…

-- Jan (Poki)

wferi · 2016-12-21T10:19:41Z

Sure we can return to this next year. I'm already on holiday. Merry Christmas! -- Feri

wferi · 2017-05-31T14:53:19Z

Thanks for the merge, @jnpkrn.
How did you solve the problem of AS_VAR_COPY being undefined under EL6 in the end?

jnpkrn · 2017-05-31T15:52:02Z

On 31/05/17 07:53 -0700, wferi wrote: How did you solve the problem of `AS_VAR_COPY` being undefined under EL6 in the end?

It rests on the back burner so far, will hopefully get to is soon.

…

-- Jan (Poki)

inouekazu · 2017-07-31T10:09:55Z

wferi commented on 31 May
How did you solve the problem of AS_VAR_COPY being undefined under EL6 in the end?

I made a patch inouekazu@4b3c4c7. how is it?

I pulled request it #267.

wferi · 2017-07-31T18:16:00Z

I made a patch inouekazu/libqb@4b3c4c7. how is it?

Looks fine, but is it enough to fix the EL6 build, as @jnpkrn queried?

inouekazu · 2017-08-08T09:56:30Z

Looks fine, but is it enough to fix the EL6 build, as @jnpkrn queried?

Yes. I checked with RHEL 6.9.

[root@rhel69-1 ~]# cat /etc/redhat-release
Red Hat Enterprise Linux Server release 6.9 (Santiago)

[root@rhel69-1 ~]# git clone https://github.com/ClusterLabs/libqb
[root@rhel69-1 ~]# cd libqb

[root@rhel69-1 libqb]# vi m4/ax_compat.m4
# For compatibility with autoconf < 2.63b
m4_ifndef([AS_VAR_COPY],
  [AC_DEFUN([AS_VAR_COPY],
     [AS_LITERAL_IF([$1[]$2], [$1=$$2], [eval $1=\$$2])])])

[root@rhel69-1 libqb]# ./autogen.sh
(snip)
examples/Makefile.am: installing `build-aux/depcomp'
autoreconf: Leaving directory `.'
Now run ./configure and make

[root@rhel69-1 libqb]# ./configure
(snip)

libqb configuration:
  Version                  = 1.0.2.18-333f
  Prefix                   = /usr
  Executables              = ${exec_prefix}/sbin
  Man pages                = ${datarootdir}/man
  Doc dir                  = ${datarootdir}/doc/${PACKAGE_TARNAME}
  Libraries                = /usr/lib64
  Header files             = ${prefix}/include
  Arch-independent files   = ${datarootdir}
  State information        = /var
  System configuration     = /etc
  SOCKETDIR                = /var/run
  Features                 =  epoll gcc__sync attribute-section

libqb build info:
  Optimization             =
  Debug options            =
  Extra compiler warnings  =
  Env. defined CFLAG       = -g -O2
  Env. defined CPPFLAGS    =
  Env. defined LDFLAGS     =
  ANSI defined CPPFLAGS    =
  Coverage     CFLAGS      =
  Coverage     LDFLAGS     =
  Fatal War.   CFLAGS      =
  Final        CFLAGS      = -g -O2       -Wall -Wextra -Wunused -Wshadow -Wmissing-prototypes -Wmissing-declarations -Wstrict-prototypes -Wpointer-arith -Wwrite-strings -Wcast-align -Wbad-function-cast -Wmissing-format-attribute -Wfloat-equal -Wformat=2 -Woverlength-strings -Winit-self -Wuninitialized -Wunknown-pragmas -Wno-unused-parameter -Wno-format-nonliteral
  Final        CPPFLAGS    =
  Final        LDFLAGS     =
[root@rhel69-1 libqb]#

jnpkrn reviewed Dec 7, 2016

View reviewed changes

wferi added 4 commits December 8, 2016 14:10

Fix typos: synchonization -> synchronization, paramaters -> parameters

27d9b3b

configure: help string cleanup

ff7a3f4

The [ default="no" ] branches were sense- and (mostly) harmless.

configure: LTLIBOBJS is also a Make variable

6437130

So let's use the more friendly syntax.

configure: restrict -ldl to where it's actually needed

56754d0

This reduces overlinking of qb-blackbox. Being a seldom used executable, the gains are mostly theoretical, but at least this silences warnings from some QA tools.

wferi force-pushed the autoconf branch from a0abc52 to 67441f1 Compare December 8, 2016 13:17

wferi closed this Dec 8, 2016

jnpkrn reviewed Dec 9, 2016

View reviewed changes

wferi reopened this Dec 9, 2016

wferi added 3 commits December 12, 2016 11:46

configure: restrict pthreads to where it's actually needed

cb5ee92

mq_open() is no longer relevant beyond 70a9623 (Remove message queues).

configure: restrict socket lib to where it's actually needed

1a9b412

configure: restrict nsl lib to where it's actually needed

8fad8ac

wferi force-pushed the autoconf branch from 67441f1 to 8fad8ac Compare December 12, 2016 10:47

chrissie-c merged commit 8fad8ac into ClusterLabs:master Dec 16, 2016

chrissie-c added a commit that referenced this pull request Dec 16, 2016

Merge pull request #241 from jnpkrn/PR240-addendum

5a54a51

PR #240 + addendum (drop syslog-tests opt-in switch)

jnpkrn mentioned this pull request May 19, 2017

build: follow-up on introducing custom m4 macros #253

Merged

inouekazu mentioned this pull request Aug 15, 2017

configure: define AS_VAR_COPY #267

Merged

wferi deleted the autoconf branch October 12, 2017 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mostly Autoconf cleanups #240

Mostly Autoconf cleanups #240

wferi commented Dec 7, 2016

jnpkrn commented Dec 7, 2016

jnpkrn Dec 7, 2016

wferi Dec 7, 2016

jnpkrn Dec 7, 2016

jnpkrn Dec 7, 2016

wferi Dec 8, 2016

jnpkrn Dec 8, 2016 •

edited

Loading

wferi commented Dec 8, 2016

jnpkrn commented Dec 8, 2016 •

edited

Loading

wferi commented Dec 8, 2016

jnpkrn commented Dec 8, 2016 via email

jnpkrn Dec 9, 2016 •

edited

Loading

wferi Dec 9, 2016

jnpkrn commented Dec 12, 2016 via email

wferi commented Dec 12, 2016 via email

wferi commented Dec 12, 2016

jnpkrn commented Dec 12, 2016 via email

chrissie-c commented Dec 12, 2016

jnpkrn commented Dec 16, 2016

wferi commented Dec 18, 2016

jnpkrn commented Dec 21, 2016 via email

wferi commented Dec 21, 2016 via email

wferi commented May 31, 2017

jnpkrn commented May 31, 2017 via email

inouekazu commented Jul 31, 2017 •

edited

Loading

wferi commented Jul 31, 2017

inouekazu commented Aug 8, 2017

Mostly Autoconf cleanups #240

Mostly Autoconf cleanups #240

Conversation

wferi commented Dec 7, 2016

jnpkrn commented Dec 7, 2016

jnpkrn Dec 7, 2016

Choose a reason for hiding this comment

wferi Dec 7, 2016

Choose a reason for hiding this comment

jnpkrn Dec 7, 2016

Choose a reason for hiding this comment

jnpkrn Dec 7, 2016

Choose a reason for hiding this comment

wferi Dec 8, 2016

Choose a reason for hiding this comment

jnpkrn Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

wferi commented Dec 8, 2016

jnpkrn commented Dec 8, 2016 • edited Loading

wferi commented Dec 8, 2016

jnpkrn commented Dec 8, 2016 via email

jnpkrn Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

wferi Dec 9, 2016

Choose a reason for hiding this comment

jnpkrn commented Dec 12, 2016 via email

wferi commented Dec 12, 2016 via email

wferi commented Dec 12, 2016

jnpkrn commented Dec 12, 2016 via email

chrissie-c commented Dec 12, 2016

jnpkrn commented Dec 16, 2016

wferi commented Dec 18, 2016

jnpkrn commented Dec 21, 2016 via email

wferi commented Dec 21, 2016 via email

wferi commented May 31, 2017

jnpkrn commented May 31, 2017 via email

inouekazu commented Jul 31, 2017 • edited Loading

wferi commented Jul 31, 2017

inouekazu commented Aug 8, 2017

jnpkrn Dec 8, 2016 •

edited

Loading

jnpkrn commented Dec 8, 2016 •

edited

Loading

jnpkrn Dec 9, 2016 •

edited

Loading

inouekazu commented Jul 31, 2017 •

edited

Loading