Bug 249352 - Review Request: popt - C library for parsing command line parameters
Review Request: popt - C library for parsing command line parameters
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Till Maas
Fedora Extras Quality Assurance
:
: 203538 (view as bug list)
Depends On:
Blocks: 102254 135428 178413 202061 205849 249814
  Show dependency treegraph
 
Reported: 2007-07-23 17:08 EDT by Robert Scheck
Modified: 2007-11-30 17:12 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-23 19:22:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
opensource: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Suggested spec tweaks (1.75 KB, patch)
2007-08-07 09:03 EDT, Panu Matilainen
no flags Details | Diff

  None (edit)
Description Robert Scheck 2007-07-23 17:08:14 EDT
Spec URL: http://labs.linuxnetz.de/bugzilla/popt.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/popt-1.12-1.src.rpm
Description: Popt is a C library for parsing command line parameters. Popt
was heavily influenced by the getopt() and getopt_long() functions, but it
improves on them by allowing more powerful argument expansion. Popt can parse
arbitrary argv[] style arrays and automatically set variables based on command
line arguments. Popt allows command line arguments to be aliased via
configuration files and includes utility functions for parsing arbitrary
strings into argv[] arrays using shell-like rules.
Comment 1 Bill Nottingham 2007-07-23 21:44:39 EDT
I'm assuming it's intended to split this out of rpm?
Comment 2 Robert Scheck 2007-07-24 01:42:35 EDT
Yes, same like mutt <-> urlview or gmp <-> mpfr or similar.
Comment 3 Panu Matilainen 2007-07-24 02:15:55 EDT
Bill: Yes, the intent is to split it out, having a separate library with it's
own versioning coming out of rpm package is a huge pita and I want to see this
done in time for F8.

But this needs to be carefully coordinated to avoid making a total mess as rpm
is quite intimately married to popt, hold it for a moment, need to discuss this
with Paul first. 
Comment 4 Robert Scheck 2007-07-24 03:21:56 EDT
Panu: Well, splitting popt out of rpm is not that huge problem as it currently 
looks like to you. Finally popt ever had it's own versioning (just read the 
popt changelog inside of the popt directory).
Comment 5 Panu Matilainen 2007-07-30 04:12:54 EDT
Didn't say it was a huge problem, just that it needs to be coordinated.
Comment 6 Patrice Dumas 2007-07-31 12:00:25 EDT
*** Bug 203538 has been marked as a duplicate of this bug. ***
Comment 7 Patrice Dumas 2007-07-31 12:03:07 EDT
Also popt-devel will have to be added to a lot of BR, unless
my rough analysis in Bug 203538 is incorrect.
Comment 8 Robert Scheck 2007-07-31 13:53:26 EDT
Correct, initscripts and mkinitrd (nash) would be two important ones except 
rpm. IIRC static linking is required for initscripts, maybe I'm wrong, just 
what my brain thinks to know ;)
Comment 9 Panu Matilainen 2007-08-01 01:30:22 EDT
There are a LOT of packages depending on popt actually:
[pmatilai@localhost ~]$ repoquery --quiet --whatrequires --alldeps --queryformat
"%{sourcerpm}" popt|sort -u|wc -l
297

A fair amount of duplicate versions in that number but it's > 200 packages
anyway. They haven't needed the explicit BuildRequires on popt because main popt
package gets pulled in by rpm always.
Comment 10 Patrice Dumas 2007-08-01 06:17:06 EDT
The transition path could be eased by having rpm provide 
explicitly popt-devel, tell on the list that all packages that use
popt should BuildRequires popt-devel, wait a bit that most important
packages have been rebuilt then split out popt-devel (either in rpm
on as part of popt).
Comment 11 Till Maas 2007-08-02 09:40:40 EDT
This spec installs libpopt to /usr/lib(64) instead of /lib(64), see #249814 
Comment 12 Panu Matilainen 2007-08-07 09:03:59 EDT
Created attachment 160808 [details]
Suggested spec tweaks

I'd suggest the attached patch to the spec:
1) move the library to /lib[64] as per bug #249814
2) generate and include api documentation

Whether a separate subpackage is needed for 2) is another question, it could of
course be stuffed into -devel just as well.
Comment 13 Ralf Corsepius 2007-08-07 09:36:33 EDT
(In reply to comment #12)
> Created an attachment (id=160808) [edit]
> Suggested spec tweaks
> 
> I'd suggest the attached patch to the spec:
> 1) move the library to /lib[64] as per bug #249814
I am opposed to this change, because this would put devel-libs into /lib.

Instead I'd suggest that you apply the same approach as glib2 does: 
Put the devel libs into /usr/lib, put the run-time-libs into /lib
and add a couple of symlinks inbetween.
Comment 14 Panu Matilainen 2007-08-07 09:50:20 EDT
Right, stuffing devel libs into /lib was indeed a stupid oversight on my behalf :)
Comment 15 Robert Scheck 2007-08-10 06:12:37 EDT
Ralf, thanks for this hint. Panu, thanks for your inital patch, I'll merge 
today your patch with Ralfs last comment into a -2 and push to the same base 
URL as my initial review request. When done, URLs will get added.
Comment 16 Panu Matilainen 2007-08-10 06:28:53 EDT
Yup. Once the /lib move has been done I think it's ok to proceed with the review.

I've done a bit of testing with rpm 4.4.2.1 and didn't see any problems with the
newer popt. Some of points worth noting:
- rpm needs to loose the strict dependency on popt = <version>-<release> (will do)
- fedora-maintainers + fedora-devel-announce needs to be notified because the
amount of implicit build dependencies on popt-devel
- once this package hits rawhide, rpm needs to be modified to build against
external popt and kill of the internal version entirely (will do)
- I want co-maintainership for the new popt package, just so I know what's going
on with it due to the close marriage with rpm (not that I expect much to happen,
popt isn't that actively developed)
Comment 17 Till Maas 2007-08-10 06:36:30 EDT
The License Tag does not math current Guidelines:
http://fedoraproject.org/wiki/Packaging/LicensingGuidelines - it needs to have a
value that is mentioned in http://fedoraproject.org/wiki/Licensing

It seems to be a MIT License "Modern Style with sublicense":
http://fedoraproject.org/wiki/Licensing/MIT

So the License-tag should be "MIT" instead of "X Consortium".
But I do not know, whether or not it is a problem that, test2.c is (L)GPL
licensed according to its header, but I guess it is not distributed in the
binary rpm. Also  some files are missing a license header (test3.c, popint.c and
system.h)
Comment 18 Robert Scheck 2007-08-11 14:21:00 EDT
Panu, does rpm.org's rpm depend on the libpopt.la file? Can you please verify, 
whether rebuilding without works or shall we simply keep this file? IIRC rpm is 
the only package which at least was depending on this file.
Comment 19 Ralf Corsepius 2007-08-11 23:27:18 EDT
(In reply to comment #18)
> Panu, does rpm.org's rpm depend on the libpopt.la file?
No, rpm.org's hg mainline doesn't

> Can you please verify, 
> whether rebuilding without works or shall we simply keep this file?
At least, rpm.org'd hg mainline doesn't.

Comment 20 Panu Matilainen 2007-08-12 08:10:01 EDT
No need to package the .la file, even 4.4.2.1 appears to builds ok without it.
Comment 21 Panu Matilainen 2007-08-12 10:08:18 EDT
Oh and btw this part is now done in latest rawhide rpm to remove silly testing
roadblocks:
> - rpm needs to loose the strict dependency on popt = <version>-<release> 
Comment 22 Robert Scheck 2007-08-12 15:01:08 EDT
The /lib move has been done, other suggested changes have been merged in. Can 
we walk through the official review now? Updated package below:

Spec URL: http://labs.linuxnetz.de/bugzilla/popt.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/popt-1.12-2.src.rpm
Comment 23 Till Maas 2007-08-22 06:08:16 EDT
It seems that the bugs in "depends on" were intended for "blocks", because one
is for RHEL and may be fixed in this version and the other is more a feature
request and may also be already fixed in the reviewed version. Please change
this back in case I am wrong.
Comment 24 Till Maas 2007-08-22 07:42:07 EDT
graphviz (or something else that provides dot) is missing in the BuildRequires,
without it errors like this are shown:

sh: dot: command not found
Problems running dot: exit code=127, command='dot',
Arguments='"poptint_8h__dep__incl.dot" -Tpng -o "poptint_8h__dep__incl.png"'

Review:

- rpmlint: ok (this is with BR on graphviz)
E: popt-devel zero-length /usr/share/doc/popt-devel-1.12/html/popt_8h__incl.map
This is created by doxygen, imho there is no need to remove it manually.
W: popt-static no-documentation
This is no problem, too.

- naming: ok
- spec file readability: ok

- source: ok
f45290e9ac4b1cf209d0042eb6755543  /tmp/popt-1.12.tar.gz
f45290e9ac4b1cf209d0042eb6755543  popt-1.12.tar.gz

- mockbuild i386 F7: ok
- BuildRequires: NEEDSWORK (add graphviz)
- ldconfig scriptlets: ok
- Directory Ownership: ok
- %clean section: ok
- macro usage: ok
- doc: large api doc is in devel, ok
- devel subpackage with headers: ok
- static subpackage: ok
- .so file in -devel: ok
- subpackage requires: ok
- .la removed: ok
- correct %install
- license included: ok
- Buildroot: ok


Add graphviz to BR before you import the package and it is APPROVED
Comment 25 Robert Scheck 2007-08-22 10:49:37 EDT
New Package CVS Request
=======================
Package Name: popt
Short Description: C library for parsing command line parameters
Owners: redhat-bugzilla@linuxnetz.de,pmatilai@redhat.com

For the guy doing the CVS: /me is maintainer, Panu is co-maintainer. Only 
development branch (upcoming F8), please.
Comment 26 Warren Togami 2007-08-22 12:57:28 EDT
Could you please provide this in the new template format?
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

Specifically, we need the FAS account names.
Comment 27 Robert Scheck 2007-08-22 14:19:58 EDT
Okay Warren, here we go again:

New Package CVS Request
=======================
Package Name: popt
Short Description: C library for parsing command line parameters
Owners: robert,pmatilai
Branches: 
InitialCC: 
Cvsextras Commits: no
Comment 28 Kevin Fenzi 2007-08-23 16:28:38 EDT
cvs done. 
Comment 29 Robert Scheck 2007-08-23 19:22:39 EDT
popt 1.12-3 was sucessfully built for Rawhide in Koji.

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