Bug 226188 - Merge Review: ncurses
Merge Review: ncurses
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 15:15 EST by Nobody's working on this, feel free to take it
Modified: 2008-01-03 13:53 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-02-26 07:03:46 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:15:20 EST
Fedora Merge Review: ncurses

http://cvs.fedora.redhat.com/viewcvs/devel/ncurses/
Initial Owner: mlichvar@redhat.com
Comment 1 Jason Tibbitts 2007-02-04 13:34:40 EST
I fear this will be mildly difficult, as the ncurses build process seems mildly complicated and I can't really tell if it all actually needs to be that way, so I'll need help from you to comprehend what's going on.

First, let's look at rpmlint output:

E: ncurses tag-not-utf8 %changelog
E: ncurses-debuginfo tag-not-utf8 %changelog
E: ncurses-devel tag-not-utf8 %changelog
E: ncurses tag-not-utf8 %changelog
E: ncurses non-utf8-spec-file ncurses.spec
   These are due to bero's name in the changelog and should go away with a bit
   of editing or a pass through iconv.

W: ncurses invalid-license distributable
W: ncurses-debuginfo invalid-license distributable
W: ncurses-devel invalid-license distributable
W: ncurses invalid-license distributable
   The license is actually BSD, and the license tag should be changed to
   match.

E: ncurses-devel obsolete-not-provided ncurses-c++-devel
W: ncurses unversioned-explicit-obsoletes ncurses-c++-devel
   These should just go away; the last time that package was shipped was RH9.

E: ncurses-devel script-without-shebang /usr/share/doc/ncurses-devel-5.6/test/savescreen.c
E: ncurses-devel script-without-shebang /usr/share/doc/ncurses-devel-5.6/test/demo_panels.c
W: ncurses-devel doc-file-dependency /usr/share/doc/ncurses-devel-5.6/test/tracemunch perl(strict)
W: ncurses-devel doc-file-dependency /usr/share/doc/ncurses-devel-5.6/test/tracemunch /usr/bin/perl
   None of these should be executable.  I suppose there's plenty of value in
   packaging these because it's not really possible to run the tests at build
   time, but the bottom line is that documentation should not be executable.

E: ncurses hardcoded-library-path in /lib
   This is specifying the location of the terminfo directory; my understanding
   is that it needs to be in /lib regardless of the architecture.

W: ncurses rpm-buildroot-usage %build --with-install-prefix=$RPM_BUILD_ROOT \\\
   This I am not conmpletely sure of.  The intent is to make sure you don't
   mess with the buildroot in a way that breaks short-circuiting (because
   nothing should go into the buildroot until %install).  But this is just
   defining something that sets up the install location and so it should be
   OK, but I'd like to know before approving this that the usual
   "make DESTDIR=... install" or even %makeinstall doesn't work for this
   package.

Those are the only issues of note, and should be pretty easy to fix up.

Review:
* source files match upstream:
   f9cac2b31683a37d65bc37119599752198a0691e462d0d1a252cf9815f5724d5
   ncurses-5.6.tar.gz
* package meets naming and versioning guidelines:
   Looks like a post-release snapshot and is named according to upstream's
   policy.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
X license field says "distributable" but is really BSD.
* license is open source-compatible.  License text included in package.
O latest version is not being packaged:
    I think a new rollup came out yesterday morning, but it's certainly not a
    blocker.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* %makeinstall is not used.
* package builds in mock.
* debuginfo package looks complete.
X rpmlint is silent.
X final provides and requires are sane:
  There are a couple of errant Perl requirements in the -devel package that
  come from executable documentation.
  ncurses-5.6-2.20070120.fc7.i386.rpm:
   libform.so.5
   libformw.so.5
   libmenu.so.5
   libmenuw.so.5
   libncurses.so.5
   libncursesw.so.5
   libpanel.so.5
   libpanelw.so.5
   libtic.so.5
   ncurses = 5.6-2.20070120.fc7
  =
   /sbin/ldconfig
   libform.so.5
   libformw.so.5
   libmenu.so.5
   libmenuw.so.5
   libncurses.so.5
   libncursesw.so.5
   libpanel.so.5
   libpanelw.so.5
   libtic.so.5

  ncurses-devel-5.6-2.20070120.fc7.i386.rpm:
   ncurses-devel = 5.6-2.20070120.fc7
  =
   /bin/sh
X  /usr/bin/perl
   libform.so.5
   libformw.so.5
   libmenu.so.5
   libmenuw.so.5
   libncurses.so.5
   libncursesw.so.5
   libpanel.so.5
   libpanelw.so.5
   libtic.so.5
   ncurses = 5.6-2.20070120.fc7
X  perl(strict)

* %check is not present; there's a test suite upstream, but it's interactive
  and so not runnable at build time.
* shared libraries are present and ldconfig is called properly.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
X file permissions are appropriate (executable documentation)
* scriptlets are OK (ldconfig)
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* no pkgconfig files.
   There are old-style *-config files in /usr/bin, and they are packaged
   properly.
* no libtool .la droppings.
Comment 2 Miroslav Lichvar 2007-02-05 11:36:19 EST
Thanks for the review.

> W: ncurses invalid-license distributable
>    The license is actually BSD, and the license tag should be changed to
>    match.

Or even better MIT license, is that correct?

> W: ncurses rpm-buildroot-usage %build --with-install-prefix=$RPM_BUILD_ROOT \\\
>    This I am not conmpletely sure of.  The intent is to make sure you don't
>    mess with the buildroot in a way that breaks short-circuiting (because
>    nothing should go into the buildroot until %install).  But this is just
>    defining something that sets up the install location and so it should be
>    OK, but I'd like to know before approving this that the usual
>    "make DESTDIR=... install" or even %makeinstall doesn't work for this
>    package.

The configure option just sets DESTDIR value in Makefiles, it is the same as
installing with DESTDIR=..., but less typing. Should I remove it?
Comment 3 Miroslav Lichvar 2007-02-06 07:11:59 EST
Should be fixed in ncurses-5.6-3.20070203.fc7.
Comment 4 Robert Scheck 2007-02-15 14:30:49 EST
Fixing of bug #228891 is IMHO a _MUST_, too. Jason...correct me please, if 
you've got another opinion regarding this bug report.
Comment 5 Jason Tibbitts 2007-02-15 14:37:08 EST
Well, perhaps; the package as I have it available to me doesn't include that
file (/usr/lib/libncurses.so.5) and it is not explicitly created by the package,
so where is it coming from?  Is ldconfig creating it at install time?

libncurses.so.5 is in /lib as far as I can tell.
Comment 6 Robert Scheck 2007-02-15 14:48:34 EST
It looks like the file is created by ldconfig at install time. However, we 
should get rid of the problem and I know, that there is /lib/libncurses.so.5.
Comment 7 Jason Tibbitts 2007-02-15 15:26:02 EST
A simple %ghost /usr/lib/libncurses.so.5 would probably do it, but it would be
best to understand exactly why ldconfig is creating that symlink.  I have no
rawhide machines around at the moment so I can't really experiment.
Comment 8 Jason Tibbitts 2007-02-19 16:19:35 EST
OK, seems this has been fixed by using a linker script instead of a symlink.

Looks like a -static subpackage popped up since I first looked at this; I think
this is one of those packages that warrants the availability of static libs so I
have no complaints.

I've done a fresh build on my buildsys, which gives me more information than
what was available to us at fudcon.  Here's the current set of rpmlint complaints:

W: ncurses unused-direct-shlib-dependency /usr/lib64/libmenuw.so.5.6
/lib64/libdl.so.2
W: ncurses unused-direct-shlib-dependency /usr/lib64/libpanel.so.5.6
/lib64/libdl.so.2
W: ncurses unused-direct-shlib-dependency /usr/lib64/libform.so.5.6
/lib64/libdl.so.2
W: ncurses unused-direct-shlib-dependency /usr/lib64/libformw.so.5.6
/lib64/libdl.so.2
W: ncurses unused-direct-shlib-dependency /usr/lib64/libpanelw.so.5.6
/lib64/libdl.so.2
W: ncurses unused-direct-shlib-dependency /usr/lib64/libmenu.so.5.6
/lib64/libdl.so.2
   These will only show up if you run rpmlint on the installed package.  I think
they show up because the configure script sticks "-ldl" on the link line for
whatever reason.

E: ncurses-static devel-dependency ncurses-devel
W: ncurses-static no-documentation
W: ncurses-static devel-file-in-non-devel-package /usr/lib64/libncursesw.a
W: ncurses-static devel-file-in-non-devel-package /usr/lib64/libmenuw_g.a
   [and several more identical warnings]
   These are all OK.

I have to run now; I'll finish this a bit later.
Comment 9 Jason Tibbitts 2007-02-19 21:41:08 EST
About the license, the FSF recommends against using "MIT" because that
institution has used many licenses.  (The Expat license is also referred to as
"MIT", but it's not the same thing.)  It's better to use "X11" in that case.

The executable documentation is gone now, and the errant dependencies are gone
as well.

Either way of setting DESTDIR seems fine to me, but you've already changed it to
the more conventional method so that's fine.

So really I'd say to just set the License: to X11 and this package is done. 
There's no need to hold things up for that, so I'll ack this now.  But it would
be good to look into the source of those unused-direct-shlib-dependency warnings
at some point.

APPROVED
Comment 10 Miroslav Lichvar 2007-02-20 11:05:18 EST
Thanks for the review.

But I will stay with MIT license for now. There is only one package in Extras
that has License: X11, and many have MIT. If you think this should be changed,
please bring it up to the fedora list.
Comment 11 Jason Tibbitts 2007-02-20 11:17:34 EST
MIT isn't so much incorrect as it is ambiguous, but I can't fault you with
wanting to stick with what other packages are using.  We'll clear this up once
and for all when we get down to the big license cleanup, perhaps in a few months.
Comment 12 Jason Tibbitts 2007-02-22 13:27:24 EST
If the approved package has been built for rawhide, you can go ahead and close
this bug.
Comment 13 Patrice Dumas 2008-01-03 04:58:51 EST
I am looking at the gpm and ncurse cross dependencies, and 
it looks like something needs a bit of explanation. I don't 
understand exactly what is happening, but it looks like there
are cross-dependencies in the 2 libraries. The issue is mitigated
by some workarounds, but overall it looks quite wrong to me.
It looks like ncurses dlopens gpm looking at the soname found
at build time... which is not right.

Moreover this leads to a loop build dependency between the 2 
packages.
Comment 14 Miroslav Lichvar 2008-01-03 08:31:29 EST
Hm, ncurses don't require gpm, libgpm will be used only when installed. What
exactly is wrong with dlopening libgpm?
Comment 15 Patrice Dumas 2008-01-03 09:57:14 EST
It seems to me that it is better to link against other libraries,
not dlopen them. Otherwise the applications behaviour will be
unreproductible, and without gpm installed functionalities will be 
missing. Also a gpm soname bump may be missed, with an optional 
dependency.
Comment 16 Miroslav Lichvar 2008-01-03 11:30:39 EST
Well, I think the advantages of dlopening the library outweigh the disadvantages.

Most of the users probably don't use gpm in ncurses applications at all, ncurses
is a pretty low-level library where I think keeping minimal dependencies is
preferred (libgpm would need to be split from gpm package first to avoid
circular dependency). Also, the libgpm soname is not likely to change anytime soon.
Comment 17 Patrice Dumas 2008-01-03 13:53:50 EST
gpm is also low level, and I don't think that adding gpm 
runtime dependency will lead to additional runtime dependencies.
Also there shouldn't be any circular dependency since (unless
I am missing something) the ncurses symbols are declared weak in
gpm, and therefore gpm doesn't depend on ncurses.

There is already a circular build dependency since gpm-devel 
BuildRequires ncurses-devel and ncurses BuildRequires gpm-devel.

I don't have any idea how this could be avoided, though.
Indeed gpm requires the ncurses headers, and ncurses also
requires gpm headers (and libs currently to get soname, though
in my opinion it should be linked against gpm...). 

Note You need to log in before you can comment on or make changes to this bug.