[0.4.x] libvisual + libvisual-plugins: configure.ac: Be more helpful in absence of autoconf-archive and/or pkg.m4

[hartwork]

In reaction to #300 (comment)

[eli-schwartz]

As I mentioned in the linked ticket, I'd also suggest doing this for PKG_CHECK_MODULES.

You have some additional checks at the time ./configure is run by the user to verify that $PKG_CONFIG is defined (which is unnecessary, I'd say -- the default PKG_CHECK_MODULES action-if-not-found already suggests an url to get pkg-config, and errors out saying that it could not find pkg-config or pkg-config was too old). Either way, nothing makes sure that pkg.m4 was detected by autoconf, and it's actually possible to have pkg-config installed but not developer tooling such as pkg.m4 (most likely to happen when manually installing autoconf and pkg-config outside of a distro and ending up with different autoconf search paths, I suspect).

[hartwork]

@eli-schwartz I checked autoconf-archive and mis-assumed earlier that macro PKG_CHECK_MODULES is from core Autoconf if it's not from autoconf-archive. That's why I saw no point in forbidding PKG_CHECK_MODULES on m4 level. I found it as part of dev-util/pkgconf in Gentoo now, so it finally makes sense to me. I'll add it in a second…

[hartwork]

@eli-schwartz added and pushed

[eli-schwartz]

You only need to do it once per file.

[hartwork]

You only need to do it once per file.

@eli-schwartz I'm aware. There can either be one central place with all the non-core macros collected or macros are forbidden right next to their use which may help future maintainers discover and adapt use of m4_pattern_forbid. (It will also produce fewer merge conflicts in theory but that won't be relevant in practice for this repository.) Do you have concerns about going forward with this approach?

[eli-schwartz]

It just seems very verbose to me.

That being said, I suppose pkg.m4 has a central place to do this already if you consider PKG_PROG_PKG_CONFIG, so maybe that's an option...

[hartwork]

It just seems very verbose to me.

@eli-schwartz it sure is. I have changed to the centralized version now, pushing in a second…

That being said, I suppose pkg.m4 has a central place to do this already

I'm not sure what you mean. Their [m4_pattern_forbid([^_?PKG_[A-Z_]+$] would be too late and not even run if pkg.m4 is not found. Is that what you referred to? If not: what was the idea?

if you consider PKG_PROG_PKG_CONFIG, so maybe that's an option...

PKG_PROG_PKG_CONFIG was missing from the forbid list, so I'm adding that now too…

How do you feel about the latest version?

[eli-schwartz]

I'm not sure what you mean. Their [m4_pattern_forbid([^_?PKG_[A-Z_]+$] would be too late and not even run if pkg.m4 is not found. Is that what you referred to? If not: what was the idea?

I meant use of pkg.m4 macros implies a central place in your configure.ac to check this: PKG_PROG_PKG_CONFIG.

Technically, you only "need" one macro per file to be forbidden, since using two macros from one file will always either both work, or both fail, and if either one is m4_pattern_forbid'den then the desired end goal of erroring out when pkg.m4 is unavailable is met.

Probably doesn't really matter which way you handle it -- I just thought perhaps it would suffice to place the m4_pattern_forbid once, by line 28. No big deal if you're moving it to a central block anyway. :)

How do you feel about the latest version?

lgtm via eyeball.

There are other unrelated improvements that could be made to pkg-config handling (e.g. if $PKG_CONFIG --atleast-pkgconfig-version 0.14 ; then is redundant, if test x$PKG_CONFIG = x ; then would be covered by dropping custom error messages from PKG_CHECK_MODULES(..., ..., ..., [please install XXX]))

[hartwork]

@eli-schwartz thanks for the idea and the review! Merging…