Fedora Merge Review: ncurses http://cvs.fedora.redhat.com/viewcvs/devel/ncurses/ Initial Owner: mlichvar
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.
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?
Should be fixed in ncurses-5.6-3.20070203.fc7.
Fixing of bug #228891 is IMHO a _MUST_, too. Jason...correct me please, if you've got another opinion regarding this bug report.
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.
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.
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.
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.
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
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.
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.
If the approved package has been built for rawhide, you can go ahead and close this bug.
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.
Hm, ncurses don't require gpm, libgpm will be used only when installed. What exactly is wrong with dlopening libgpm?
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.
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.
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...).