From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Date: Wed, 1 Oct 2014 19:44:15 +0200
Subject: [Buildroot] [PATCH 4/5] x264: new package
In-Reply-To: <1412154841-708-1-git-send-email-0intro@gmail.com>
References: <1412154841-708-1-git-send-email-0intro@gmail.com>
Message-ID: <20141001194415.3519dc04@free-electrons.com>
List-Id: <buildroot.busybox.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: buildroot@busybox.net

Dear David du Colombier,

On Wed,  1 Oct 2014 11:14:00 +0200, David du Colombier wrote:
> This package is based on an earlier package
> proposed by Ayaka in December 2013.

Thanks for reviving this patch, definitely appreciated!

One question: we received only PATCH 4/5 and PATCH 5/5 from your
series. Is this normal? Maybe it's just a small mistake on your side
when generating the patches.

Some comments below.


> diff --git a/package/Config.in b/package/Config.in
> index 2ad72bc..33616e1 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -33,6 +33,7 @@ menu "Audio and video applications"
>  	source "package/vlc/Config.in"
>  	source "package/vorbis-tools/Config.in"
>  	source "package/wavpack/Config.in"
> +	source "package/x264/Config.in"

I'd x264 is mostly a library, so I'd prefer to see it in the libraries.


> diff --git a/package/x264/x264.mk b/package/x264/x264.mk
> new file mode 100644
> index 0000000..dff2313
> --- /dev/null
> +++ b/package/x264/x264.mk
> @@ -0,0 +1,42 @@
> +###############################################################
> +#
> +# x264
> +#
> +###############################################################
> +
> +X264_VERSION = 20140930-2245-stable
> +X264_SOURCE = x264-snapshot-$(X264_VERSION).tar.bz2
> +X264_SITE= ftp://ftp.videolan.org/pub/videolan/x264/snapshots
> +X264_LICENSE = GPL

The license is actually GPLv2+.

> +X264_LICENSE_FILES = COPYING
> +X264_INSTALL_STAGING = YES
> +X264_INSTALL_TARGET = YES

Line not needed.

> +X264_DEPENDENCIES = host-pkgconf
> +

Might be worth adding a comment right before X264_CONFIGURE_CMDS to
indicate that the configure script is not generated by autoconf.

> +define X264_CONFIGURE_CMDS
> +	(cd $(@D);./configure \
> +		--prefix=/usr \
> +		--host="$(GNU_TARGET_NAME)" \
> +		--cross-prefix="$(TARGET_CROSS)" \
> +		--enable-static \
> +		--enable-strip \
> +		--enable-pic \
> +		--enable-shared \

--enable-shared will not work for static only builds. Maybe you should
try something such as:

X264_CONF_OPTS = --enable-static

ifeq ($(BR2_PREFER_STATIC_LIB),)
X264_CONF_OPTS = --enable-pic --enable-shared
endif

and then use $(X264_CONF_OPTS) when calling the configure script.

If you want to try a purely static configuration, you can try with the
basic configuration at
http://autobuild.buildroot.org/toolchains/configs/bfin-uclinux.config.

> +		--disable-ffms \
> +		--disable-cli \
> +	)
> +endef
> +
> +define X264_BUILD_CMDS
> +	$(MAKE) CC="$(TARGET_CC)" -C $(@D)

Could you try:

	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D)

 ?

CC="$(TARGET_CC)" as well as many other useful variables are passed in
$(TARGET_CONFIGURE_OPTS). It's also a good habit to pass
$(TARGET_MAKE_ENV) in the environment.

> +endef
> +
> +define X264_INSTALL_STAGING_CMDS
> +	$(MAKE) DESTDIR="$(STAGING_DIR)" -C $(@D) install

Also pass $(TARGET_MAKE_ENV).

> +endef
> +
> +define X264_INSTALL_TARGET_CMDS
> +	$(MAKE) DESTDIR="$(TARGET_DIR)" -C $(@D) install

Ditto.

> +endef
> +
> +$(eval $(generic-package))

Could you respin after fixing those minor issues?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com