Message ID | 1462454180-27035-1-git-send-email-bert@biot.com |
---|---|
State | Changes Requested |
Headers | show |
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. */ >
>>>>> "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>"'
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?
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
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?
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
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?
>>>>> "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.
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 --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. */
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