[Buildroot] [PATCH] new package: minidlna

[Buildroot] [PATCH] new package: minidlna

[Wolfram Gloger]

MiniDLNA is server software with the aim of being fully compliant with
DLNA/UPnP-AV clients.

This new package depends on libav (see my other patch) as I could not
get it to work with the current ffmpeg package in buildroot.

Signed-off-by: Wolfram Gloger <wg@malloc.de>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: buildroot-2011.05-minidlna.diff
Type: application/octet-stream
Size: 12435 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20110701/39f6cadb/attachment.obj>

[Buildroot] [PATCH] new package: minidlna

[Thomas Petazzoni]

Hello Wolfram,

Le Fri, 01 Jul 2011 18:32:02 +0200,
Wolfram Gloger <wg@malloc.de> a ?crit :

> MiniDLNA is server software with the aim of being fully compliant with
> DLNA/UPnP-AV clients.
> 
> This new package depends on libav (see my other patch) as I could not
> get it to work with the current ffmpeg package in buildroot.
> 
> Signed-off-by: Wolfram Gloger <wg@malloc.de>

Could you sent your patch inline ?

Your .mk file lists multiple dependencies, but your Config.in forgets
to "depends on" or "select" them.

Your minidlna-1.0.20-cross.patch file must come with a header detailing
what the patch does and why it is needed, along with a Signed-off-by
line identifying the author of the patch. This patch uses STAGING_DIR
in several places, which makes the patch completely unsuitable for
upstream inclusion. Isn't there a way of making this a little bit more
generic so that there's at least a hope of getting this patch merged
upstream in the minidlna project ?

In the .mk file, remove all useless commented lines.

Also remove the

+		BR2_DEFAULT_KERNEL_HEADERS=$(BR2_DEFAULT_KERNEL_HEADERS) \

line, which apparently is useless.

Regarding the dependencies, is it directly minidlna that depends on
libvorbis, flac, libogg, etc. ? Or is it libav that depends on those ?

Regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [PATCH] new package: minidlna

[Wolfram Gloger]

Hello,

> Could you sent your patch inline ?

Sure, will do.

> Your .mk file lists multiple dependencies, but your Config.in forgets
> to "depends on" or "select" them.

Ok, thanks.  I suspect "depends on" is the correct one?

> Your minidlna-1.0.20-cross.patch file must come with a header detailing
> what the patch does and why it is needed, along with a Signed-off-by
> line identifying the author of the patch.

Will add.

> This patch uses STAGING_DIR
> in several places, which makes the patch completely unsuitable for
> upstream inclusion. Isn't there a way of making this a little bit more
> generic so that there's at least a hope of getting this patch merged
> upstream in the minidlna project ?

Why do you think this makes it completely unsuitable for upstream?
_Some_ form of cross compile support needs to happen, and whether you
call the base dir STAGING_DIR or anything else really shouldn't
matter.  I was careful not to break anything generic in the patch, and
keep it minimal.  Actually, I do intend to propose it for upstream.

BTW an autotools patch was proposed for minidlna some time ago,
but it is of course much bigger and wasn't so far applied.

> In the .mk file, remove all useless commented lines.

I see only one, but ok.

> Also remove the
> 
> +		BR2_DEFAULT_KERNEL_HEADERS=$(BR2_DEFAULT_KERNEL_HEADERS) \
> 
> line, which apparently is useless.

I'll try this.  Grepping I could not see where
BR2_DEFAULT_KERNEL_HEADERS is automagically environment-exported to
config scripts.

> Regarding the dependencies, is it directly minidlna that depends on
> libvorbis, flac, libogg, etc. ? Or is it libav that depends on those ?

You have a good point there.  minidlna actually only uses av_open()
and not any of the vorbis, flac, or ogg library entry points directly,
AFAICS.  So I would say that this is another shortcoming in the
minidlna config script.  Since libav has had native demuxers for
ogg/vorbis and flac for a long time (there is a libvorbis option but
that only enables an alternative encoder which is irrelevant for
minidlna), I think the best solution is simply to get rid of these
dependencies in minidlna.

Thanks,
Wolfram.

[Buildroot] [PATCH] new package: minidlna

[Thomas Petazzoni]

Hello Wolfram,

Le Wed, 13 Jul 2011 18:59:40 +0200,
Wolfram Gloger <wg@malloc.de> a ?crit :

> > Your .mk file lists multiple dependencies, but your Config.in forgets
> > to "depends on" or "select" them.
> 
> Ok, thanks.  I suspect "depends on" is the correct one?

In general for dependencies on libraries, we use "select", because it's
not easy for an user to figure out all the libraries needed to enable
an application. Seeing the dependencies of minidlna, I think I would
use "select".

> > This patch uses STAGING_DIR
> > in several places, which makes the patch completely unsuitable for
> > upstream inclusion. Isn't there a way of making this a little bit more
> > generic so that there's at least a hope of getting this patch merged
> > upstream in the minidlna project ?
> 
> Why do you think this makes it completely unsuitable for upstream?
> _Some_ form of cross compile support needs to happen, and whether you
> call the base dir STAGING_DIR or anything else really shouldn't
> matter.  I was careful not to break anything generic in the patch, and
> keep it minimal.  Actually, I do intend to propose it for upstream.

STAGING_DIR isn't very standard. DESTDIR is probably more widespread,
as it is the variable used by autotools.

> BTW an autotools patch was proposed for minidlna some time ago,
> but it is of course much bigger and wasn't so far applied.

Ok.

> > In the .mk file, remove all useless commented lines.
> 
> I see only one, but ok.

Well, in:

+#MINIDLNA_INSTALL_STAGING = YES
+MINIDLNA_DEPENDENCIES = libav sqlite libid3tag jpeg libexif libogg libvorbis flac
+
+#
+define MINIDLNA_CONFIGURE_CMDS
+	(cd $(@D); rm -f config.h; \
+		BR2_DEFAULT_KERNEL_HEADERS=$(BR2_DEFAULT_KERNEL_HEADERS) \
+		$(TARGET_CONFIGURE_OPTS) \
+		./genconfig.sh \
+	)
+endef
+
+#define MINIDLNA_INSTALL_TARGET_CMDS
+#endef

I see four useless commented lines.

> > Also remove the
> > 
> > +		BR2_DEFAULT_KERNEL_HEADERS=$(BR2_DEFAULT_KERNEL_HEADERS) \
> > 
> > line, which apparently is useless.
> 
> I'll try this.  Grepping I could not see where
> BR2_DEFAULT_KERNEL_HEADERS is automagically environment-exported to
> config scripts.

It is not automagically exported to the environment. I failed to see
that your patch makes use of it. This part is definitely not suitable
for upstream inclusion: BR2_ is a Buildroot specific prefix.

Unless I'm wrong, it is only used to find the OS_NAME and OS_VERSION.
I'd prefer to have the OS_NAME and OS_VERSION passed to the config
script by the Buildroot .mk file. You can hardcode OS_NAME=Linux and
OS_VERSION=2.6.

Any idea how OS_VERSION is used exactly by minidlna ?

> > Regarding the dependencies, is it directly minidlna that depends on
> > libvorbis, flac, libogg, etc. ? Or is it libav that depends on those ?
> 
> You have a good point there.  minidlna actually only uses av_open()
> and not any of the vorbis, flac, or ogg library entry points directly,
> AFAICS.  So I would say that this is another shortcoming in the
> minidlna config script.  Since libav has had native demuxers for
> ogg/vorbis and flac for a long time (there is a libvorbis option but
> that only enables an alternative encoder which is irrelevant for
> minidlna), I think the best solution is simply to get rid of these
> dependencies in minidlna.

Ok.

Thanks for your persistence. minidlna is indeed a very interesting
package to have in Buildroot.

Regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com