diff mbox

iftop: Update to latest version, and drop patch

Message ID 1462454180-27035-1-git-send-email-bert@biot.com
State Changes Requested
Headers show

Commit Message

Bert Vermeulen May 5, 2016, 1:16 p.m. UTC
The patch made sure the ncursesw library was not selected to save space,
but that library doesn't exist in this distribution at all.

Signed-off-by: Bert Vermeulen <bert@biot.com>
---
 package/network/utils/iftop/Makefile                         |  4 ++--
 package/network/utils/iftop/patches/0001-force-ncurses.patch | 12 ------------
 2 files changed, 2 insertions(+), 14 deletions(-)
 delete mode 100644 package/network/utils/iftop/patches/0001-force-ncurses.patch

Comments

John Crispin May 7, 2016, 5:17 a.m. UTC | #1
On 05/05/2016 15:16, Bert Vermeulen wrote:
> The patch made sure the ncursesw library was not selected to save space,
> but that library doesn't exist in this distribution at all.

are you sure that ncursesw is not inside any of the feeds either ?

	John

> 
> Signed-off-by: Bert Vermeulen <bert@biot.com>
> ---
>  package/network/utils/iftop/Makefile                         |  4 ++--
>  package/network/utils/iftop/patches/0001-force-ncurses.patch | 12 ------------
>  2 files changed, 2 insertions(+), 14 deletions(-)
>  delete mode 100644 package/network/utils/iftop/patches/0001-force-ncurses.patch
> 
> diff --git a/package/network/utils/iftop/Makefile b/package/network/utils/iftop/Makefile
> index 037fb15..bd82f2d 100644
> --- a/package/network/utils/iftop/Makefile
> +++ b/package/network/utils/iftop/Makefile
> @@ -8,12 +8,12 @@
>  include $(TOPDIR)/rules.mk
>  
>  PKG_NAME:=iftop
> -PKG_VERSION:=1.0pre2
> +PKG_VERSION:=1.0pre4
>  PKG_RELEASE:=1
>  
>  PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz
>  PKG_SOURCE_URL:=http://www.ex-parrot.com/~pdw/iftop/download
> -PKG_MD5SUM:=fef521a49ec0122458d02c64212af3c5
> +PKG_MD5SUM:=7e6decb4958e8a4890cccac335239f24
>  
>  PKG_MAINTAINER:=Jo-Philipp Wich <jow@openwrt.org>
>  PKG_LICENSE:=GPL-2.0
> diff --git a/package/network/utils/iftop/patches/0001-force-ncurses.patch b/package/network/utils/iftop/patches/0001-force-ncurses.patch
> deleted file mode 100644
> index bf23fb4..0000000
> --- a/package/network/utils/iftop/patches/0001-force-ncurses.patch
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -diff -ru iftop-1.0pre2-old/configure iftop-1.0pre2/configure
> ---- iftop-1.0pre2-old/configure	2011-10-04 13:30:30.000000000 -0700
> -+++ iftop-1.0pre2/configure	2012-09-09 22:26:05.000000000 -0700
> -@@ -7568,7 +7568,7 @@
> - { $as_echo "$as_me:$LINENO: checking for a curses library containing mvchgat" >&5
> - $as_echo_n "checking for a curses library containing mvchgat... " >&6; }
> - oldLIBS=$LIBS
> --for curseslib in ncursesw curses ncurses ; do
> -+for curseslib in ncurses ; do
> -     LIBS="$oldLIBS -l$curseslib"
> -     cat >conftest.$ac_ext <<_ACEOF
> - /* confdefs.h.  */
>
Russell Senior May 7, 2016, 9:41 a.m. UTC | #2
>>>>> "John" == John Crispin <john@phrozen.org> writes:

> On 05/05/2016 15:16, Bert Vermeulen wrote:
>> The patch made sure the ncursesw library was not selected to save
>> space, but that library doesn't exist in this distribution at all.

> are you sure that ncursesw is not inside any of the feeds either ?

Hey, I recognize that patch! ;-)

ncursesw does appear to still exist, e.g.:

  $ git grep ncursesw
  package/libs/ncurses/Makefile:define Package/libncursesw
  package/libs/ncurses/Makefile:  VARIANT:=libncursesw
  package/libs/ncurses/Makefile:ifeq ($(BUILD_VARIANT),libncursesw)
  package/libs/ncurses/Makefile:          --includedir="/usr/include/ncursesw" \
  package/libs/ncurses/Makefile:define Package/libncursesw/install
  package/libs/ncurses/Makefile:ifeq ($(BUILD_VARIANT),libncursesw)
  package/libs/ncurses/Makefile:  $(INSTALL_DIR) $(1)/usr/include/ncursesw/
  package/libs/ncurses/Makefile:  $(CP) $(PKG_INSTALL_DIR)/usr/include/ncursesw/*.h $(1)/usr/include/ncursesw/
  package/libs/ncurses/Makefile:  $(CP) $(PKG_INSTALL_DIR)/usr/bin/ncursesw5-config $(2)/bin/
  package/libs/ncurses/Makefile:          $(2)/bin/ncursesw5-config
  package/libs/ncurses/Makefile:  ln -sf $(STAGING_DIR)/host/bin/ncursesw5-config $(1)/usr/bin/ncursesw5-config
  package/libs/ncurses/Makefile:$(eval $(call BuildPackage,libncursesw))
  package/network/utils/iftop/patches/0001-force-ncurses.patch:-for curseslib in ncursesw curses ncurses ; do
  scripts/config/lxdialog/check-lxdialog.sh:      pkg-config --libs ncursesw 2>/dev/null && exit
  scripts/config/lxdialog/check-lxdialog.sh:              for lib in ncursesw ncurses curses ; do
  scripts/config/lxdialog/check-lxdialog.sh:      if pkg-config --cflags ncursesw 2>/dev/null; then
  scripts/config/lxdialog/check-lxdialog.sh:      elif [ -f /usr/include/ncursesw/curses.h ]; then
  scripts/config/lxdialog/check-lxdialog.sh:              echo '-I/usr/include/ncursesw -DCURSES_LOC="<curses.h>"'
Bert Vermeulen May 7, 2016, 9:43 a.m. UTC | #3
On 05/07/2016 07:17 AM, John Crispin wrote:
> 
> 
> On 05/05/2016 15:16, Bert Vermeulen wrote:
>> The patch made sure the ncursesw library was not selected to save space,
>> but that library doesn't exist in this distribution at all.
> 
> are you sure that ncursesw is not inside any of the feeds either ?

It's not clear to me what are considered "official" feeds, i.e. which
should be checked for dependencies and this sort of thing.

Going by the uncommented ones in feeds.conf.default, I find four
references to ncursesw: two patches to avoid compiling against it (i.e.
same as iftop), and two that weirdly _prefer_ it over ncurses. It seems
to me all of those can go.

Is there anything else that could pull in libncursesw?
Felix Fietkau May 7, 2016, 10:01 a.m. UTC | #4
On 2016-05-07 11:43, Bert Vermeulen wrote:
> On 05/07/2016 07:17 AM, John Crispin wrote:
>> 
>> 
>> On 05/05/2016 15:16, Bert Vermeulen wrote:
>>> The patch made sure the ncursesw library was not selected to save space,
>>> but that library doesn't exist in this distribution at all.
>> 
>> are you sure that ncursesw is not inside any of the feeds either ?
> 
> It's not clear to me what are considered "official" feeds, i.e. which
> should be checked for dependencies and this sort of thing.
> 
> Going by the uncommented ones in feeds.conf.default, I find four
> references to ncursesw: two patches to avoid compiling against it (i.e.
> same as iftop), and two that weirdly _prefer_ it over ncurses. It seems
> to me all of those can go.
> 
> Is there anything else that could pull in libncursesw?
Packages from feeds sometimes use libncursesw instead of libncurses. I
just did a size comparison:
110481 libncurses_5.9-3_mipsel_1004kc_dsp.ipk
125871 libncursesw_5.9-3_mipsel_1004kc_dsp.ipk

I think the difference in size is not big enough to justify having two
separate packages to choose from via dependencies.

We should probably just move everything to libncursesw and drop
libncurses entirely.

- Felix
Bert Vermeulen May 7, 2016, 10:40 a.m. UTC | #5
On 05/07/2016 12:01 PM, Felix Fietkau wrote:
> On 2016-05-07 11:43, Bert Vermeulen wrote:
>> Is there anything else that could pull in libncursesw?
> Packages from feeds sometimes use libncursesw instead of libncurses. I
> just did a size comparison:
> 110481 libncurses_5.9-3_mipsel_1004kc_dsp.ipk
> 125871 libncursesw_5.9-3_mipsel_1004kc_dsp.ipk
>
> I think the difference in size is not big enough to justify having two
> separate packages to choose from via dependencies.
>
> We should probably just move everything to libncursesw and drop
> libncurses entirely.

Actually I'd gotten this wrong: libncursesw is an extra build option to 
libncurses, so you only get it if you explicitly want it. In that case 
though, you specifically DO want your applications to use ncursesw instead 
of regular ncurses.

So regardless of whether you keep libncurses around or not, I think patches 
preferring one or the other should be dropped, right?
Felix Fietkau May 7, 2016, 10:46 a.m. UTC | #6
On 2016-05-07 12:40, Bert Vermeulen wrote:
> On 05/07/2016 12:01 PM, Felix Fietkau wrote:
>> On 2016-05-07 11:43, Bert Vermeulen wrote:
>>> Is there anything else that could pull in libncursesw?
>> Packages from feeds sometimes use libncursesw instead of libncurses. I
>> just did a size comparison:
>> 110481 libncurses_5.9-3_mipsel_1004kc_dsp.ipk
>> 125871 libncursesw_5.9-3_mipsel_1004kc_dsp.ipk
>>
>> I think the difference in size is not big enough to justify having two
>> separate packages to choose from via dependencies.
>>
>> We should probably just move everything to libncursesw and drop
>> libncurses entirely.
> 
> Actually I'd gotten this wrong: libncursesw is an extra build option to 
> libncurses, so you only get it if you explicitly want it. In that case 
> though, you specifically DO want your applications to use ncursesw instead 
> of regular ncurses.
It's a build variant, meaning both can be built and staged at the same time.

> So regardless of whether you keep libncurses around or not, I think patches 
> preferring one or the other should be dropped, right?
What the package ends up using absolutely needs to match the
dependencies that the package specifies (otherwise you get a build
error). Allowing the package source build system to simply pick one will
result in very quirky behavior.

- Felix
Bert Vermeulen May 7, 2016, 11:03 a.m. UTC | #7
On 05/07/2016 12:46 PM, Felix Fietkau wrote:
> On 2016-05-07 12:40, Bert Vermeulen wrote:
>> On 05/07/2016 12:01 PM, Felix Fietkau wrote:
>>> We should probably just move everything to libncursesw and drop
>>> libncurses entirely.
>>
>> Actually I'd gotten this wrong: libncursesw is an extra build option to
>> libncurses, so you only get it if you explicitly want it. In that case
>> though, you specifically DO want your applications to use ncursesw instead
>> of regular ncurses.
> It's a build variant, meaning both can be built and staged at the same time.
>
>> So regardless of whether you keep libncurses around or not, I think patches
>> preferring one or the other should be dropped, right?
> What the package ends up using absolutely needs to match the
> dependencies that the package specifies (otherwise you get a build
> error). Allowing the package source build system to simply pick one will
> result in very quirky behavior.

Ok, so will you drop regular libncurses? Or do you need me to submit a patch 
to do it?
Russell Senior May 7, 2016, 6:11 p.m. UTC | #8
>>>>> "Bert" == Bert Vermeulen <bert@biot.com> writes:

Bert> On 05/07/2016 12:46 PM, Felix Fietkau wrote:
>> On 2016-05-07 12:40, Bert Vermeulen wrote:
>>> On 05/07/2016 12:01 PM, Felix Fietkau wrote:
>>>> We should probably just move everything to libncursesw and drop
>>>> libncurses entirely.
>>> 
>>> Actually I'd gotten this wrong: libncursesw is an extra build option
>>> to libncurses, so you only get it if you explicitly want it. In that
>>> case though, you specifically DO want your applications to use
>>> ncursesw instead of regular ncurses.
>> It's a build variant, meaning both can be built and staged at the
>> same time.
>> 
>>> So regardless of whether you keep libncurses around or not, I think
>>> patches preferring one or the other should be dropped, right?
>> What the package ends up using absolutely needs to match the
>> dependencies that the package specifies (otherwise you get a build
>> error). Allowing the package source build system to simply pick one
>> will result in very quirky behavior.

Bert> Ok, so will you drop regular libncurses? Or do you need me to
Bert> submit a patch to do it?

In this particular case, as the author of the patch involved, I would
suggest "if it isn't broken, don't fix it".  That is, iftop doesn't
really need ncursesw, the patch lets it use plain ncurses even if
ncursesw is available.  I don't see the problem leaving things like
that.  It means carrying a small patch.  Seems like not that big a
problem.
Bert Vermeulen May 8, 2016, 8:57 a.m. UTC | #9
On 05/07/2016 08:11 PM, Russell Senior wrote:
> In this particular case, as the author of the patch involved, I would
> suggest "if it isn't broken, don't fix it".  That is, iftop doesn't
> really need ncursesw, the patch lets it use plain ncurses even if
> ncursesw is available.  I don't see the problem leaving things like
> that.  It means carrying a small patch.  Seems like not that big a
> problem.

Carrying patches is of course a problem: as I showed with numbers, a
problem that won't solve itself and will get worse over time.

Upstreaming your patch must clearly be the first choice. Did you even
try to upstream an ncurses preference in iftop? It's been in the OpenWrt
repo for 3.5 YEARS!
diff mbox

Patch

diff --git a/package/network/utils/iftop/Makefile b/package/network/utils/iftop/Makefile
index 037fb15..bd82f2d 100644
--- a/package/network/utils/iftop/Makefile
+++ b/package/network/utils/iftop/Makefile
@@ -8,12 +8,12 @@ 
 include $(TOPDIR)/rules.mk
 
 PKG_NAME:=iftop
-PKG_VERSION:=1.0pre2
+PKG_VERSION:=1.0pre4
 PKG_RELEASE:=1
 
 PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz
 PKG_SOURCE_URL:=http://www.ex-parrot.com/~pdw/iftop/download
-PKG_MD5SUM:=fef521a49ec0122458d02c64212af3c5
+PKG_MD5SUM:=7e6decb4958e8a4890cccac335239f24
 
 PKG_MAINTAINER:=Jo-Philipp Wich <jow@openwrt.org>
 PKG_LICENSE:=GPL-2.0
diff --git a/package/network/utils/iftop/patches/0001-force-ncurses.patch b/package/network/utils/iftop/patches/0001-force-ncurses.patch
deleted file mode 100644
index bf23fb4..0000000
--- a/package/network/utils/iftop/patches/0001-force-ncurses.patch
+++ /dev/null
@@ -1,12 +0,0 @@ 
-diff -ru iftop-1.0pre2-old/configure iftop-1.0pre2/configure
---- iftop-1.0pre2-old/configure	2011-10-04 13:30:30.000000000 -0700
-+++ iftop-1.0pre2/configure	2012-09-09 22:26:05.000000000 -0700
-@@ -7568,7 +7568,7 @@
- { $as_echo "$as_me:$LINENO: checking for a curses library containing mvchgat" >&5
- $as_echo_n "checking for a curses library containing mvchgat... " >&6; }
- oldLIBS=$LIBS
--for curseslib in ncursesw curses ncurses ; do
-+for curseslib in ncurses ; do
-     LIBS="$oldLIBS -l$curseslib"
-     cat >conftest.$ac_ext <<_ACEOF
- /* confdefs.h.  */