Bug 531544 - Review Request: python-trml2pdf - Tiny RML2PDF is a tool to easily create PDF documents without programming
Review Request: python-trml2pdf - Tiny RML2PDF is a tool to easily create PDF...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Toshio Ernie Kuratomi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-28 13:32 EDT by Cristian Ciupitu
Modified: 2010-12-02 18:49 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-12-02 18:49:06 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
a.badger: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Cristian Ciupitu 2009-10-28 13:32:47 EDT
Spec URL: http://github.com/ciupicri/rpmbuild/blob/master/SPECS/python-trml2pdf.spec
SRPM URL: http://cristian.ciupitu.googlepages.com/python-trml2pdf-1.2-1.fc11.src.rpm
Description: 
Convert Report Markup Language (RML) files to PDF.


This is my first package, so I'll need a sponsor. My FAS account  and IRC nick is ciupicri.
This package will be used by Satchmo which I also plan adding to Fedora as soon as upstream fixes some packaging issues.


$ mock rebuild -r fedora-11-x86_64 /srv/nfs/my-SRPMS/python-trml2pdf-1.2-1.fc11.src.rpm
$ rpmlint /var/lib/mock/fedora-11-x86_64/result/python-trml2pdf-1.2-1.fc11.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.


Here are some links that will prove that I'm familiar with packaging and the guidelines:
https://bugzilla.redhat.com/show_bug.cgi?id=529250
https://bugzilla.redhat.com/show_bug.cgi?id=529188
https://bugzilla.redhat.com/show_bug.cgi?id=522899
https://bugzilla.redhat.com/show_bug.cgi?id=504881
https://bugzilla.redhat.com/show_bug.cgi?id=530724
https://bugzilla.redhat.com/show_bug.cgi?id=530726
https://bugzilla.redhat.com/show_bug.cgi?id=487097#c9
http://code.google.com/p/sorl-thumbnail/issues/detail?id=103
http://code.google.com/p/django-app-plugins/issues/detail?id=13#c2
http://code.google.com/p/django-app-plugins/issues/detail?id=16
https://bugzilla.redhat.com/show_bug.cgi?id=492200
https://bugzilla.redhat.com/show_bug.cgi?id=519440
http://bitbucket.org/chris1610/satchmo/issue/923/use-system-provided-simplejson-where-possible-instead-of
http://bitbucket.org/chris1610/satchmo/issue/919/convert-contributors-to-utf-8-encoding
http://bitbucket.org/chris1610/satchmo/issue/924/some-files-are-executable-even-if-they-arent-supposed-to
http://bitbucket.org/chris1610/satchmo/issue/935/some-files-have-a-misleading-shebang
Comment 1 Matthias Runge 2010-02-09 16:31:33 EST
Since, I'm not approved, I may give you an unofficial review:

* rpmlint python-trml2pdf.spec 
python-trml2pdf.spec: W: no-cleaning-of-buildroot %install
python-trml2pdf.spec: W: no-cleaning-of-buildroot %clean
python-trml2pdf.spec: W: no-%prep-section
python-trml2pdf.spec: W: no-%build-section
python-trml2pdf.spec: W: no-%install-section
python-trml2pdf.spec: W: no-%clean-section
python-trml2pdf.spec: E: specfile-error error: line 8: Unknown tag: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
python-trml2pdf.spec: E: specfile-error error: query of specfile python-trml2pdf.spec failed, can't parse
0 packages and 1 specfiles checked; 2 errors, 6 warnings.

* Name is according to the Naming Guidlines

* License LGPLv2+

* MUST: The License field in the package spec file must match the actual license.
ok

* License.txt included in %doc
ok

(to be continued...)
Comment 2 Cristian Ciupitu 2010-02-10 06:31:28 EST
(In reply to comment #1)
> Since, I'm not approved, I may give you an unofficial review:
> 

Thank for your review, but what rpmlint are you using? I'm using rpmlint-0.91-1.fc12.noarch and it gives me "0 errors, 0 warnings". I think that you should have used the "raw" version of the spec file - http://github.com/ciupicri/rpmbuild/raw/master/SPECS/python-trml2pdf.spec .
Comment 3 Matthias Runge 2010-02-10 10:15:47 EST
I've take the version from 
http://github.com/ciupicri/rpmbuild/blob/master/SPECS/python-trml2pdf.spec
mentioned in the original post. I took the version, wget got me from that URL. Looks like that file is not the intendet spec.

I've taken rpmlint-0.93-2.fc12.noarch
Comment 4 Matthias Runge 2010-02-10 11:36:49 EST
OK, using the "raw" version, rpmlint lists no error, no warning in your spec-file.

To continue the unofficial review:
SPEC-File is in english and legible. (ok)

Package compiles into binary RPM

Build-Requires are ok.

No locale (guess it's ok)

No shared library, so no %post and %postun (ok)

No system library bundled (ok)

%clean removes buildroot (ok) 

%files section ok, file permissions are sane (ok)

No header files, no static libraries (ok)

(to be continued)
Comment 5 Susi Lehtola 2010-07-08 18:28:55 EDT
First of all, as a sponsor I think you need to do more informal reviews. I see you have been already active otherwise, but that does not make up for review experience.

Reviewing is a very important part of being involved in Fedora packaging, and there is no better way to show that you understand the guidelines. As a reminder the most important ones are
 http://fedoraproject.org/wiki/Packaging/Guidelines
 http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
Additionally to the Packaging Guidelines, there are a bunch of language / application specific guidelines that are linked to in the Packaging Guidelines.

Here are some tricks of the trade:
http://fedoraproject.org/wiki/Packaging_tricks
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
http://fedoraproject.org/wiki/Common_Rpmlint_issues


Some comments about the package itself:

- The name is incorrect. This is a tool, not a pure python library, so the name should be just trml2pdf.

- The summary and description conflict. Besides, the description being shorter than the summary is quite nonorthodox. I'd change them both to something like
"A tool to convert Report Markup Language (RML) files to PDF"

- URL is incorrect, it should be something of the sort
http://packages.pardus.org.tr/contrib/source/trml2pdf.html
(it should point to the package homepage, not the directory where the tarball has been taken from)

- You don't need to use "%{__python}", plain "python" will do just fine.

- Don't use wildcards where they are not necessary: from the statement
 %{python_sitelib}/*
it is not clear what actually gets included. In the case of Python this has also practical implications - normally, in addition to the library also an egg is produced. Using the wildcard prevents you from being notified that the egg is missing. Please use more explicit statements such as
 %{python_sitelib}/trml2pdf/
 %{python_sitelib}/trml2pdf-*.egg-info
Comment 6 Cristian Ciupitu 2010-07-08 19:41:21 EDT
First of all, I want to mention that I've updated the RPM in the meantime:
Spec URL:
 http://github.com/ciupicri/rpmbuild/raw/master/SPECS/python-trml2pdf.spec
SRPM URL:
 http://sites.google.com/site/cristianciupitu/python-trml2pdf-1.2-1.fc13.src.rpm

(In reply to comment #5)
> First of all, as a sponsor I think you need to do more informal reviews. I see
> you have been already active otherwise, but that does not make up for review
> experience.
Ok, I'll try to help more with other reviews.

> Reviewing is a very important part of being involved in Fedora packaging, and
> there is no better way to show that you understand the guidelines. As a
> reminder the most important ones are
>  http://fedoraproject.org/wiki/Packaging/Guidelines
>  http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
> Additionally to the Packaging Guidelines, there are a bunch of language /
> application specific guidelines that are linked to in the Packaging Guidelines.
> 
> Here are some tricks of the trade:
> http://fedoraproject.org/wiki/Packaging_tricks
> http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
> http://fedoraproject.org/wiki/Common_Rpmlint_issues
Thanks.

> Some comments about the package itself:
> 
> - The name is incorrect. This is a tool, not a pure python library, so the name
> should be just trml2pdf.
My initial name was trml2pdf, but another Fedora packager suggested me something else:
  <abadger1999> ciupicri: Since it's a dependency, is it the python module or the program that's being used?
  <ciupicri> abadger1999, the python module
  <ciupicri> abadger1999, satchmo (django app) uses its library not its program
  <abadger1999> ciupicri: Ideally, upstream would split the script out into its own file and import the module to make the script run but it sounds like upstream is pretty dead.
  <abadger1999> So I'd do this:
  <abadger1999> link the trml2pdf.py file to %{_bindir}/trml2pdf  (leave off the .py extension in bindir)
  <abadger1999> and name the package python-trml2pdf
 
> - The summary and description conflict. Besides, the description being shorter
> than the summary is quite nonorthodox. I'd change them both to something like
> "A tool to convert Report Markup Language (RML) files to PDF"
The summary was taken from the description field of "setup.py", but I do agree that the current situation is a bit odd and I'll try to use something better.
 
> - URL is incorrect, it should be something of the sort
> http://packages.pardus.org.tr/contrib/source/trml2pdf.html
> (it should point to the package homepage, not the directory where the tarball
> has been taken from)
As far as I know the package is unmaintained, so that why I've used that URL.

> - You don't need to use "%{__python}", plain "python" will do just fine.
I know, but that's what rpmdev-newspec suggests.

> - Don't use wildcards where they are not necessary: from the statement
>  %{python_sitelib}/*
> it is not clear what actually gets included. In the case of Python this has
> also practical implications - normally, in addition to the library also an egg
> is produced. Using the wildcard prevents you from being notified that the egg
> is missing. Please use more explicit statements such as
>  %{python_sitelib}/trml2pdf/
>  %{python_sitelib}/trml2pdf-*.egg-info    
It's nice to have a more explicit list of the included files, but on the other hand I think that it makes things a bit harder to maintain. I've also seen a counter example: http://cvs.fedoraproject.org/viewvc/rpms/python-fedora/F-13/python-fedora.spec?view=markup .
Comment 7 Susi Lehtola 2010-07-09 02:13:08 EDT
(In reply to comment #6)
> > - The name is incorrect. This is a tool, not a pure python library, so the name
> > should be just trml2pdf.
> My initial name was trml2pdf, but another Fedora packager suggested me
> something else:
>   <abadger1999> ciupicri: Since it's a dependency, is it the python module or
> the program that's being used?
>   <ciupicri> abadger1999, the python module
>   <ciupicri> abadger1999, satchmo (django app) uses its library not its program
>   <abadger1999> ciupicri: Ideally, upstream would split the script out into its
> own file and import the module to make the script run but it sounds like
> upstream is pretty dead.
>   <abadger1999> So I'd do this:
>   <abadger1999> link the trml2pdf.py file to %{_bindir}/trml2pdf  (leave off
> the .py extension in bindir)
>   <abadger1999> and name the package python-trml2pdf

Hmm, OK, in that case the name python-trml2pdf is probably OK.

> > - URL is incorrect, it should be something of the sort
> > http://packages.pardus.org.tr/contrib/source/trml2pdf.html
> > (it should point to the package homepage, not the directory where the tarball
> > has been taken from)
> As far as I know the package is unmaintained, so that why I've used that URL.

How do you know that it's unmaintained? You can always ask upstream...

> > Please use more explicit statements such as
> >  %{python_sitelib}/trml2pdf/
> >  %{python_sitelib}/trml2pdf-*.egg-info    
> It's nice to have a more explicit list of the included files, but on the other
> hand I think that it makes things a bit harder to maintain. I've also seen a
> counter example:
> http://cvs.fedoraproject.org/viewvc/rpms/python-fedora/F-13/python-fedora.spec?view=markup
> .    

Well, that's really just a sad excuse :) And maintaining is not complicated at all, you just have to write a few more characters initially..
Comment 8 Cristian Ciupitu 2010-11-04 12:35:17 EDT
(In reply to comment #7)
> (In reply to comment #6)
> > > - URL is incorrect, it should be something of the sort
> > > http://packages.pardus.org.tr/contrib/source/trml2pdf.html
> > > (it should point to the package homepage, not the directory where the tarball
> > > has been taken from)
> > As far as I know the package is unmaintained, so that why I've used that URL.
> 
> How do you know that it's unmaintained? You can always ask upstream...

I tried to contact upstream and I got no answer.


The latest versions can be found here:
- http://github.com/ciupicri/rpmbuild/raw/master/SPECS/python-trml2pdf.spec
- http://sites.google.com/site/cristianciupitu/python-trml2pdf-1.2-1.fc14.src.rpm
Comment 9 Toshio Ernie Kuratomi 2010-11-07 16:54:11 EST
Please up the Release number whenever you make a new spec file.

Good:
* Package named appropriately
* License file is included
* specfile is readable
* tarball matches what's on satchmo but see the information in the Notes section.
* Builds in koji
* No locales to handle
* No shared libraries
* No bundled libraries
* Not relocatable
* Package owns all directories it creates
* No files listed multiple times
* Permissions set properly
* Packages used consistently
* Code not content
* Nothing in %doc is used at runtime
* No GUI
* All filenames are valid UTF-8


Needswork:
* License is LGPLv2+.  Please update the spec file to reflect that.

Note:
* Removing the python_sitelib definition is fine as long as you are not
  planning on building on older releases.  If you do want to build this on F13
  or less or EL-4 or EL-5 you will need to add it back in.
* Upstream seems dead as you note but there's several projects that have copies:
  - satchmo (where you got the tarball from) http://www.satchmoproject.com/snapshots/
  - kraft: http://sourceforge.net/projects/kraft/files/
  - template2pdf: http://code.google.com/p/template2pdf/

  It would be very nice if you could contact these groups and get them to
  revive the project in some shape or form.  For instance, everyone could
  contirbute to the template2pdf project and make that module the new upstream

  Currently the tarballs for these projects do not match but the differences
  are minor and not in the code.  satchmo has a setup.py file, kraft is saved
  as tar.bz2, template2pdf has bundled the module in their code.
* Realize that currently, as there is no upstream for this package, you are
  putting yourself on the hook for fixing anything wrong in the code, bugs,
  security issues, etc.  This can be alleviated if you get those other projects
  to help out in creating a new canonical upstream.

rpmlint:

python-trml2pdf.src: W: no-cleaning-of-buildroot %clean
python-trml2pdf.src: W: no-%clean-section

Both of these are something that's needed in EPEL builds but not F-14.  If
you're not planning to build on EPEL-5 then you don't need to worry about this.

python-trml2pdf.noarch: W: no-manual-page-for-binary trml2pdf

This is not a requirement but if you want to write a man page for the script,
it would be helpful for people looking to run it.  It doesn't look like Debian
has one so we'd have to write one rather than copy it.  This is not a blocker.

3 packages and 0 specfiles checked; 0 errors, 5 warnings.

Summary:

So fixing the licensing issue is the only blocker to the package.  Do take the things in the Note section under consideration.

I also don't see any reviews from you.  There's quite a few python-* reviews listed here: 
http://fedoraproject.org/PackageReviewStatus/NEW.html

I know that the maintainer of the python-zope-* packages is quite active if you want a relatively quick turnaround time.  If you'll just choose one review and let me know what you're doing, I'll check your review and then sponsor you so you can officially complete it.
Comment 10 Cristian Ciupitu 2010-11-07 18:02:47 EST
(In reply to comment #9)

> Needswork:
> * License is LGPLv2+.  Please update the spec file to reflect that.

I didn't see any mention of "or later", so I used LGPLv2 to be safe.
I'll address the rest of the issues later.
Comment 11 Toshio Ernie Kuratomi 2010-11-08 09:29:19 EST
I usually do a grep -ri license in the toplevel directory and then take a quick look at files that have matches (or just the matched lines) to see what we have.  The grep finds the standard "version 2.1 of the License, or (at your option) any later version." in all of the source files.
Comment 12 Cristian Ciupitu 2010-11-08 11:57:18 EST
(In reply to comment #9)
> Please up the Release number whenever you make a new spec file.

I thought that this doesn't make too much sense when the SPEC is still work in progress.


> Needswork:
> * License is LGPLv2+.  Please update the spec file to reflect that.

Fixed.

> Note:
> * Removing the python_sitelib definition is fine as long as you are not
>   planning on building on older releases.  If you do want to build this on F13
>   or less or EL-4 or EL-5 you will need to add it back in.

I'm aiming for Fedora 14. EL4 is out of the question because as far as I know it offers Python 2.3 which is too old. EL5 might be an option, given the fact that Satchmo is supposed to work on Python 2.4, but there were some hiccups[1] which required Python 2.5. Also I don't use EL5, so I don't know if I could provide support for it.

> * Upstream seems dead as you note but there's several projects that have
> copies:
>   - satchmo (where you got the tarball from)
> http://www.satchmoproject.com/snapshots/
>   - kraft: http://sourceforge.net/projects/kraft/files/
>   - template2pdf: http://code.google.com/p/template2pdf/
> 
>   It would be very nice if you could contact these groups and get them to
>   revive the project in some shape or form.  For instance, everyone could
>   contirbute to the template2pdf project and make that module the new upstream
> 
>   Currently the tarballs for these projects do not match but the differences
>   are minor and not in the code.  satchmo has a setup.py file, kraft is saved
>   as tar.bz2, template2pdf has bundled the module in their code.

I'm packing trml2pdf as a dependency for Satchmo, so this is my upstream at least for now.

> * Realize that currently, as there is no upstream for this package, you are
>   putting yourself on the hook for fixing anything wrong in the code, bugs,
>   security issues, etc.  This can be alleviated if you get those other projects
>   to help out in creating a new canonical upstream.
 
I'm counting on the Satchmo developers for fixes, but given my Python skills I could fix some minor bugs myself. Anyway, creating a new canonical upstream is an interesting idea. By the way, Satchmo tried to replace trml2pdf with something else, but without any luck[2].

> rpmlint:
> 
> python-trml2pdf.src: W: no-cleaning-of-buildroot %clean
> python-trml2pdf.src: W: no-%clean-section
> 
> Both of these are something that's needed in EPEL builds but not F-14.  If
> you're not planning to build on EPEL-5 then you don't need to worry about this.

See above.

> python-trml2pdf.noarch: W: no-manual-page-for-binary trml2pdf
> 
> This is not a requirement but if you want to write a man page for the script,
> it would be helpful for people looking to run it.  It doesn't look like Debian
> has one so we'd have to write one rather than copy it.  This is not a blocker.

I have not used the script myself nor have I ever written a man page, but I could give it try. Regarding Debian, I think that it does have a man page[3]. 

> Summary:
> 
> So fixing the licensing issue is the only blocker to the package.  Do take the
> things in the Note section under consideration.

I'll update the SPEC and the SRC.rpm in a couple of hours.

> I also don't see any reviews from you.  There's quite a few python-* reviews
> listed here: 
> http://fedoraproject.org/PackageReviewStatus/NEW.html
> 
> I know that the maintainer of the python-zope-* packages is quite active if you
> want a relatively quick turnaround time.  If you'll just choose one review and
> let me know what you're doing, I'll check your review and then sponsor you so
> you can officially complete it.

I'll try to be more active. I've already looked at 2 requests:
- https://bugzilla.redhat.com/show_bug.cgi?id=547621#c1
- https://bugzilla.redhat.com/show_bug.cgi?id=578024#c33


[1] http://bitbucket.org/chris1610/satchmo/changeset/98c41ad27249
[2] http://bitbucket.org/chris1610/satchmo/wiki/PdfCreation
[3] http://svn.debian.org/viewsvn/python-modules/packages/python-trml2pdf/trunk/debian/trml2pdf.1?view=markup
Comment 13 Toshio Ernie Kuratomi 2010-11-08 12:42:52 EST
(In reply to comment #12)
> (In reply to comment #9)
> > Please up the Release number whenever you make a new spec file.
> 
> I thought that this doesn't make too much sense when the SPEC is still work in
> progress.
> 
It does two things:  It lets reviewers and packagers refer to a specific revision like 'I thought I fixed the %prep issue in 1.0-3'

It shows that you know what you know what to do when updating the release.  We've had people that bumped epoch with every new build or people who updated the release numbers in a way that went backwards instead of forwards so it's good to check this.

> 
> I'm aiming for Fedora 14.

Sounds good.
> Anyway, creating a new canonical upstream is
> an interesting idea. By the way, Satchmo tried to replace trml2pdf with
> something else, but without any luck[2].
>
<nod>  Getting everybody to work on something together will help everyone in the long run.  In certain development communities *cough*java*cough* we've seen projects fork dead code and then, because the code is useful to more than one project, each project has its own fork which diverges in small, subtle ways from all the others.  This makes for a maintenance nightmare.

> 
> I have not used the script myself nor have I ever written a man page, but I
> could give it try. Regarding Debian, I think that it does have a man page[3]. 
>
Ah excellent!  You can just copy the man page from Debian then.
> 
> I'll try to be more active. I've already looked at 2 requests:
> - https://bugzilla.redhat.com/show_bug.cgi?id=547621#c1
> - https://bugzilla.redhat.com/show_bug.cgi?id=578024#c33
> 
Excellent.
Comment 14 Cristian Ciupitu 2010-11-08 22:52:09 EST
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > Please up the Release number whenever you make a new spec file.
> > 
> > I thought that this doesn't make too much sense when the SPEC is still work in
> > progress.
> > 
> It does two things:  It lets reviewers and packagers refer to a specific
> revision like 'I thought I fixed the %prep issue in 1.0-3'
Now I regret squashing git commits :-)

> It shows that you know what you know what to do when updating the release. 
> We've had people that bumped epoch with every new build or people who updated
> the release numbers in a way that went backwards instead of forwards so it's
> good to check this.
I guess I could consider it some kind of exercise that proves my packaging skills.

> > Anyway, creating a new canonical upstream is
> > an interesting idea. By the way, Satchmo tried to replace trml2pdf with
> > something else, but without any luck[2].
> >
> <nod>  Getting everybody to work on something together will help everyone in
> the long run.  In certain development communities *cough*java*cough* we've seen
> projects fork dead code and then, because the code is useful to more than one
> project, each project has its own fork which diverges in small, subtle ways
> from all the others.  This makes for a maintenance nightmare.
I've found another email address of the author and I've sent him a message regarding the maintenance of the project. I should, hopefully, get an answer in a couple of days. If not, I could ask the projects using it to join forces (as you've already suggested).

The latest files are here:
- https://github.com/ciupicri/rpmbuild/blob/master/SPECS/python-trml2pdf.spec
- https://sites.google.com/site/cristianciupitu/python-trml2pdf-1.2-2.fc14.src.rpm

I'll look at other review requests tomorrow.
Comment 15 Toshio Ernie Kuratomi 2010-12-01 01:03:16 EST
All problems with this package resolved and I've sponsored you since your python-genshi06 review shows that you're knowledgable enough to be a packager.

This package is APPROVED.

You're now on step four of: https://fedoraproject.org/wiki/Package_Review_Process

Requesting that the package have a branch created in our SCM: https://fedoraproject.org/wiki/Package_SCM_admin_requests
Comment 16 Cristian Ciupitu 2010-12-01 07:41:37 EST
New Package SCM Request
=======================
Package Name: python-trml2pdf
Short Description: Tiny RML2PDF is a tool to easily create PDF documents without programming
Owners: ciupicri
Branches: f14
InitialCC:
Comment 17 Jason Tibbitts 2010-12-02 14:32:47 EST
Git done (by process-git-requests).

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