Bug 1306258 - Review Request: python-Lektor - A static content management system
Summary: Review Request: python-Lektor - A static content management system
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Tomas Orsava
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-02-10 12:32 UTC by Charalampos Stratakis
Modified: 2016-03-01 11:27 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-03-01 11:26:55 UTC
Type: ---
Embargoed:
torsava: fedora-review+


Attachments (Terms of Use)

Description Charalampos Stratakis 2016-02-10 12:32:59 UTC
Spec URL: http://copr-dist-git.fedorainfracloud.org/cgit/cstratak/python-Lektor/python-Lektor.git/plain/python-Lektor.spec

SRPM URL: https://copr-be.cloud.fedoraproject.org/results/cstratak/python-Lektor/fedora-rawhide-x86_64/00159006-python-Lektor/python-Lektor-1.1-1.fc24.src.rpm

Description: Lektor is a static website generator. It builds out an entire project from static files into many individual HTML pages and has a built-in admin UI and minimal desktop app.

Fedora Account System Username: cstratak

Comment 1 Tomas Orsava 2016-02-23 12:51:32 UTC
Hi, here's the review (my first!):

- change URL to point to the upstream home page instead of pypi
- suggestion: include PKG-INFO as a %doc
- suggestion: set permissions on LICENSE file to 644 instead of the current 664
- fix rpmlint warning: python2-Lektor.noarch: W: hidden-file-or-dir /usr/lib/python2.7/site-packages/lektor/quickstart-templates/plugin/.gitignore.in (I suggest deleting the file before %install)

[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/python2.7/site-
     packages/lektor
- change line "{python2_sitelib}/lektor/*" to "{python2_sitelib}/lektor"

[ ]: Changelog in prescribed format.
- format is "Firstname Lastname", you have them reversed

[ ]: Requires correct, justified where necessary.
- add requires on Jinja2>=2.4 as per requires.txt

[ ]: Latest version is packaged.
- version 1.1 is in spec file, 1.2.1 is available upstream

[x]: Reviewer should test that the package builds in mock.
- builds in rawhide, but fails: pkg_resources.DistributionNotFound: The 'argh>=0.24.1' distribution was not found and is required by watchdog

Comment 2 Bohuslav "Slavek" Kabrda 2016-02-23 15:02:57 UTC
You probably don't want to mark PKG-INFO as %doc. Since it's in the .egg-info directory, some other software may use it for processing metadata about the package. Thus if someone installed the package without docs (`rpm -i --excludedocs`), the package might appear broken.

Comment 4 Tomas Orsava 2016-02-23 16:02:55 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #2)
> You probably don't want to mark PKG-INFO as %doc. Since it's in the
> .egg-info directory, some other software may use it for processing metadata
> about the package. Thus if someone installed the package without docs (`rpm
> -i --excludedocs`), the package might appear broken.

There are two PKG-INFO files, one in the .egg-info directory, and one in the root directory. I believe the second one can (and should?) be included in %doc.

Comment 6 Bohuslav "Slavek" Kabrda 2016-02-24 08:00:52 UTC
(In reply to T. Orsava from comment #4)
> (In reply to Bohuslav "Slavek" Kabrda from comment #2)
> > You probably don't want to mark PKG-INFO as %doc. Since it's in the
> > .egg-info directory, some other software may use it for processing metadata
> > about the package. Thus if someone installed the package without docs (`rpm
> > -i --excludedocs`), the package might appear broken.
> 
> There are two PKG-INFO files, one in the .egg-info directory, and one in the
> root directory. I believe the second one can (and should?) be included in
> %doc.

Ah, this one. Sorry, that can be marked as %doc. I'm not sure if it should, though. It seems that most of the information it provides is already available in RPM metadata and some other info will probably not be that interesting to normal users. Since there'll be PKG-INFO in the package anyway, I'd probably not include this one.
I think this is a matter of taste, so I'll let you do the final decision and just leave my "-0" here.

Comment 7 Tomas Orsava 2016-02-24 08:25:15 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #6)
> (In reply to T. Orsava from comment #4)
> > (In reply to Bohuslav "Slavek" Kabrda from comment #2)
> > > You probably don't want to mark PKG-INFO as %doc. Since it's in the
> > > .egg-info directory, some other software may use it for processing metadata
> > > about the package. Thus if someone installed the package without docs (`rpm
> > > -i --excludedocs`), the package might appear broken.
> > 
> > There are two PKG-INFO files, one in the .egg-info directory, and one in the
> > root directory. I believe the second one can (and should?) be included in
> > %doc.
> 
> Ah, this one. Sorry, that can be marked as %doc. I'm not sure if it should,
> though. It seems that most of the information it provides is already
> available in RPM metadata and some other info will probably not be that
> interesting to normal users. Since there'll be PKG-INFO in the package
> anyway, I'd probably not include this one.
> I think this is a matter of taste, so I'll let you do the final decision and
> just leave my "-0" here.

Ok in that case my apologies to Charalampos for suggesting he makes it so.

Also for future reference, we found out that the ".gitignore.in" file pointed out by a lint warning should NOT be tampered with as it is actually integral to the working of the package.

Comment 8 Tomas Orsava 2016-02-24 09:20:14 UTC
(In reply to Charalampos Stratakis from comment #5)
> SPEC URL:
> http://copr-dist-git.fedorainfracloud.org/cgit/cstratak/Lektor/Lektor.git/
> plain/Lektor.spec
> 
> SRPM URL:
> https://copr-be.cloud.fedoraproject.org/results/cstratak/Lektor/fedora-
> rawhide-x86_64/00162504-Lektor/Lektor-1.2.1-1.fc24.src.rpm

Looks almost perfect, just one small thing: You should rename the base package to python-Lektor instead of just Lektor if you're going to be providing the python2-Lektor subpackage.

Comment 10 Robert Kuska 2016-02-24 09:37:13 UTC
There is no need for providing python2-Lektor subpackage, Lektor is an application as I understand and there is no library kind of usage, thus it should be named just Lektor and shipped as Lektor.

Comment 11 Charalampos Stratakis 2016-02-24 09:46:48 UTC
(In reply to Robert Kuska from comment #10)
> There is no need for providing python2-Lektor subpackage, Lektor is an
> application as I understand and there is no library kind of usage, thus it
> should be named just Lektor and shipped as Lektor.

It is possible to create python plugins for Lektor [0]. So shouldn't it be shipped as python-Lektor?

[0]https://www.getlektor.com/docs/plugins/dev/

Comment 12 Tomas Orsava 2016-02-24 09:58:50 UTC
(In reply to Charalampos Stratakis from comment #11)
> (In reply to Robert Kuska from comment #10)
> > There is no need for providing python2-Lektor subpackage, Lektor is an
> > application as I understand and there is no library kind of usage, thus it
> > should be named just Lektor and shipped as Lektor.
> 
> It is possible to create python plugins for Lektor [0]. So shouldn't it be
> shipped as python-Lektor?
> 
> [0]https://www.getlektor.com/docs/plugins/dev/

I agree with Charalampos. Lektor modules are being imported into third party python code (plugins), therefore I believe it should provide a python2 subpackage.

Comment 13 Robert Kuska 2016-02-24 10:01:57 UTC
In that case you are right and it is probably safer to name it python-Lektor (btw, sorry for renaming the summary, that was done automatically by bugzilla because of some conflicts)

Comment 15 Gwyn Ciesla 2016-02-24 13:34:44 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/python-Lektor


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