From mboxrd@z Thu Jan  1 00:00:00 1970
From: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
Date: Wed, 30 Mar 2016 11:17:56 +0530
Subject: [Buildroot] [PATCH] powerpc-utils: Bump powerpc-utils to v1.3.0
In-Reply-To: <20160329150150.63d5f136@free-electrons.com>
References: <20160329121047.3804.35383.stgit@hegdevasant.in.ibm.com>
 <20160329150150.63d5f136@free-electrons.com>
Message-ID: <56FB688C.4010000@linux.vnet.ibm.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

On 03/29/2016 06:31 PM, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 29 Mar 2016 17:41:07 +0530, Vasant Hegde wrote:
>> This patch makes below changes to powerpc-utils package:
>>    - Update to latest upstream version (v1.3.0)
>>    - Update License (from CPL to GPLv2)
>>    - Update source link (from SF to github)
>>    - Disable librtas by default
>>    - Finally make necessary adjustment to compile the source
>>      (run autogen.sh before ./configure as we don't have configure in new tarball).
>>
>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>

Thomas,

> Thanks for this patch! It looks mostly good, but there are a few issues
> here and there. Read on below.

Thanks for the detailed review.

>
>> @@ -18,7 +18,7 @@ config BR2_PACKAGE_POWERPC_UTILS_RTAS
>>   	bool "RTAS support"
>>   	select BR2_PACKAGE_LIBRTAS
>>   	depends on BR2_TOOLCHAIN_USES_GLIBC
>> -	default y
>> +	default n
>
> Then just remove this line, because "disabled" in the default state for

Fixed.

> an option. BTW, your commit log just says that you disable it by
> default, but not why. What is the reasoning? Not that I personally care
> much about powerpc-utils and specifically its rtas support, but it
> might be useful to have a short explanation about this change.

Agreed. I should have explained the reason.

RTAS:
      This package contains few tools (like nvram, ppc64_cpu, etc) which are not
       dependent on RTAS support. Traditionally we always had RTAS support (at least
       on IBM Power system). But now a days we do have environments like PowerNV 
host
       where we do not have RTAS support. Hence lets disable RTAS by default. If
       someone wants to build powerpc-utils with RTAS they can enable it.

I will add this to patch description.


>
>> diff --git a/package/powerpc-utils/powerpc-utils.mk b/package/powerpc-utils/powerpc-utils.mk
>> index ae4d662..61b0fbe 100644
>> --- a/package/powerpc-utils/powerpc-utils.mk
>> +++ b/package/powerpc-utils/powerpc-utils.mk
>> @@ -4,15 +4,21 @@
>>   #
>>   ################################################################################
>>
>> -POWERPC_UTILS_VERSION = 1.2.24
>> -POWERPC_UTILS_SITE = http://downloads.sourceforge.net/project/powerpc-utils/powerpc-utils
>> +POWERPC_UTILS_VERSION = 1.3.0
>> +POWERPC_UTILS_SITE = https://github.com/nfont/powerpc-utils/archive/
>
> Since the tarballs are not uploaded by the developer, but directly
> generated by Github, please use the "github" macro in Buildroot. Check
> in other packages and in the Buildroot manual for details on how to use
> it.

Yep. I should have explored this option. Fixed in v2.

>
>> +POWERPC_UTILS_SOURCE = v$(POWERPC_UTILS_VERSION).tar.gz
>>   POWERPC_UTILS_DEPENDENCIES = zlib
>> -POWERPC_UTILS_LICENSE = Common Public License Version 1.0
>> -POWERPC_UTILS_LICENSE_FILES = COPYRIGHT
>> +POWERPC_UTILS_LICENSE = GPLv2
>
> Just to verify: did you make sure it's actually GPLv2 and not GPLv2+ ?

Yes. Its GPLv2.

>
>> +POWERPC_UTILS_LICENSE_FILES = COPYING
>>
>>   POWERPC_UTILS_CONF_ENV = \
>>   	ax_cv_check_cflags___fstack_protector_all=$(if $(BR2_TOOLCHAIN_HAS_SSP),yes,no)
>>
>> +define POWERPC_UTILS_AUTOGEN
>> +	cd $(@D) && PATH=$(BR_PATH) ./autogen.sh
>> +endef
>> +POWERPC_UTILS_PRE_CONFIGURE_HOOKS += POWERPC_UTILS_AUTOGEN
>
> This is not good, because you don't depend on host-autoconf,
> host-automake and host-libtool. Instead, Buildroot has a built-in
> mechanism to regenerate the automake/autoconf stuff. Just do:
>
> POWERPC_UTILS_AUTORECONF = YES

Fixed.

>
> This works in 99% of the cases. There are a few cases where additional
> tweaks are needed, because the autogen.sh does funky non-standard stuff.
>
> Could you rework your patch to take into account those comments and
> send an updated version?

Sure. Will send v2 soon.

Thanks!
Vasant