Spec URL: http://blackbean.org/review/ledger.spec SRPM URL: http://blackbean.org/review/ledger-2.6.1-1.fc10.src.rpm Description: Ledger is a powerful, double-entry accounting system that is accessed from the UNIX command-line. This may put off some users — as there is no flashy UI — but for those who want unparalleled reporting access to their data, there really is no alternative.
Some initial comments: - If packaging Emacs modes, you might want to consider making a separate subpackage that depends on emacs. See for instance how I handle it for vala: http://cvs.fedoraproject.org/viewvc/devel/vala/vala.spec?revision=1.36 - chmod -x scripts/* should probably be done in %setup, not %build. It's not really part of the build process - There are some GCC 4.4 incompatibilities that will need to be fixed: http://koji.fedoraproject.org/koji/taskinfo?taskID=1240356 C++'s <cstr> and <string.h> functions now return a const char * if the input is const char *, and a char * if the input is char *. - You modified the .texi file, but do not BuildRequire: texinfo, so the documentation is not regenerated See F-10 build log: http://koji.fedoraproject.org/koji/taskinfo?taskID=1240369
(In reply to comment #1) > - If packaging Emacs modes, you might want to consider making a separate > subpackage that depends on emacs. See for instance how I handle it for vala: > http://cvs.fedoraproject.org/viewvc/devel/vala/vala.spec?revision=1.36 I think this is overkill for small files (like .el files). Plus, I can't picture wanting emacs installed because I installed ledger. > - chmod -x scripts/* should probably be done in %setup, not %build. It's not > really part of the build process Moved. > C++'s <cstr> and <string.h> functions now return a const char * if the input > is const char *, and a char * if the input is char *. Patched. > - You modified the .texi file, but do not BuildRequire: texinfo, so the > documentation is not regenerated Fixed. Thanks for your comments. Spec URL: http://blackbean.org/review/ledger.spec SRPM URL: http://blackbean.org/review/ledger-2.6.1-2.fc10.src.rpm
(In reply to comment #2) > (In reply to comment #1) > > - If packaging Emacs modes, you might want to consider making a separate > > subpackage that depends on emacs. See for instance how I handle it for vala: > > http://cvs.fedoraproject.org/viewvc/devel/vala/vala.spec?revision=1.36 > > I think this is overkill for small files (like .el files). Plus, I can't > picture wanting emacs installed because I installed ledger. Sure, you don't want to get emacs pulled in by default. Which is why I suggested splitting off the emacs editing modes; this is the convention for other packages that come with emacs modes; see e.g. emacs-{gambit,git,gnuplot,lua,mercurial,pyrex,vala}). All of these are split off from their main packages, so that the main package itself does not depend on Emacs. The other alternatives you could do is 1) Not package the Emacs mode files. That would be a shame 2) Own /usr/share/emacs and /usr/share/emacs/site-packages. This is problematic. Not owning these mean that if a user install ledger on a system *without* emacs installed, uninstalling ledger means these directories will be left dangling, as they are not registered as belonging to the package. On the other hand, owning these would also be problematic and against the guidelines (though some packages still do this): as much as possible, a package should not own directories that belong to other packages, in case the two specify different permissions for the directories / files concerned. There are exceptions -- bash-completion support scripts are considered trivial enough that packages normally just own /etc/bash_completion.d rather than pull in bash-completion as a dependency. Hope that clarifies things. Now that the lowest supported Fedora version is F-9, whose Emacs comes with pkgconfig, properly packaging Emacs modes is not hard (you don't have to hardcode the Emacs version anymore). Unless you want to target Red Hat Enterprise Linux (and CentOS).. so I'd probably suggest either dropping the .el files or packaging them properly.
(In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > - If packaging Emacs modes, you might want to consider making a separate > > > subpackage that depends on emacs. See for instance how I handle it for vala: > > > http://cvs.fedoraproject.org/viewvc/devel/vala/vala.spec?revision=1.36 > > I think this is overkill for small files (like .el files). Plus, I can't > > picture wanting emacs installed because I installed ledger. > Sure, you don't want to get emacs pulled in by default. Which is why I > suggested splitting off the emacs editing modes; this is the convention for > other packages that come with emacs modes; see e.g. Let's just say I have emacs installed and I use it, and I hear about ledger, so I install it (yum install ledger) and start playing around with it. I open a few ledger text files, but no emacs mode is auto-detected. I probably wouldn't even know a ledger mode exists because some packager didn't bother to install a single text file. User cost: lack of a full experience, User gain: 4k disk savings and no dangling directory. Let's say we did include a the el files. The user always gets the full access to ledger mode. User benefit: satisfaction, User cost: 4k disk usage and a dangling directory. As far as I'm concerned the dangling directory is a tool bug/wart and should be treated as such. Same with not being able to automatically install emacs-ledger if someone installs ledger and emacs. Another tool bug. Without these being fixed, there is a *clear* choice that make the user's life better with *very* little cost. > emacs-{gambit,git,gnuplot,lua,mercurial,pyrex,vala}). All of these are split > off from their main packages, so that the main package itself does not depend > on Emacs. I think these should be fixed as well. The emacs-git situation is totally uncalled for. vc-git should just work out of the box if I "yum install git".
Sorry for the delay, I'll try and post an updated response later today.
Until RPM supports soft dependencies of the recommends/suggests sort, there is no good way of fixing the issue. I'm personally happy with the current guidelines -- though unhappy with the lack of soft dependency -- but given that your current package violates two compulsory points: http://fedoraproject.org/wiki/Packaging:UnownedDirectories http://fedoraproject.org/wiki/Packaging:ReviewGuidelines#Things_To_Check_On_Review MUST: Packages must not own files or directories already owned by other packages I'd find it hard to go ahead with this review. I suggest contacting the fedora-packaging list for a clarification, or I can step back and you can try convincing another maintainer that dangling links are the best way out of this. (when RPM gets the ability to recommends packages, of course, you would be able to just make the main ledger app recommends emacs-ledger, and keep Vim users happy)
> http://fedoraproject.org/wiki/Packaging:UnownedDirectories > MUST: Packages must not own files or directories already owned by other packages It is common for this rule to be broken for the very reasons I outlined. See for example the rule on MIME files. https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo Notice that like bash_completion, you just drop a file in the directory. If you have shared-mime-info, bash-completion or in this case emacs installed then things just work and if you don't they work as well. Win-win. The reason humans do reviews is so they can make exceptions to well meaning but not always applicable rules. If these prominent examples are not enough to convince you, I'm sure I can find more... See: clisp puppet monotone ... Then again there are lots of packages that have a separate emacs packages. I still hold that this is a stupid practice. I want things to just work and given the limits of rpm/yum, the best way in this case to just include the emacs mode in the main package. As for the directory ownership issue, I'm ambivalent. Some packages (like puppet) grab the common directory and some like the users of bash-completion and shared-mime-info don't. Given that the latter uses are explicit policy, I lean toward that, but both have merits. > I'd find it hard to go ahead with this review. I suggest contacting the fedora-packaging list for a clarification, or I can step back and you can try convincing another maintainer that dangling links are the best way out of this. No problem. Thanks for your time. I'll consider bringing it up on the list if no one else steps up.
That's a bad example -- /usr/share/mime is actually owned by the filesystem package as well, which is guaranteed to be installed at all times. With bash-completion, the consensus is actually to grab the directory. The rationale is similar to the one you outline -- that it's too trivial to create a subpackage. With emacs support, the consensus is the other way around, as far as I can tell, and any heavy Emacs user would probably know to look for emacs-* subpackages anyway. If you want, you can mention it in the package description or in a README.Fedora file. Any deviation is a bug (I'd appreciate it if you could file those, they do need to be fixed). If I still don't convince you, then let me know and I'll remove myself from the bug.
(In reply to comment #8) > That's a bad example -- /usr/share/mime is actually owned by the filesystem > package as well, which is guaranteed to be installed at all times. $ rpm -qf /usr/share/mime/packages shared-mime-info-0.70-3.fc12.x86_64 > With bash-completion, the consensus is actually to grab the directory. The > rationale is similar to the one you outline -- that it's too trivial to create > a subpackage. I would say that rationale applies in this case as well. > With emacs support, the consensus is the other way around, as far as I can tell, I stopped looking after finding a few examples to support my position, but the official policy agrees with you. https://fedoraproject.org/wiki/Packaging:Emacs They also mention issues with the .elc files across versions. > any heavy Emacs user would probably know to look for emacs-* > subpackages anyway. I know to look for them, but I *hate* doing it. The elisp files are small text files that affect nothing if you don't have emacs installed but provide joy and happiness if you do. The choice is clear to me, especially in this case where the program was *intended* to be used with emacs. > Any deviation is a bug (I'd appreciate it if you could file those, they do need to be fixed). As you might expect, I disagree that it's a bug. The rationale you mention "size of the files" clearly doesn't apply in this case. $ wc -l /usr/share/emacs/site-lisp/ledger.el* 1307 /usr/share/emacs/site-lisp/ledger.el 255 /usr/share/emacs/site-lisp/ledger.elc $ wc -l /etc/bash_completion.d/{git,subversion} 2276 /etc/bash_completion.d/git 1332 /etc/bash_completion.d/subversion > If I still don't convince you, then let me know and I'll remove myself from the bug. You haven't convinced me, but you represent the official policy just fine. I plan to look for a technical solution, but failing that I'll put it in a separate package.
(In reply to comment #9) > (In reply to comment #8) > > That's a bad example -- /usr/share/mime is actually owned by the filesystem > > package as well, which is guaranteed to be installed at all times. > > $ rpm -qf /usr/share/mime/packages > shared-mime-info-0.70-3.fc12.x86_64 > My mistake; I was checking at work and the only RPM machines there are running openSUSE. The openSUSE solution would actually make sense: make /usr/share/mime and /usr/share/mime/packages be part of the basic filesystem package. I'll check with the Fedora packaging list on the specific issue of shared-mime-info, actually. One would think that it should be a component installed by default, but that no other package depends on (per packaging policy), but as it is, it is pulled in by PackageKit. Thanks for noticing!
*** Bug 602064 has been marked as a duplicate of this bug. ***
Jim, I'd packaged up a version in bug 602064 before seeing your post here about it. I'd broken out the sub-packages in what I think was a Fedora-approved manner (and included the man page from the Ubuntu package), although your version handles regeneration of the info documentation in a much more sane manner than mine. The rest looks pretty much identical. (You have some handling in place for building git versions, although looking over the CentOS spec for git HEAD, all of that is basically out the window anyway. ;-) Since you beat me to the punch by well over a year and have obviously spent more time thinking about it, I'll defer to your build; just thought I'd mention the one I'd put together, in case it's helpful.
It may make sense to re-synchronize this review against upstream's 'git' as there have been many improvements in the last year -- Russ herrold
Michel, this version splits out the emacs packages according to the guidelines. https://fedoraproject.org/wiki/Packaging:Emacs Ed, this incorporates the Debian man page you had included. Spec URL: http://blackbean.org/review/ledger.spec SRPM URL: http://blackbean.org/review/ledger-2.6.2-3.fc10.src.rpm
Typo. We are at fc13 now, not fc10. How time flies. Spec URL: http://blackbean.org/review/ledger.spec SRPM URL: http://blackbean.org/review/ledger-2.6.2-3.fc13.src.rpm
Ah, wonderful. Thanks, Jim and Ed. Expect the updated review later today -- should be a formality, really, as we were only stuck on the subpackages issue, but I'll try and verify the new features from upstream as well.
Some minor issues need fixing; see below for details #+TODO: TODO(t) WAIT(w@/!) FAIL(f@) | DONE(d) N/A(n) * TODO Review [44%] ** DONE Names [2/2] *** DONE Package name *** DONE Spec name ** DONE Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]] ** FAIL source files match upstream - State "FAIL" from "TODO" [2010-06-11 Fri 22:51] \\ Bundled tgz's MD5 matches upstream, however the URL listed is wrong 2.6.2 is only available from GitHub, not SF: http://github.com/downloads/jwiegley/ledger/ledger-2.6.2.tar.gz ** DONE License [3/3] *** DONE License is Fedora-approved BSD *** DONE License field accurate *** DONE License included iff packaged by upstream ** TODO rpmlint [0/2] *** FAIL on src.rpm - State "FAIL" from "TODO" [2010-06-11 Fri 23:18] \\ $ rpmlint ./ledger-2.6.2-3.fc13.src.rpm ledger.src:92: E: files-attr-not-set ledger.src:95: E: files-attr-not-set ledger.src: W: invalid-url Source0: http://downloads.sourceforge.net/ledger/ledger-2.6.2.tar.gz HTTP Error 404: Not Found 1 packages and 0 specfiles checked; 2 errors, 1 warnings. Each subpackage needs a %defattr line *** WAIT on x86_64.rpm - State "WAIT" from "TODO" [2010-06-11 Fri 23:45] \\ $ rpmlint ~/Downloads/*ledger*.rpm emacs-common-ledger.x86_64: W: no-documentation emacs-common-ledger-el.x86_64: W: spelling-error Summary(en_US) elisp -> lisp, e lisp, Elise emacs-common-ledger-el.x86_64: W: spelling-error %description -l en_US elisp -> lisp, e lisp, Elise emacs-common-ledger-el.x86_64: W: no-documentation ledger.x86_64: W: incoherent-version-in-changelog 2.6.1-1 ['2.6.2-3.fc13', '2.6.2-3'] ledger-devel.x86_64: W: spelling-error %description -l en_US libamounts -> lib amounts, lib-amounts, amounts ledger-devel.x86_64: W: no-documentation 5 packages and 0 specfiles checked; 0 errors, 7 warnings. - ChangeLog needs updating - Spelling errors can be ignored - Documentation warning can be ignored ** DONE Language & locale [3/3] *** DONE Spec in US English *** DONE Spec legible *** N/A Use %find_lang to handle locale files ** TODO Build [2/3] *** DONE Koji results http://koji.fedoraproject.org/koji/taskinfo?taskID=2245711 *** DONE BRs complete *** TODO Directory ownership ** TODO Spec inspection *** DONE ldconfig for libraries *** TODO No duplicate files *** TODO File permissions *** TODO Filenames must be UTF-8 *** DONE Has %clean section (except F-13+: https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean) *** TODO %buildroot cleaned on %install *** DONE Macro usage consistent *** TODO Documentation **** TODO If large docs, separate -doc **** TODO %doc files are non-essential *** DONE Development [4/4] **** DONE Headers in -devel **** DONE If versioned .so's, unversioned in -devel **** DONE -devel requires main **** DONE No .la *** FAIL Other subpackages - State "FAIL" from "" [2010-06-11 Fri 23:44] \\ Emacs packaging: emacs-common-ledger and emacs-common-ledger-el should be swapped: -el for source files, not for byte-compiled files https://fedoraproject.org/wiki/Packaging:Emacs#Package_naming_and_sub-package_organization On F-11+ (all the versions still supported), RPM supports noarch subpackages. You might want to make the Emacs subpackages noarch https://fedoraproject.org/wiki/Packaging:Emacs#Use_of_BuildArch:_noarch ** TODO [[http://fedoraproject.org/wiki/Packaging/ScriptletSnippets][Scriptlets]] *** FAIL info files - State "FAIL" from "TODO" [2010-06-11 Fri 23:29] \\ Minor issue: probably should Requires(post) and Requires(preun) the info package rather than the /sbin/install-info binary
New version with fixes for all of the above. Spec URL: http://blackbean.org/review/ledger.spec SRPM URL: http://blackbean.org/review/ledger-2.6.3-1.fc13.src.rpm
One last thing -- the Emacs packages should be emacs-ledger and emacs-ledger-el since they won't work on XEmacs -- unless you're supporting both GNU and XEmacs explicitly, -common- should not exist. See the packaging guideline -- XEmacs files are installed into a totally separate directory. Sorry for forgetting to mention this. We don't have a package popularity survey, but from looking at Debian's numbers it looks like GNU Emacs installations are outnumbering XEmacs by a ratio of 4:1, so given the two choices of supporting GNU Emacs only or supporting both, the former is probably easiest. (From experience, I package bigloo which supports both Emacs flavors, but in the latest versions XEmacs support does not even work out-of-the-box and I ended up disabling them until a fix is found) Apart from the subpackage naming, this package is ready for approval - make that change and I'll officially approve it.
I removed -common. Spec URL: http://blackbean.org/review/ledger.spec SRPM URL: http://blackbean.org/review/ledger-2.6.3-2.fc13.src.rpm
Michel, would it be possible for you to do a final review of this package? (Don't mean to prod, just wanted to make sure you didn't miss that Jim had packaged up an update.)
Apologies for the delay. The last update fixes all outstanding issue -- package is APPROVED.
New Package SCM Request ======================= Package Name: ledger Short Description: A powerful command-line double-entry accounting system Owners: radford Branches: f12 f13 f14 el5 el6 InitialCC:
Git done (by process-git-requests).
ledger-2.6.3-2.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/ledger-2.6.3-2.fc13
ledger-2.6.3-2.fc12 has been submitted as an update for Fedora 12. https://admin.fedoraproject.org/updates/ledger-2.6.3-2.fc12
ledger-2.6.3-2.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/ledger-2.6.3-2.fc14
ledger-2.6.3-2.fc13 has been pushed to the Fedora 13 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update ledger'. You can provide feedback for this update here: https://admin.fedoraproject.org/updates/ledger-2.6.3-2.fc13
ledger-2.6.3-2.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report.
ledger-2.6.3-2.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report.
ledger-2.6.3-2.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report.