Re: [PATCH v2 1/5] connman: Add VPN support

[Phil Blundell <pb@pbcl.net>]

On Mon, 2013-05-13 at 15:49 +0300, Jukka Rissanen wrote:
> +do_install_append_${PN}-vpn() {
> +	mkdir -p ${D}${libdir}/connman/scripts
> +	mkdir -p ${D}${libdir}/connman/plugins-vpn
> +}
> +

Does that actually do anything useful?

>+bb.note( "Adding rdependency on %s to %s" % ( rdepends, package ) )

I know the existing connman.inc does the same thing, but I don't think
we want to proliferate this sort of debugging chit-chat in new code.

Also, the way that this code (both the existing stuff and the new block
you've added) interacts with do_split_packages() is a bit hokey.  You
have a hook function which basically just adds an entry to a list, and
then a separate loop which iterates over the resulting list and fiddles
with the RDEPENDS.  As far as I can tell there's no reason that the
RDEPENDS tinkering couldn't be done inside the hook function itself,
which would avoid having two copies of essentially the same code.

> -FILES_${PN} = "${bindir}/* ${sbindir}/* ${libexecdir}/* ${libdir}/lib*.so.* \
> +FILES_${PN} = "${sbindir}/connmand ${libexecdir}/* ${libdir}/lib*.so.* \
>              ${libdir}/connman/plugins \
> -            ${sysconfdir} ${sharedstatedir} ${localstatedir} \
> -            ${base_bindir}/* ${base_sbindir}/* ${base_libdir}/*.so* ${datadir}/${PN} \
> -            ${datadir}/dbus-1/system-services/*"
> +            ${sysconfdir}/connman ${sysconfdir}/dbus-1/system.d/connman.conf \
> +            @base_contains('DISTRO_FEATURES','sysvinit','${sysconfdir}/init.d','',d)} \
> +            ${sharedstatedir} ${localstatedir} \
> +            ${base_bindir}/* ${base_sbindir}/* ${base_libdir}/*.so* \
> +            ${datadir}/${PN}"

That seems like rather a wide-ranging set of changes.  You prepended
${PN}-vpn to PACKAGES which should mean that it gets first pick of the
FILES.  Are all those changes to FILES_${PN} actually necessary?

p.