Bug 241978
Summary: | Review Request: tig - Text-mode interface for git | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | James Bowes <jbowes> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | bkearney, jeff, steve.traylen |
Target Milestone: | --- | Flags: | j:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | tig-0.17-1.el6 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-06-02 16:29:55 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
James Bowes
2007-06-01 01:59:57 UTC
rpmlint output: $ rpmlint ~/rpmbuild/SRPMS/tig-0.7-1.fc7.src.rpm $ rpmlint ~/rpmbuild/RPMS/i386/tig-* W: tig spurious-executable-perm /usr/share/man/man1/tig.1.gz W: tig spurious-executable-perm /usr/share/man/man5/tigrc.5.gz The man pages are installed 755, this has already been fixed upstream, so I can patch before the next release, if needed. Looks good. I do think a quick chmod of the manpages is in order just for the sake of cleanliness, although I can't imagine how the weird permissions could actually hurt anything. The compiler is not called with the proper set of flags, which leads to a busted debuginfo package among other things. I believe it suffices to pass CFLAGS on the make line: make CFLAGS='%{optflags}' %{?_smp_mflags} all doc-man The only other suggestion I have is that while don't think there's anything wrong with the summary and description you have, perhaps not everyone who sees the summary or description will know what git is. Maybe something like: Summary: Text-mode interface for the git revision control system %description Tig is a repository browser for the git revision control system that... Review: * source files match upstream: ad611a9449ff9786892183874ee79255ab92e82b3d1251556cd1a0cd1224ee12 tig-0.7.tar.gz * package meets naming and versioning guidelines. * specfile is properly named, is cleanly written and uses macros consistently. O summary is OK (see comments above, not a blocker) O description is OK ("") * dist tag is present. * build root is OK. * license field matches the actual license. * license is open source-compatible. * license text included in package. * latest version is being packaged. * BuildRequires are proper. X compiler flags are appropriate. * %clean is present. * package builds in mock (development, x86_64). * package installs properly X debuginfo package is mostly empty. * rpmlint is silent. * final provides and requires are sane: tig = 0.7-1.fc7 = git-core libncurses.so.5()(64bit) libtinfo.so.5()(64bit) * %check is not present (no upstream test suite) but package runs fine. * no shared libraries are added to the regular linker search paths. * owns the directories it creates. * doesn't own any directories it shouldn't. * no duplicates in %files. X file permissions are appropriate (executable manpages) * no scriptlets present. * code, not content. * documentation is small, so no -docs subpackage is necessary. * %docs are not necessary for the proper functioning of the package. * no headers. * no pkgconfig files. * no static libraries. * no libtool .la files. * not a GUI app. *** Bug 241990 has been marked as a duplicate of this bug. *** (In reply to comment #2) > Looks good. I do think a quick chmod of the manpages is in order just for the > sake of cleanliness, although I can't imagine how the weird permissions could > actually hurt anything. > > The compiler is not called with the proper set of flags, which leads to a busted > debuginfo package among other things. I believe it suffices to pass CFLAGS on > the make line: > make CFLAGS='%{optflags}' %{?_smp_mflags} all doc-man > > The only other suggestion I have is that while don't think there's anything > wrong with the summary and description you have, perhaps not everyone who sees > the summary or description will know what git is. Maybe something like: > > Summary: Text-mode interface for the git revision control system > %description > Tig is a repository browser for the git revision control system that... This all sounds good. I've updated the spec with your suggestions. Thanks, Jason. New locations: Spec URL: http://jbowes.dangerouslyinc.com/tmp/tig.spec SRPM URL: http://jbowes.dangerouslyinc.com/tmp/tig-0.7-2.fc7.src.rpm A few of things that I did in my spec (none of them blockers): 1. Build the HTML documentation and list them in %doc. After they are built you'll need to convert CRLF line endings to LF line endings. 2. Include the manual.txt in %doc. 3. Added -DVERSION\\\"%{version}-%{release}\\\" to the CFLAGS. That way tig will print the rpm release when you type "tig --version". Could help with debugging down the road... 4. Use %{_mandir} and %{_prefix} instead of hard-coded paths in the "make install" line. Otherwise my spec and your spec are nearly identical. Feel free to list me as a co-maintainer once the package is approved. (In reply to comment #5) > A few of things that I did in my spec (none of them blockers): > > 1. Build the HTML documentation and list them in %doc. After they are built > you'll need to convert CRLF line endings to LF line endings. > > 2. Include the manual.txt in %doc. > > 3. Added -DVERSION\\\"%{version}-%{release}\\\" to the CFLAGS. That way tig > will print the rpm release when you type "tig --version". Could help with > debugging down the road... > > 4. Use %{_mandir} and %{_prefix} instead of hard-coded paths in the "make > install" line. > I went ahead and updated the spec with these, with the exception of #3. It feels kind of weird to me, but if you feel strongly about it, we can put it in later. New locations: Spec URL: http://jbowes.dangerouslyinc.com/tmp/tig.spec SRPM URL: http://jbowes.dangerouslyinc.com/tmp/tig-0.7-3.fc7.src.rpm > Otherwise my spec and your spec are nearly identical. Feel free to list me as a > co-maintainer once the package is approved. Will do! (In reply to comment #6) > (In reply to comment #5) > > 3. Added -DVERSION\\\"%{version}-%{release}\\\" to the CFLAGS. That way tig > > will print the rpm release when you type "tig --version". Could help with > > debugging down the road... > > with the exception of #3. It feels > kind of weird to me, but if you feel strongly about it, we can put it in later. It's not a big deal to me, just something I picked up from the upstream spec file. Looking good to me: Symmary and %description look great now. Compiler flags are OK and debuginfo package is complete. Permissions of the manpages are OK. The other additions are OK. The included documentation is getting larger than the program itself, but the whole thing's so tiny that it doesn't much matter. APPROVED New Package CVS Request ======================= Package Name: tig Short Description: Text-mode interface for the git revision control system Owners: jbowes, jeff Branches: F-7 InitialCC: (In reply to comment #8) > Looking good to me: Thanks for the review, Jason! CVS done. Package Change Request ====================== Package Name: tig New Branches: el5 el6 Owners: stevetraylen I asked for an EPEL branch in bug #722951 some time ago. I will maintain these branches now. Steve. Git done (by process-git-requests). tig-0.17-1.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/tig-0.17-1.el6 tig-0.17-1.el5.1 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/tig-0.17-1.el5.1 tig-0.17-1.el5.1 has been pushed to the Fedora EPEL 5 stable repository. tig-0.17-1.el6 has been pushed to the Fedora EPEL 6 stable repository. |