This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 991639 - Review Request: python-facct - Fig's accountancy tool
Review Request: python-facct - Fig's accountancy tool
Status: CLOSED DEFERRED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2013-08-03 03:47 EDT by efigue
Modified: 2015-07-21 10:01 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-07-21 10:01:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
one way to fix the spec (1.82 KB, patch)
2013-08-04 03:46 EDT, Michael Schwendt
no flags Details | Diff

  None (edit)
Description efigue 2013-08-03 03:47:58 EDT
Spec URL: http://efigue.foss.free.fr/python-facct.spec
SRPM URL: http://efigue.foss.free.fr/python-facct-0.1.2-1.fc19.src.rpm

Description: I just packaged python-facct and python3-facct. I would appreciate a review to know if we could push it in Fedora.

About Facct:
Manages accountancy in command line.
  - Double entry accounting
  - Allows you to register entries in CSV format (via spreadsheet)
  - Manages repetitive patterns
  - Reads HSBC statements and transforms them into bookkeeping entries
  - Generates Journal, General Ledger, Balance sheet and Income statements
  - Closes the year and retrieves the entries to open the new year
  - Understands French corporate taxes and social contributions
  - Reads calendar delivery entries for consulting company in order to:
      - generate PDF bill from latex template
      - enter billing in accounts
  - Localized in French and English

Status: Still a lot to be done to be considered as quality software (localization, code, genericity) but it has already been used for 4 years by my very little company. So, I'm in the process of improving it to share it with others.

FE-NEEDSPONSOR
Fedora Account System Username:efigue

- Eric
Comment 1 Christopher Meng 2013-08-03 06:30:39 EDT
Don't change the version from rawhide to 19, or this bug may become closed when 19 is EOL.

ISSUE FIRST NEED TO BE SOLVED:

Except py3 define, please do not made your spec like a template, please remove other %define and move them to the right space. Like version or release,  please write the number in the tag field, do not use define to define them. 

And please change %define to %global, we now recommend using their latter one. 

Also, remove %clean section.
Comment 2 Christopher Meng 2013-08-03 06:57:20 EDT
Also, cleanup:

1. BuildRoot/Prefix/Vendor tag. We don't use these junk.

2. Remove rm -rf %{buildroot} in %install section.

3. Remove %defattr(-,root,root) in %files.

4. DO NOT use INSTALLED_FILES to list files, please manually write down all.
Comment 3 Michael Schwendt 2013-08-03 07:51:51 EDT
Confirmed. Macro-madness is not a blocker according to the guidelines, but all those macros will cause you some headaches eventually. I won't approve a package that redefines several implicitly defined macros. Examples:

> %define version 0.1.2

The "Version" tag already defines %{version}, so replace

  Version: %{version}

with

  Version: 0.1.2

and you can use %{version} anywhere you like.


> %define release 1

This is wrong and misleading. Here you assign the value 1 to %release, but further down in the spec you do
 
  Release: %{release}%{?dist}

which defines %{release} to this new value. The old definition is lost. Mass-rebuild scripts also will have a lot of fun trying to figure out whether to modify (= "bump") the Release tag or the definition of "release". Please don't make it so complicated.


> Url: %{url}

"URL: http://efigue.foss.free.fr" and you're done. Well, you reuse %{url} for the Source0 tag, but hey, the "URL" tag defines %{url}, too. This is not an obfuscation contest, but a rather simple package which should come with a rather simple and readable spec file.


>  %define name python-facct

Again, the "Name" tag defines %{name}. There is really no need to redefine these macros at the top of the spec file, becoming a crowded place, and you would need to scroll back'n'forth to locate where and how those macros are used throughout the spec file.


* Run rpmlint (or rpmlint -I for more helpful output) on the src.rpm and all
built rpms. Feel free to ignore obvious false positives in the report, but fix
anything else. Preferably add a comment here about whether/when you think what
rpmlint reports is correct or incorrect.

  $ rpmlint python-facct-0.1.2-1.fc19.src.rpm 
  python-facct.src: W: file-size-mismatch facct-0.1.2.tar.gz = 316035, http://efigue.foss.free.fr/facct-0.1.2.tar.gz = 316042
  1 packages and 0 specfiles checked; 0 errors, 1 warnings.


> %files -f %{_tmppath}/INSTALLED_FILES
> %files -f %{srcname}.lang

This is severely flawed. Just query the package contents to verify what is included currently:

  $ rpmls -p python-facct-0.1.2-1.fc19.noarch.rpm|grep ^d
  drwxr-xr-x  /usr/lib/python2.7/site-packages/facct-0.1.2-py2.7.egg-info
  drwxr-xr-x  /usr/share/doc/python-facct-0.1.2

Directories are missing!
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

There are also duplicate files in both packages, e.g.:

$ rpmls -p python3-facct-0.1.2-1.fc19.noarch.rpm | grep locale
-rw-r--r--  /usr/lib/python2.7/site-packages/facct/locale/fr_FR/LC_MESSAGES/facct.mo
-rw-r--r--  /usr/lib/python3.3/site-packages/facct/locale/fr_FR/LC_MESSAGES/facct.mo

https://fedoraproject.org/wiki/Packaging:Guidelines#Duplicate_Files
Comment 4 efigue 2013-08-03 14:50:15 EDT
(In reply to Christopher Meng from comment #2)
> Also, cleanup:
> 
> 1. BuildRoot/Prefix/Vendor tag. We don't use these junk.
> 
> 2. Remove rm -rf %{buildroot} in %install section.
> 
> 3. Remove %defattr(-,root,root) in %files.
> 
> 4. DO NOT use INSTALLED_FILES to list files, please manually write down all.

Thanks for the review.

I cleaned up RPM spec file:
     - remove clean section, defattr removed, %define -> %global
     - remove also rm -rf in install section
     - not define some macro automatically defined (source, buildroot, release, url)
     - remove Vendor, Prefix, BuildRoot
     - remove unwanted .mo file (don't know how to use %search_lang while packaging simultaneously for Python2 qnd Python3.)
Comment 5 efigue 2013-08-03 14:53:12 EDT
(In reply to Michael Schwendt from comment #3)
> Confirmed. Macro-madness is not a blocker according to the guidelines, but
> all those macros will cause you some headaches eventually. I won't approve a
> package that redefines several implicitly defined macros. Examples:
> 
> > %define version 0.1.2
> 
> The "Version" tag already defines %{version}, so replace
> 
>   Version: %{version}
> 
> with
> 
>   Version: 0.1.2
> 
> and you can use %{version} anywhere you like.
> 
> 
> > %define release 1
> 
> This is wrong and misleading. Here you assign the value 1 to %release, but
> further down in the spec you do
>  
>   Release: %{release}%{?dist}
> 
> which defines %{release} to this new value. The old definition is lost.
> Mass-rebuild scripts also will have a lot of fun trying to figure out
> whether to modify (= "bump") the Release tag or the definition of "release".
> Please don't make it so complicated.
> 
> 
> > Url: %{url}
> 
> "URL: http://efigue.foss.free.fr" and you're done. Well, you reuse %{url}
> for the Source0 tag, but hey, the "URL" tag defines %{url}, too. This is not
> an obfuscation contest, but a rather simple package which should come with a
> rather simple and readable spec file.
> 
> 
> >  %define name python-facct
> 
> Again, the "Name" tag defines %{name}. There is really no need to redefine
> these macros at the top of the spec file, becoming a crowded place, and you
> would need to scroll back'n'forth to locate where and how those macros are
> used throughout the spec file.
> 
> 
> * Run rpmlint (or rpmlint -I for more helpful output) on the src.rpm and all
> built rpms. Feel free to ignore obvious false positives in the report, but
> fix
> anything else. Preferably add a comment here about whether/when you think
> what
> rpmlint reports is correct or incorrect.
> 
>   $ rpmlint python-facct-0.1.2-1.fc19.src.rpm 
>   python-facct.src: W: file-size-mismatch facct-0.1.2.tar.gz = 316035,
> http://efigue.foss.free.fr/facct-0.1.2.tar.gz = 316042
>   1 packages and 0 specfiles checked; 0 errors, 1 warnings.
> 
> 
> > %files -f %{_tmppath}/INSTALLED_FILES
> > %files -f %{srcname}.lang
> 
> This is severely flawed. Just query the package contents to verify what is
> included currently:
> 
>   $ rpmls -p python-facct-0.1.2-1.fc19.noarch.rpm|grep ^d
>   drwxr-xr-x  /usr/lib/python2.7/site-packages/facct-0.1.2-py2.7.egg-info
>   drwxr-xr-x  /usr/share/doc/python-facct-0.1.2
> 
> Directories are missing!
> https://fedoraproject.org/wiki/Packaging:UnownedDirectories
> 
> There are also duplicate files in both packages, e.g.:
> 
> $ rpmls -p python3-facct-0.1.2-1.fc19.noarch.rpm | grep locale
> -rw-r--r-- 
> /usr/lib/python2.7/site-packages/facct/locale/fr_FR/LC_MESSAGES/facct.mo
> -rw-r--r-- 
> /usr/lib/python3.3/site-packages/facct/locale/fr_FR/LC_MESSAGES/facct.mo
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Duplicate_Files

Thanks for the review and explanations.
I uploaded a new version also for the size check.

Regards
Comment 6 Michael Schwendt 2013-08-03 16:24:22 EDT
> remove unwanted .mo file (don't know how to use %search_lang
> while packaging simultaneously for Python2 qnd Python3.)

You could have asked. ;-)

https://fedoraproject.org/wiki/Packaging:Guidelines#Why_do_we_need_to_use_.25find_lang.3F


One solution would be straight-forward. You've had this

  %find_lang %{srcname}

which in the %{buildroot} contents found the message object files and created a %files list in file "%{srcname}.lang". You could simply extract lines from that file, which contain %{python_sitelib} or %{python3_sitelib} in their path to create two new lists for your two sub-packages, which to use in your %files sections instead of the original list:

  grep '%{python_sitelib}' %{srcname}.lang > py2%{srcname}.lang
  grep '%{python3_sitelib}' %{srcname}.lang > py3%{srcname}.lang


> %{python3_sitelib}/%{srcname}/__pycache__/*
> %{python3_sitelib}/%{srcname}/*/__pycache__/*

Here you show that you do know that you can use wildcards, but in the update package you listed each and every installed file in the %files list. Wow, that came unexpected. :)

The %files list for the python2 package can get as simple as:

  %{python_sitelib}/%{srcname}*

to include everything correctly.

Similarly for the python3 based subpackage. You didn't tell why you don't include the __pycache__ directory, btw.
Comment 7 efigue 2013-08-03 20:19:06 EDT
(In reply to Michael Schwendt from comment #6)
> The %files list for the python2 package can get as simple as:
> 
>   %{python_sitelib}/%{srcname}*
> 
> to include everything correctly.
Ok! That said I split it into:
%lang(fr) %{python_sitelib}/%{srcname}/locale/fr_FR/LC_MESSAGES/facct.mo
%{python_sitelib}/%{srcname}*.egg-info
%{python_sitelib}/%{srcname}/[^l]*

I used %lang to avoid rpmlint warning about file-not-in-%lang and split globbing to remove locale/ directory from inclusion to avoid rpmbuild warning "files listed twice".
=> rpmlint and mock seem happy.

> Similarly for the python3 based subpackage. You didn't tell why you don't
> include the __pycache__ directory, btw.
In Python3, .pyo and .pyc are now tidied in __pycache__ directories.

Regards
Comment 8 Michael Schwendt 2013-08-04 03:44:24 EDT
Hmmm, then some more work is needed. I didn't pay attention to where the locale dir is installed in the python module path. Embarrassing. Of course that created "files listed twice" warnings. Sorry! But the current spec doesn't fix it, as it still creates several "unowned" directories that would not be removed when uninstalling the package:

  %{python_sitelib}/%{srcname}/locale/
  %{python_sitelib}/%{srcname}/locale/fr_FR/
  %{python_sitelib}/%{srcname}/locale/fr_FR/LC_MESSAGES/

Below  /usr/share/locale/  that would not be a problem, because that tree is owned by either the filesystem and glibc packages. And therefore %find_lang only needs to find the individual files, not create any %dir entries for them.

When not moving the locale dir, the %files section would get a little bit more complex, but %find_lang could be used and is a MUST according to the guidelines (even if it may appear overhead for just the single French translation).

> %{python_sitelib}/%{srcname}/[^l]*

As short and convenient as it may seem, it would get intricate and tedious whenever you wanted to add a file starting with l.

Patch for the locale issue follows...
Comment 9 Michael Schwendt 2013-08-04 03:46:27 EDT
Created attachment 782418 [details]
one way to fix the spec
Comment 10 efigue 2013-08-04 08:09:50 EDT
(In reply to Michael Schwendt from comment #9)
> Created attachment 782418 [details]
> one way to fix the spec

I uploaded the new version.

Vielen Dank für Ihre Hilfe
Eric
Comment 11 Michael Schwendt 2013-08-27 13:00:32 EDT
So, "fedora-review -b 991639" completes and doesn't complain about anything.

Let's see what else there is:

[...]

Differences between python3-facct and python-facct.

 * The Python2 build specifies "Group: Development/Libraries", whereas the Python3 build sets "Group: Applications/Productivity".

 * The Python2 requires texlive-latex and ghostscript, the Python3 build doesn't.

Those are no blockers, but worth fixing later prior to a first release at Fedora.

As for the run-time, I'm not sure, do you want this to be more of a Python module/framework or a set of command-line tools? Or both at once? The README.txt examples call scripts via "python …" and relative paths, which isn't convenient and would only work when entering the top-level Python facct module dir.
Comment 12 Christopher Meng 2013-09-26 02:03:40 EDT
Any news?
Comment 13 Miroslav Suchý 2015-07-21 10:01:59 EDT
Closing due long inactivity. Feel free to reopen if you want to continue.

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