On 2013-03-06 10:45 AM, Waldemar Brodkorb wrote:
> Hi,
> 
> I would like to work on a better BSD support as a host system
> for OpenWrt. Here is my first patch which fixes a lot of compile
> errors on OpenBSD.
> 
> What you think about this patch?
Approach mostly looks OK, some comments below:


> Index: tools/e2fsprogs/patches/003-openbsd-compat.patch
> ===================================================================
> --- tools/e2fsprogs/patches/003-openbsd-compat.patch  (revision 0)
> +++ tools/e2fsprogs/patches/003-openbsd-compat.patch  (working copy)
> @@ -0,0 +1,22 @@
> +diff -Nur e2fsprogs-1.42.7.orig/lib/blkid/getsize.c 
> e2fsprogs-1.42.7/lib/blkid/getsize.c
> +--- e2fsprogs-1.42.7.orig/lib/blkid/getsize.c        Fri Nov 30 03:40:18 2012
> ++++ e2fsprogs-1.42.7/lib/blkid/getsize.c     Tue Mar  5 14:55:43 2013
> +@@ -26,6 +26,7 @@
> + #include <fcntl.h>
> + #ifdef HAVE_SYS_IOCTL_H
> + #include <sys/ioctl.h>
> ++#include <sys/dkio.h>
> + #endif
> + #ifdef HAVE_LINUX_FD_H
> + #include <linux/fd.h>
I think this might break on Linux systems, probably needs an #ifdef


> Index: tools/quilt/Makefile
> ===================================================================
> --- tools/quilt/Makefile      (revision 35879)
> +++ tools/quilt/Makefile      (working copy)
> @@ -15,11 +15,10 @@
>  
>  include $(INCLUDE_DIR)/host-build.mk
>  
> -GETOPT:=$(shell which getopt)
> -
>  HOST_CONFIGURE_ARGS += \
> -     --with-patch=$(PATCH) \
> +     --with-patch=$(STAGING_DIR_HOST)/bin/patch \
>       --with-find=$(FIND) \
> +     --with-diff=$(DIFF) \
>       --with-getopt=$(GETOPT)
>  
>  define Host/Configure
Is this really necessary? If you use PrepareCommand in tools/Makefile,
it ends up being in $PATH before the regular system PATH contents.

> Index: tools/patch/Makefile
> ===================================================================
> --- tools/patch/Makefile      (revision 0)
> +++ tools/patch/Makefile      (working copy)
> @@ -0,0 +1,38 @@
> +# 
> +# Copyright (C) 2013 OpenWrt.org
> +#
> +# This is free software, licensed under the GNU General Public License v2.
> +# See /LICENSE for more information.
> +#
> +include $(TOPDIR)/rules.mk
> +
> +PKG_NAME:=patch
> +PKG_VERSION:=2.7.1
> +
> +PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.xz
> +PKG_SOURCE_URL:=ftp://ftp.gnu.org/gnu/patch
> +PKG_MD5SUM:=e9ae5393426d3ad783a300a338c09b72
> +
> +include $(INCLUDE_DIR)/host-build.mk
> +
> +#HOST_CONFIGURE_ARGS += \
> +
> +#define Host/Configure
> +#    cd $(HOST_BUILD_DIR) && autoconf
> +#    $(call Host/Configure/Default)
> +#    [ -f $(HOST_BUILD_DIR)/Makefile ]
> +#endef
> +
> +#define Host/Compile
> +#    $(MAKE) -C $(HOST_BUILD_DIR) SHELL="$(BASH)" all lib/backup-files
> +#endef
> +
> +#define Host/Install
> +#    $(MAKE) -C $(HOST_BUILD_DIR) SHELL="$(BASH)" install
> +#endef
> +
> +#define Host/Clean
> +#    rm -f $(STAGING_DIR_HOST)/bin/quilt
> +#endef
> +
> +$(eval $(call HostBuild))
Please delete the lines that were commented out here.

> Index: tools/Makefile
> ===================================================================
> --- tools/Makefile    (revision 35879)
> +++ tools/Makefile    (working copy)
> @@ -15,7 +15,7 @@
>  endif
>  tools-y += m4 libtool autoconf automake flex bison pkg-config sed mklibs
>  tools-y += sstrip ipkg-utils genext2fs e2fsprogs mtd-utils mkimage
> -tools-y += firmware-utils patch-image quilt yaffs2 flock padjffs2
> +tools-y += firmware-utils patch-image patch quilt yaffs2 flock padjffs2
>  tools-y += mm-macros xorg-macros xfce-macros missing-macros xz cmake scons
>  tools-$(CONFIG_TARGET_orion_generic) += wrt350nv2-builder upslug2
>  tools-$(CONFIG_powerpc) += upx
> @@ -128,8 +128,10 @@
>  $(eval $(call PrepareCommand,md5sum,gmd5sum md5sum $(SCRIPT_DIR)/md5sum))
>  $(eval $(call PrepareCommand,cp,gcp cp))
>  $(eval $(call PrepareCommand,seq,gseq seq))
> -$(eval $(call PrepareCommand,python,python2 python))
> +$(eval $(call PrepareCommand,python,python2 python2.7 python))
>  $(eval $(call PrepareCommand,awk,gawk awk))
> +$(eval $(call PrepareCommand,getopt,gnugetopt getopt))
> +$(eval $(call PrepareCommand,grep,ggrep grep))
>  
>  $(curdir)/cmddeps = $(patsubst %,$(STAGING_DIR_HOST)/bin/%,find md5sum cp 
> stat seq python awk)
>  $(curdir)//prepare = $(STAGING_DIR)/.prepared $(STAGING_DIR_HOST)/.prepared 
> $($(curdir)/cmddeps)
> Index: include/host-build.mk
> ===================================================================
> --- include/host-build.mk     (revision 35879)
> +++ include/host-build.mk     (working copy)
> @@ -71,7 +71,7 @@
>  
>  HOST_MAKE_FLAGS =
>  
> -HOST_CONFIGURE_CMD = ./configure
> +HOST_CONFIGURE_CMD = $(BASH) ./configure
>  
>  ifneq ($(HOST_OS),Darwin)
>    ifeq ($(CONFIG_BUILD_STATIC_TOOLS),y)
This looks like a workaround for a specific package that doesn't build.
Please provide some details why this is necessary.

> Index: include/prereq-build.mk
> ===================================================================
> --- include/prereq-build.mk   (revision 35879)
> +++ include/prereq-build.mk   (working copy)
> @@ -39,6 +39,7 @@
>  ))
>  
>  define Require/getopt
> +     gnugetopt --help 2>&1 | grep long >/dev/null || \
>       getopt --help 2>&1 | grep long >/dev/null
>  endef
>  $(eval $(call Require,getopt, \
> @@ -134,15 +135,19 @@
>       Please install bzip2. \
>  ))
>  
> -$(eval $(call RequireCommand,patch, \
> -     Please install patch. \
> -))
> +#$(eval $(call RequireCommand,patch, \
> +#    Please install patch. \
> +#))
Please delete it instead of commenting it out.

>  $(eval $(call RequireCommand,perl, \
>       Please install perl. \
>  ))
>  
> -$(eval $(call RequireCommand,python, \
> +define Require/python
> +     $(PYTHON) --version 2>&1 > /dev/null
> +endef
> +
> +$(eval $(call Require,python, \
>       Please install python. \
>  ))
>  
Why the --version check instead of just RequireCommand?

> Index: include/host.mk
> ===================================================================
> --- include/host.mk   (revision 35879)
> +++ include/host.mk   (working copy)
> @@ -62,6 +62,15 @@
>               PATCH=`which gpatch 2>/dev/null`; \
>               [ -n "$$PATCH" -a -x "$$PATCH" ] || PATCH=`which patch 
> 2>/dev/null`; \
>               echo "PATCH:=$$PATCH" >> $@; \
> +             DIFF=`which gdiff 2>/dev/null`; \
> +             [ -n "$$DIFF" -a -x "$$DIFF" ] || DIFF=`which diff 
> 2>/dev/null`; \
> +             echo "DIFF:=$$DIFF" >> $@; \
> +             GETOPT=`which gnugetopt 2>/dev/null`; \
> +             [ -n "$$GETOPT" -a -x "$$GETOPT" ] || GETOPT=`which getopt 
> 2>/dev/null`; \
> +             echo "GETOPT:=$$GETOPT" >> $@; \
> +             PYTHON=`which python2.7 2>/dev/null`; \
> +             [ -n "$$PYTHON" -a -x "$$PYTHON" ] || PYTHON=`which python 
> 2>/dev/null`; \
> +             echo "PYTHON:=$$PYTHON" >> $@; \
>       )
>  
>  endif
Why not use PrepareCommand in tools/Makefile for those?

Thanks,

- Felix
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Reply via email to