Bug 211850 - devel/tig: Update to 2.2
Summary: devel/tig: Update to 2.2
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Dmitry Marakasov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-14 23:37 UTC by lightside
Modified: 2016-10-05 19:57 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (darcsis)


Attachments
Proposed patch (since 412346 revision) (1.40 KB, patch)
2016-08-14 23:37 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) (1.66 KB, patch)
2016-08-18 14:12 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) with bash_cv_termcap_lib=libncursesw (2.14 KB, patch)
2016-08-19 00:58 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) with bash_cv_termcap_lib=libc (2.13 KB, patch)
2016-08-19 01:00 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) with LIBS=-lncursesw (2.09 KB, patch)
2016-08-19 01:02 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) with bash_cv_termcap_lib=libc (2.37 KB, patch)
2016-08-19 11:47 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) with readline stub (2.97 KB, patch)
2016-08-19 11:50 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) with reordering of termcap's library checks (3.29 KB, patch)
2016-08-19 12:46 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) with reordering of termcap's library checks (3.29 KB, patch)
2016-08-19 12:58 UTC, lightside
no flags Details | Diff
Proposed patch (since 412346 revision) with reordering of termcap's library checks (3.29 KB, patch)
2016-08-19 17:10 UTC, lightside
lightside: maintainer-approval? (darcsis)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lightside 2016-08-14 23:37:27 UTC
Created attachment 173684 [details]
Proposed patch (since 412346 revision)

Patch to update devel/tig port from 2.1.1 to 2.2 version.

Look following links for changes:
http://jonas.nitro.dk/tig/NEWS.html#_tig_2_2
https://github.com/jonas/tig/releases/tag/tig-2.2
https://github.com/jonas/tig/compare/tig-2.1.1...tig-2.2

Tested with using poudriere on FreeBSD 9.3 amd64 and native build on FreeBSD 10.2 amd64.
Comment 1 Dmitry Marakasov freebsd_committer 2016-08-18 09:34:30 UTC
Looks good, awaiting maintainer's feedback
Comment 2 Dmitry Marakasov freebsd_committer 2016-08-18 10:01:30 UTC
While here, is the patch under files/ really needed any longer? Seems like it can be upstreamed or removed. Also I'd suggest to add MAKE_ARGS=     V=1 to verbosify build. Otherwise looks good and builds fine, awaiting maintainer's feedback.
Comment 3 lightside 2016-08-18 14:12:09 UTC
Created attachment 173823 [details]
Proposed patch (since 412346 revision)

(In reply to comment #2)
> While here, is the patch under files/ really needed any longer?
The files/patch-config.make.in applies still. It was added in ports r353967. The purpose of it was explained in bug 189539 comment 0:
> - Fix display wide character problem that cause by readline, Change LDLIBS
> order to resolve.
I reproduced this. For example, try to build devel/tig without mentioned patch. Run tig for some repository and press "SHIFT + :", then try to type UTF-8 (non-english) character(s). It will display correct character(s) with patch.

(In reply to comment #2)
> Also I'd suggest to add MAKE_ARGS=     V=1 to verbosify build.
Ok, added.
Comment 4 Dmitry Marakasov freebsd_committer 2016-08-18 14:42:46 UTC
(In reply to lightside from comment #3)

> > While here, is the patch under files/ really needed any longer?
> The files/patch-config.make.in applies still. It was added in ports r353967.
> The purpose of it was explained in bug 189539 comment 0:
> > - Fix display wide character problem that cause by readline, Change LDLIBS
> > order to resolve.
> I reproduced this. For example, try to build devel/tig without mentioned
> patch. Run tig for some repository and press "SHIFT + :", then try to type
> UTF-8 (non-english) character(s). It will display correct character(s) with
> patch.

OK then, leaving the patch there. I'd prefer this investigated further though and, if possible, fixed/upstreamed. This, for instance, smells fishy:

/usr/local/bin/tig:
        libncursesw.so.8 => /lib/libncursesw.so.8 (0x80085d000)
        libreadline.so.6 => /usr/local/lib/libreadline.so.6 (0x800ab9000)
        libncurses.so.8 => /lib/libncurses.so.8 (0x800d05000)
        libc.so.7 => /lib/libc.so.7 (0x800f57000)

libncurses comes from libreadline, but I'm not sure why symbols from it are picked up, as we have disabled recursive linking.
Comment 5 lightside 2016-08-19 00:58:35 UTC
Created attachment 173839 [details]
Proposed patch (since 412346 revision) with bash_cv_termcap_lib=libncursesw

(In reply to comment #2)
> OK then, leaving the patch there.
The proposed method in ports r353967 is similar to https://github.com/jonas/tig/blob/tig-2.2/contrib/config.make, which used as example in https://github.com/jonas/tig/blob/tig-2.2/INSTALL.adoc#build-configuration, where "-lncursesw" added before "-lreadline" for LDLIBS variable.

(In reply to comment #4)
> libncurses comes from libreadline, but I'm not sure why symbols from it are
> picked up, as we have disabled recursive linking.
This issue somehow related to termcap library, which linked, but not used, according to ldd output.
The tools/ax_lib_readline.m4 script, which included in generated configure file, selects bash_cv_termcap_lib=libtermcap first:
https://github.com/jonas/tig/blob/tig-2.2/tools/ax_lib_readline.m4#L36
% readelf -Ds /usr/lib/libtermcap.so | grep tgetent
  214 331: 0000000000015590  1415    FUNC GLOBAL DEFAULT  11 tgetent

If explicitly define bash_cv_termcap_lib variable to libncursesw value:
% readelf -Ds /usr/lib/libncursesw.so | grep tgetent
  377 331: 00000000000177b0  1415    FUNC GLOBAL DEFAULT  11 tgetent

then this issue may be fixed:
% make build && ldd -a work/tig-2.2/src/tig | sed -e '1d ; s| (.*)$||'
	libreadline.so.6 => /usr/local/lib/libreadline.so.6
	libncursesw.so.8 => /lib/libncursesw.so.8
	libc.so.7 => /lib/libc.so.7
/usr/local/lib/libreadline.so.6:
	libncurses.so.8 => /lib/libncurses.so.8
	libc.so.7 => /lib/libc.so.7
/lib/libncursesw.so.8:
	libc.so.7 => /lib/libc.so.7
/lib/libncurses.so.8:
	libc.so.7 => /lib/libc.so.7
Comment 6 lightside 2016-08-19 01:00:18 UTC
Created attachment 173840 [details]
Proposed patch (since 412346 revision) with bash_cv_termcap_lib=libc

The same effect (as in comment #5) with bash_cv_termcap_lib=libc, which removes second "-lncursesw" link on FreeBSD, but libc.so didn't have tgetent function on first check.
Comment 7 lightside 2016-08-19 01:02:10 UTC
Created attachment 173841 [details]
Proposed patch (since 412346 revision) with LIBS=-lncursesw

Also possible to use LIBS=-lncursesw.
I attached many variants to check.
Comment 8 lightside 2016-08-19 01:28:06 UTC
(In reply to comment #5)
> % make build && ldd -a work/tig-2.2/src/tig | sed -e '1d ; s| (.*)$||'
Output for attachment #173840 [details] in comment #6:
	libreadline.so.6 => /usr/local/lib/libreadline.so.6
	libncursesw.so.8 => /lib/libncursesw.so.8
	libc.so.7 => /lib/libc.so.7
/usr/local/lib/libreadline.so.6:
	libncurses.so.8 => /lib/libncurses.so.8
	libc.so.7 => /lib/libc.so.7
/lib/libncursesw.so.8:
	libc.so.7 => /lib/libc.so.7
/lib/libncurses.so.8:
	libc.so.7 => /lib/libc.so.7

Output for attachment #173841 [details] in comment #7:
	libncursesw.so.8 => /lib/libncursesw.so.8
	libreadline.so.6 => /usr/local/lib/libreadline.so.6
	libc.so.7 => /lib/libc.so.7
/lib/libncursesw.so.8:
	libc.so.7 => /lib/libc.so.7
/usr/local/lib/libreadline.so.6:
	libncurses.so.8 => /lib/libncurses.so.8
	libc.so.7 => /lib/libc.so.7
/lib/libncurses.so.8:
	libc.so.7 => /lib/libc.so.7
Comment 9 lightside 2016-08-19 09:38:28 UTC
(In reply to comment #5)
> This issue somehow related to termcap library
This is true, because libtermcap.so is a link to libncurses.so on FreeBSD. For example, on FreeBSD 10.2:
% readlink /usr/lib/libtermcap.so
libncurses.so
% sha256 /usr/lib/libtermcap.so
SHA256 (/usr/lib/libtermcap.so) = 39fd11d3e0fab753e51848bacad0eef4b84567f01f737b0f2663bae71124b2ed
% sha256 /usr/lib/libncurses.so
SHA256 (/usr/lib/libncurses.so) = 39fd11d3e0fab753e51848bacad0eef4b84567f01f737b0f2663bae71124b2ed

(In reply to comment #5)
> which linked, but not used, according to ldd output.
this is why it was linked and used, but with different library name (libncurses.so.8).
Comment 10 lightside 2016-08-19 10:35:59 UTC
Comment on attachment 173840 [details]
Proposed patch (since 412346 revision) with bash_cv_termcap_lib=libc

There is bash_cv_termcap_lib=libc method, which does not add selected termcap library for linking:
https://github.com/jonas/tig/blob/tig-2.2/tools/ax_lib_readline.m4#L67

I set maintainer-approval request for it, currently. Other methods are almost equal on effect.
Comment 11 lightside 2016-08-19 11:47:42 UTC
Created attachment 173856 [details]
Proposed patch (since 412346 revision) with bash_cv_termcap_lib=libc

Looks like, the tgetent function is not available in libc.so or libncurses*.so libraries on DragonFlyBSD. It's available in tinfo library:
https://github.com/jonas/tig/blob/tig-2.2/tools/ax_lib_readline.m4#L37

% readelf -s /usr/local/lib/libtinfo.so | grep -w tgetent
   132: 0000000000014880    21 FUNC    GLOBAL DEFAULT   11 tgetent

This may require to restrict bash_cv_termcap_lib=libc method to FreeBSD only.
Comment 12 lightside 2016-08-19 11:50:14 UTC
Created attachment 173857 [details]
Proposed patch (since 412346 revision) with readline stub

Possible to add readline stub for tools/ax_lib_readline.m4 script. The configure file will be regenerated in this case.
Comment 13 lightside 2016-08-19 11:53:48 UTC
Comment on attachment 173857 [details]
Proposed patch (since 412346 revision) with readline stub

(In reply to comment #12)
> Possible to add readline stub for tools/ax_lib_readline.m4 script.
No, the implementation is wrong.
Comment 14 lightside 2016-08-19 12:46:33 UTC
Created attachment 173858 [details]
Proposed patch (since 412346 revision) with reordering of termcap's library checks

Possible to reorder termcap's library checks for tools/ax_lib_readline.m4 script. Trying to check *curses* libraries first, based on previous detection of $ax_cv_curses_which.

The used patch is possible candidate to propose for upstream.
Comment 15 lightside 2016-08-19 12:58:48 UTC
Created attachment 173860 [details]
Proposed patch (since 412346 revision) with reordering of termcap's library checks

Some fixes.
Comment 16 lightside 2016-08-19 13:27:22 UTC
(In reply to comment #14)
> The used patch is possible candidate to propose for upstream.
There is related (committed) pull request, which fixed similar issue for Mac OS X:
https://github.com/jonas/tig/pull/268
Comment 17 lightside 2016-08-19 17:10:00 UTC
Created attachment 173868 [details]
Proposed patch (since 412346 revision) with reordering of termcap's library checks
Comment 18 lightside 2016-08-26 09:18:28 UTC
Other attachments were obsoleted to not create a possible ambiguity.
Possible to return concrete attachment or propose other variant.
Comment 19 commit-hook freebsd_committer 2016-10-05 19:57:37 UTC
A commit references this bug:

Author: amdmi3
Date: Wed Oct  5 19:57:03 UTC 2016
New revision: 423383
URL: https://svnweb.freebsd.org/changeset/ports/423383

Log:
  - Update to 2.2

  PR:		211850
  Submitted by:	lightside@gmx.com
  Approved by:	maintainer timeout (darcsis@gmail.com, 6 weeks)

Changes:
  head/devel/tig/Makefile
  head/devel/tig/distinfo
  head/devel/tig/files/patch-config.make.in
  head/devel/tig/files/patch-src_ui.c
  head/devel/tig/files/patch-tools_ax__lib__readline.m4