Bug 478613 - Review Request: ledger - A powerful command-line double-entry accounting system
Review Request: ledger - A powerful command-line double-entry accounting system
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
: 602064 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-02 00:15 EST by Jim Radford
Modified: 2010-10-05 05:27 EDT (History)
7 users (show)

See Also:
Fixed In Version: ledger-2.6.3-2.fc13
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-10-05 05:27:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
michel: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jim Radford 2009-01-02 00:15:22 EST
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.
Comment 1 Michel Alexandre Salim 2009-03-13 14:53:51 EDT
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
Comment 2 Jim Radford 2009-03-14 10:42:49 EDT
(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
Comment 3 Michel Alexandre Salim 2009-03-14 23:02:11 EDT
(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.
Comment 4 Jim Radford 2009-03-15 10:55:04 EDT
(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".
Comment 5 Michel Alexandre Salim 2010-05-24 12:35:48 EDT
Sorry for the delay, I'll try and post an updated response later today.
Comment 6 Michel Alexandre Salim 2010-05-24 14:21:56 EDT
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)
Comment 7 Jim Radford 2010-05-25 12:57:40 EDT
> 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.
Comment 8 Michel Alexandre Salim 2010-05-25 13:26:15 EDT
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.
Comment 9 Jim Radford 2010-05-25 14:29:51 EDT
(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.
Comment 10 Michel Alexandre Salim 2010-05-25 14:46:45 EDT
(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!
Comment 11 Randall "Randy" Berry 2010-06-09 03:16:48 EDT
*** Bug 602064 has been marked as a duplicate of this bug. ***
Comment 12 Ed Marshall 2010-06-09 09:04:34 EDT
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.
Comment 13 R P Herrold 2010-06-09 11:54:35 EDT
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
Comment 14 Jim Radford 2010-06-09 12:38:46 EDT
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
Comment 15 Jim Radford 2010-06-09 13:56:00 EDT
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
Comment 16 Michel Alexandre Salim 2010-06-11 05:00:53 EDT
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.
Comment 17 Michel Alexandre Salim 2010-06-11 17:50:24 EDT
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
Comment 18 Jim Radford 2010-07-14 18:15:29 EDT
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
Comment 19 Michel Alexandre Salim 2010-07-16 07:06:56 EDT
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.
Comment 20 Jim Radford 2010-07-17 09:17:46 EDT
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
Comment 21 Ed Marshall 2010-09-09 14:46:59 EDT
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.)
Comment 22 Michel Alexandre Salim 2010-09-12 08:20:38 EDT
Apologies for the delay. The last update fixes all outstanding issue -- package is APPROVED.
Comment 23 Jim Radford 2010-09-12 11:36:34 EDT
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:
Comment 24 Kevin Fenzi 2010-09-12 18:52:03 EDT
Git done (by process-git-requests).
Comment 25 Fedora Update System 2010-09-14 00:10:20 EDT
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
Comment 26 Fedora Update System 2010-09-14 00:11:57 EDT
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
Comment 27 Fedora Update System 2010-09-14 00:14:26 EDT
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
Comment 28 Fedora Update System 2010-09-15 01:39:51 EDT
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
Comment 29 Fedora Update System 2010-10-01 08:24:28 EDT
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.
Comment 30 Fedora Update System 2010-10-05 05:24:14 EDT
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.
Comment 31 Fedora Update System 2010-10-05 05:27:51 EDT
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.

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