[Buildroot] [PATCH 1/1] cminpack: new package

Mathieu Mirmont mat at ampyxpower.com
Fri Oct 28 13:23:41 UTC 2016


Thanks for the feedback, I'll send an updated patch!


On 28/10/16 14:51, Thomas Petazzoni wrote:
> Hello,
> 
> Thanks for your contribution! It looks mostly good, there are just a
> few things to address, see below.
> 
> On Fri, 28 Oct 2016 14:31:06 +0200, Mathieu Mirmont wrote:
> 
>> diff --git a/package/cminpack/cminpack-0001-Do-not-install-to-lib64-on-x86_64-platforms.patch b/package/cminpack/cminpack-0001-Do-not-install-to-lib64-on-x86_64-platforms.patch
> 
> This is not the correct naming for patches, it should be just
> 0001-Do-not-install-to-lib64-on-x86_64-platforms.patch, i.e there is no
> need to repeat the name of the package.
> 
>> new file mode 100644
>> index 0000000..d7d66e2
>> --- /dev/null
>> +++ b/package/cminpack/cminpack-0001-Do-not-install-to-lib64-on-x86_64-platforms.patch
>> @@ -0,0 +1,28 @@
>> +From 86f361b980f009ec2bdeea6de6972fe5523420e3 Mon Sep 17 00:00:00 2001
>> +From: Mathieu Mirmont <mat at ampyxpower.com>
>> +Date: Wed, 24 Jun 2015 11:32:51 +0200
>> +Subject: [PATCH 1/2] Do not install to lib64 on x86_64 platforms.
> 
> Please generate your patches with "git format-patch -N" so that the 1/2
> and 2/2 information is not mentioned.
> 
> Also, please add a Signed-off-by line with your name.
> 
>> +
>> +---
>> + cmake/cminpack_utils.cmake | 5 -----
>> + 1 file changed, 5 deletions(-)
>> +
>> +diff --git a/cmake/cminpack_utils.cmake b/cmake/cminpack_utils.cmake
>> +index e380d5c..a0896d5 100644
>> +--- a/cmake/cminpack_utils.cmake
>> ++++ b/cmake/cminpack_utils.cmake
>> +@@ -8,11 +8,6 @@ macro(GET_OS_INFO)
>> +     if(NOT DEFINED CMINPACK_LIB_INSTALL_DIR)
>> +     set(CMINPACK_LIB_INSTALL_DIR "lib")
>> +     if(OS_LINUX)
>> +-        if(${CMAKE_SYSTEM_PROCESSOR} STREQUAL "x86_64")
>> +-            set(CMINPACK_LIB_INSTALL_DIR "lib64")
>> +-        else(${CMAKE_SYSTEM_PROCESSOR} STREQUAL "x86_64")
>> +-            set(CMINPACK_LIB_INSTALL_DIR "lib")
>> +-        endif(${CMAKE_SYSTEM_PROCESSOR} STREQUAL "x86_64")
> 
> Is this patch really needed? Buildroot installs lib64 -> lib and
> usr/lib64 -> usr/lib symbolic links, so it should just work even if
> your package installs in lib64. Have you faced a specific issue?
> 
> 
>> diff --git a/package/cminpack/cminpack-0002-Do-not-install-headers-cminpack.pc-and-FindCMinpack.patch b/package/cminpack/cminpack-0002-Do-not-install-headers-cminpack.pc-and-FindCMinpack.patch
>> new file mode 100644
>> index 0000000..c517ed1
>> --- /dev/null
>> +++ b/package/cminpack/cminpack-0002-Do-not-install-headers-cminpack.pc-and-FindCMinpack.patch
>> @@ -0,0 +1,39 @@
>> +From 2a213a12674c76e70171cf6f0813479613511a07 Mon Sep 17 00:00:00 2001
>> +From: Mathieu Mirmont <mat at ampyxpower.com>
>> +Date: Wed, 24 Jun 2015 12:26:22 +0200
>> +Subject: [PATCH 2/2] Do not install headers, cminpack.pc and
>> + FindCMinpack.cmake
>> +
> 
> Same comments as the previous patch: naming, git format-patch -N and
> Signed-off-by line.
> 
> But I'm also questioning why you are adding this patch. All those files
> should clearly be installed in $(STAGING_DIR) so that other
> programs/libraries can link against cminpack. And if you added this
> because you're worried about polluting $(TARGET_DIR), then you
> shouldn't be worried: we have some generic logic in Buildroot that
> removes all header files, .pc files and various other
> development-related files from $(TARGET_DIR). It is done at the end of
> the build, right before generating the filesystem image.
> 
>> diff --git a/package/cminpack/cminpack.mk b/package/cminpack/cminpack.mk
>> new file mode 100644
>> index 0000000..229c5c2
>> --- /dev/null
>> +++ b/package/cminpack/cminpack.mk
>> @@ -0,0 +1,7 @@
> 
> Please add the comment block we have in every package. Yes, it's a
> silly useless comment block, but we have it in every package, so it
> should be there as well :)
> 
>> +CMINPACK_VERSION = 1.3.4
>> +CMINPACK_SOURCE = cminpack-$(CMINPACK_VERSION).tar.gz
> 
> This assignment is useless, as it is the default value.
> 
>> +CMINPACK_SITE = http://devernay.free.fr/hacks/cminpack
>> +
>> +CMINPACK_CONF_OPTS = -DUSE_FPIC=ON -DSHARED_LIBS=ON
> 
> This line makes the assumption that support for shared libraries is
> available, which is not always the case. So you have two options here:
> 
>  1. Your package supports being built in static library only
>     environment (which normally CMake based packages are capable of).
>     In this case, you should support it.
> 
>  2. If for some reason, building in a static library only configuration
>     isn't supported, then your package should "depends
>     on !BR2_STATIC_LIBS".
> 
> Thanks!
> 
> Thomas
> 


-- 
Mathieu Mirmont <mat at ampyxpower.com>
Lead Software Engineer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 829 bytes
Desc: OpenPGP digital signature
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20161028/9149f47e/attachment-0002.asc>


More information about the buildroot mailing list