Re: [Buildroot] [PATCH v4] package/libgdal: new package

[Arnout Vandecappelle <arnout@mind.be>]

 Hi Dominik,

 I still have a few comments. I could make the necessary changes myself, but I'm
feeling lazy today :-).

On 02/08/2021 11:24, Dominik Michael Rauh wrote:
> GDAL is a translator library for raster and vector geospatial data
> formats. As a library, it presents a single raster abstract data model
> and single vector abstract data model to the calling application for all
> supported formats. It also comes with a variety of useful command line
> utilities for data translation and processing.
> 
> https://gdal.org/
> 
> Signed-off-by: Dominik Michael Rauh <dmrauh@posteo.de>

 It's really nice that you add this package, because when I added postgis
(contributed by Maxim) I didn't notice that it had on optional dependency on
GDAL, which wasn't added yet...

> ---
> Changes v3 -> v4 (after review by Thomas Petazzoni):
> -  Bump version from 3.3.0 to 3.3.1
> -  Configure libgdal to use external libraries where possible
> -  Explicitely disable libraries not yet handled by Buildroot
> 
> Changes v2 -> v3:
> -  Bump version from 3.2.2 to 3.3.0
> 
> Changes v1 -> v2 (after review by Peter Seiderer):
> -  Disable NEON and VSX support when using libgdal's libpng
> -  Disable compilation for toolchains with binutils bug 21464 or 27597
> -  Add the proper "depends" demanded by proj
> -  Fix the comment in Config.in
> -  Hopefully add the complete LIBGDAL_LICENSE information
> 
>  package/Config.in                             |   1 +
>  ...1-port-cpl_recode_iconv.cpp-use-cast.patch |  38 ++++++
>  package/libgdal/Config.in                     |  31 +++++
>  package/libgdal/libgdal.hash                  |   6 +
>  package/libgdal/libgdal.mk                    | 121 ++++++++++++++++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 package/libgdal/0001-port-cpl_recode_iconv.cpp-use-cast.patch
>  create mode 100644 package/libgdal/Config.in
>  create mode 100644 package/libgdal/libgdal.hash
>  create mode 100644 package/libgdal/libgdal.mk
> 
> diff --git a/package/Config.in b/package/Config.in
> index 046c04e994..dbbdc35390 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1929,6 +1929,7 @@ menu "Other"
>  	source "package/libevdev/Config.in"
>  	source "package/libevent/Config.in"
>  	source "package/libffi/Config.in"
> +	source "package/libgdal/Config.in"
>  	source "package/libgee/Config.in"
>  	source "package/libgeos/Config.in"
>  	source "package/libglib2/Config.in"
> diff --git a/package/libgdal/0001-port-cpl_recode_iconv.cpp-use-cast.patch b/package/libgdal/0001-port-cpl_recode_iconv.cpp-use-cast.patch
> new file mode 100644
> index 0000000000..9fa958524f
> --- /dev/null
> +++ b/package/libgdal/0001-port-cpl_recode_iconv.cpp-use-cast.patch
> @@ -0,0 +1,38 @@
> +From 0730ebc7a1e22a169bf3a1d873e130e079a68b3d Mon Sep 17 00:00:00 2001
> +From: Dominik Michael Rauh <dmrauh@posteo.de>
> +Date: Sat, 1 May 2021 20:11:30 +0200
> +Subject: [PATCH] port/cpl_recode_iconv.cpp: use cast
> +
> +Fixes error: invalid cast from type 'int' to type 'iconv_t' {aka 'long
> +int'}
> +
> +Signed-off-by: Dominik Michael Rauh <dmrauh@posteo.de>

 Please add a line here with the upstream status. The intention is that you
contribute the patch to the upstream project, so it can be removed again after
updating to a future release.

 If for any reason the patch is not applicable to the upstream project, please
indicate why not

> +---
> + port/cpl_recode_iconv.cpp | 4 ++--
> + 1 file changed, 2 insertions(+), 2 deletions(-)
> +
> +diff --git a/port/cpl_recode_iconv.cpp b/port/cpl_recode_iconv.cpp
> +index d341bb1..2346012 100644
> +--- a/port/cpl_recode_iconv.cpp
> ++++ b/port/cpl_recode_iconv.cpp
> +@@ -87,7 +87,7 @@ char *CPLRecodeIconv( const char *pszSource,
> + 
> +     sConv = iconv_open( pszDstEncoding, pszSrcEncoding );
> + 
> +-    if( sConv == reinterpret_cast<iconv_t>(-1) )
> ++    if( sConv == (iconv_t)(-1) )

 The proper C++ way is to use static_cast. An alternative is to use an explicit
type initilisation. So either

if( sConv == static_cast<iconv_t>(-1) )

or

if( sConv == iconv_t(-1) )


> +     {
> +         CPLError( CE_Warning, CPLE_AppDefined,
> +                   "Recode from %s to %s failed with the error: \"%s\".",
> +@@ -234,7 +234,7 @@ char *CPLRecodeFromWCharIconv( const wchar_t *pwszSource,
> + 
> +     sConv = iconv_open( pszDstEncoding, pszSrcEncoding );
> + 
> +-    if( sConv == reinterpret_cast<iconv_t>(-1) )
> ++    if( sConv == (iconv_t)(-1) )
> +     {
> +         CPLFree( pszIconvSrcBuf );
> +         CPLError( CE_Warning, CPLE_AppDefined,
> +-- 
> +2.31.1
> +
> diff --git a/package/libgdal/Config.in b/package/libgdal/Config.in
> new file mode 100644
> index 0000000000..1f3861e944
> --- /dev/null
> +++ b/package/libgdal/Config.in
> @@ -0,0 +1,31 @@
> +config BR2_PACKAGE_LIBGDAL
> +	bool "libgdal"
> +	depends on BR2_INSTALL_LIBSTDCPP # proj, libjson
> +	# configure can't find proj, when linking statically
> +	depends on !BR2_STATIC_LIBS
> +	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 # C++11, proj
> +	depends on !BR2_TOOLCHAIN_HAS_BINUTILS_BUG_21464
> +	depends on !BR2_TOOLCHAIN_HAS_BINUTILS_BUG_27597

 Please explain in the commit message how you arrive here. So add:

test-pkg shows that this package is affected by binutils bugs 21464 and 27597.

> +	depends on BR2_TOOLCHAIN_HAS_THREADS # proj
> +	depends on BR2_USE_WCHAR # proj
> +	select BR2_PACKAGE_JPEG
> +	select BR2_PACKAGE_LIBJSON
> +	select BR2_PACKAGE_LIBPNG
> +	select BR2_PACKAGE_PROJ
> +	select BR2_PACKAGE_ZLIB
> +	help
> +	  GDAL is a translator library for raster and vector geospatial
> +	  data formats. As a library, it presents a single raster
> +	  abstract data model and single vector abstract data model to
> +	  the calling application for all supported formats. It also
> +	  comes with a variety of useful command line utilities for data
> +	  translation and processing.
> +
> +	  https://gdal.org/
> +
> +comment "libgdal needs a toolchain w/ C++, dynamic library, gcc >= 4.7, not binutils bug 21464, 27597, threads, wchar"
> +	depends on !BR2_INSTALL_LIBSTDCPP || BR2_STATIC_LIBS || \
> +		!BR2_TOOLCHAIN_GCC_AT_LEAST_4_7 || \
> +		BR2_TOOLCHAIN_HAS_BINUTILS_BUG_21464 || \
> +		BR2_TOOLCHAIN_HAS_BINUTILS_BUG_27597 || !BR2_TOOLCHAIN_HAS_THREADS || \
> +		!BR2_USE_WCHAR
> diff --git a/package/libgdal/libgdal.hash b/package/libgdal/libgdal.hash
> new file mode 100644
> index 0000000000..ea770a38d2
> --- /dev/null
> +++ b/package/libgdal/libgdal.hash
> @@ -0,0 +1,6 @@
> +# md5 from: https://download.osgeo.org/gdal/3.3.1/gdal-3.3.1.tar.xz.md5, sha256 locally computed:
> +md5  7611300fece06853a1a88149e0cc8922  gdal-3.3.1.tar.xz
> +sha256  48ab00b77d49f08cf66c60ccce55abb6455c3079f545e60c90ee7ce857bccb70  gdal-3.3.1.tar.xz
> +
> +# Hash of license file:
> +sha256  b82e6cca0b13f5db2f22ab667f22254fb1f4b135ea73d5bd6238ef89aff31f6c  LICENSE.TXT
> diff --git a/package/libgdal/libgdal.mk b/package/libgdal/libgdal.mk
> new file mode 100644
> index 0000000000..337b8a7fb7
> --- /dev/null
> +++ b/package/libgdal/libgdal.mk
> @@ -0,0 +1,121 @@
> +################################################################################
> +#
> +# libgdal
> +#
> +################################################################################
> +
> +LIBGDAL_VERSION = 3.3.1
> +LIBGDAL_SITE = https://download.osgeo.org/gdal/$(LIBGDAL_VERSION)
> +LIBGDAL_SOURCE = gdal-$(LIBGDAL_VERSION).tar.xz

 We generally try to follow the name of the upstream package - that's also
easier for CPE and release-monitoring tracking. So it would be better to call
this package gdal instead of libgdal.


> +LIBGDAL_LICENSE = MIT (GDAL/OGR), BSD-3-Clause, BSD-Style, APACHE-2.0

 Where does the BSD-Style come from?

> +LIBGDAL_LICENSE_FILES = LICENSE.TXT

 PROVENANCE.txt gives a nice overview of the different, complicated licenses, so
I think it's useful to add that as well.

> +LIBGDAL_INSTALL_STAGING = YES
> +LIBGDAL_CONFIG_SCRIPTS = gdal-config
> +# libgdal at its core only needs host-pkgconf, libgeotiff, proj and tiff but
> +# since by default mrf driver support is enabled, it also needs jpeg, libpng
> +# and zlib. By default there are also many other drivers enabled but it seems,
> +# in contrast to mrf driver support, that they can be implicitely disabled, by
> +# configuring libgdal without their respectively needed dependencies.
> +LIBGDAL_DEPENDENCIES = host-pkgconf jpeg libgeotiff libpng proj tiff zlib
> +
> +ifeq ($(BR2_PACKAGE_POSTGRESQL),y)
> +LIBGDAL_DEPENDENCIES += postgresql
> +LIBGDAL_CONF_OPTS += --with-pg
> +else
> +LIBGDAL_CONF_OPTS += --without-pg
> +endif
> +
> +ifeq ($(BR2_PACKAGE_LIBXML2),y)
> +LIBGDAL_DEPENDENCIES += libxml2
> +LIBGDAL_CONF_OPTS += --with-xml2
> +else
> +LIBGDAL_CONF_OPTS += --without-xml2
> +endif
> +
> +LIBGDAL_CONF_OPTS += --with-cpp14 \

 In Config.in, the comment is C++11 and the minimum GCC version is 4.7. Here,
however, you claim that C++14 is supported, which means 4.9 or 5 (depending on
exactly which features are used). Perhaps just remove the option and let the
configure script figure it out?

> +					 --with-geotiff \

 Continuation lines should be indented with a single tab.

 You can make them align nicely by writing:

LIBGDAL_CONF_OPTS += \
	--with-cpp14 \
	--with-geotiff \


 Regards,
 Arnout

> +					 --with-libjson-c \
> +					 --with-libtiff \
> +					 --with-libtool \
> +					 --with-libz \
> +					 --with-jpeg \
> +					 --with-png \
> +					 --with-proj
> +
> +# disable not yet handled packages and thus the associated libgdal drivers
> +LIBGDAL_CONF_OPTS += --without-armadillo \
> +					 --without-cfitsio \
> +					 --without-crypto \
> +					 --without-cryptopp \
> +					 --without-curl \
> +					 --without-dds \
> +					 --without-dods-root \
> +					 --without-ecw \
> +					 --without-expat \
> +					 --without-exr \
> +					 --without-fgdb \
> +					 --without-fme \
> +					 --without-freexl \
> +					 --without-geos \
> +					 --without-gnm \
> +					 --without-libkml \
> +					 --without-grass \
> +					 --without-libgrass \
> +					 --without-gta \
> +					 --without-hdf4 \
> +					 --without-hdf5 \
> +					 --without-hdfs \
> +					 --without-heif \
> +					 --without-idb \
> +					 --without-ingres \
> +					 --without-java \
> +					 --without-jp2lura \
> +					 --without-jpeg12 \
> +					 --without-jasper \
> +					 --without-charls \
> +					 --without-kakadu \
> +					 --without-kea \
> +					 --without-lerc \
> +					 --without-gif \
> +					 --without-liblzma \
> +					 --without-libdeflate \
> +					 --without-mdb \
> +					 --without-mongocxx \
> +					 --without-mongocxxv3 \
> +					 --without-mrsid \
> +					 --without-jp2mrsid \
> +					 --without-macosx-framework \
> +					 --without-mrsid_lidar \
> +					 --without-msg \
> +					 --without-mysql \
> +					 --without-netcdf \
> +					 --without-null \
> +					 --without-oci \
> +					 --without-odbc \
> +					 --without-ogdi \
> +					 --without-opencl \
> +					 --without-openjpeg \
> +					 --without-pam \
> +					 --without-pcidsk \
> +					 --without-pcraster \
> +					 --without-pcre \
> +					 --without-pdfium \
> +					 --without-perl \
> +					 --without-podofo \
> +					 --without-poppler \
> +					 --without-python \
> +					 --without-qhull \
> +					 --without-rasdaman \
> +					 --without-rasterlite2 \
> +					 --without-rdb \
> +					 --without-sfcgal \
> +					 --without-sosi \
> +					 --without-spatialite \
> +					 --without-sqlite3 \
> +					 --without-teigha \
> +					 --without-tiledb \
> +					 --without-webp \
> +					 --without-xerces \
> +					 --without-zstd
> +
> +$(eval $(autotools-package))
> 
_______________________________________________
buildroot mailing list
buildroot@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot