Bug 1124994 - Review Request: dl - Download Ticket Service
Summary: Review Request: dl - Download Ticket Service
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Greg Bailey
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-07-30 19:29 UTC by Greg Bailey
Modified: 2014-09-08 20:14 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-08-01 20:42:21 UTC
herrold: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
clearsigned sha256sun of the sources (323 bytes, text/plain)
2014-08-01 15:03 UTC, R P Herrold
no flags Details
patch, adding a conditional CLI sub-package (2.94 KB, patch)
2014-08-01 18:54 UTC, R P Herrold
no flags Details | Diff

Description Greg Bailey 2014-07-30 19:29:42 UTC
Spec URL: https://gbailey.fedorapeople.org/dl/0.12-1/dl.spec
SRPM URL: https://gbailey.fedorapeople.org/dl/0.12-1/dl-0.12-1.fc20.src.rpm

Description:

dl is a file exchange service that allows you to upload any file to a web
server and generate a unique ticket for others to download. The ticket is
automatically expired according to the specified rules, so that you don't need
to keep track or cleanup afterward. dl also allows you to grant an anonymous,
one-time upload for others to send *you* a file, without the requirement of
account management.

dl is usually installed as a "email attachments replacement" due to its
simplicity (though can be used in other ways).

Fedora Account System Username: gbailey

koji rawhide:
http://koji.fedoraproject.org/koji/taskinfo?taskID=7217000

Comment 1 Greg Bailey 2014-07-31 06:52:45 UTC
Fixed some rpmlint errors/warnings.

Spec URL: https://gbailey.fedorapeople.org/dl/0.12-2/dl.spec
SRPM URL: https://gbailey.fedorapeople.org/dl/0.12-2/dl-0.12-2.fc20.src.rpm

The only error shown by rpmlint is:

Checking: dl-0.12-2.fc20.noarch.rpm
          dl-0.12-2.fc20.src.rpm
dl.noarch: E: non-standard-dir-perm /var/spool/dl 0700L
2 packages and 0 specfiles checked; 1 errors, 0 warnings.

/var/spool/dl should be visible only to the webserver, so the "standard" directory permissions of 0755 don't make sense, IMHO.

Comment 2 Greg Bailey 2014-07-31 16:34:11 UTC
Updated for DL 0.13, released today.

Spec URL: https://gbailey.fedorapeople.org/dl/0.13-1/dl.spec
SRPM URL: https://gbailey.fedorapeople.org/dl/0.13-1/dl-0.13-1.fc20.src.rpm

Comment 3 R P Herrold 2014-07-31 17:02:46 UTC
[herrold@centos-6 dl]$ rpmlint /home/herrold/rpmbuild/SRPMS/dl-0.13-1.orc6.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[herrold@centos-6 dl]$ rpmlint /home/herrold/rpmbuild/RPMS/noarch/dl-0.13-1.orc6.noarch.rpm
dl.noarch: W: incoherent-version-in-changelog 0.13-1 ['0.13-1.orc6', '0.13-1.orc6']
dl.noarch: E: non-standard-dir-perm /var/spool/dl 0700L
1 packages and 0 specfiles checked; 1 errors, 1 warnings.


The changelog version issue is probably a local issue as we carry a custom %dist tag

a '700' does not seem 'non-standard' to me
http://www.redhat.com/archives/fedora-packaging/2008-September/msg00035.html
https://lintian.debian.org/tags/non-standard-dir-perm.html

there are potentially private matter in the existing case, so constraining access seems reasonable, and so a valid exception in the context of the program's services

so: rpmlint PASS

Comment 4 R P Herrold 2014-07-31 17:04:40 UTC
naming is acceptable (albeit short, but that matches upstream)

so: naming PASS

Comment 5 R P Herrold 2014-07-31 17:05:39 UTC
spec file name matches package name

so: specfile name PASS

Comment 6 R P Herrold 2014-07-31 17:09:39 UTC
license contained in COPYING

GPLv2+

but noted as: 
License: GPLv2 in the .spec file

Greg, could you please update this?

Comment 7 Greg Bailey 2014-08-01 13:09:30 UTC
License is GPLv2+ according to AUTHORS.rst

Spec URL: https://gbailey.fedorapeople.org/dl/0.13-2/dl.spec
SRPM URL: https://gbailey.fedorapeople.org/dl/0.13-2/dl-0.13-2.fc20.src.rpm

Comment 8 Greg Bailey 2014-08-01 14:04:04 UTC
Unbundle system provided php-phpass and php-php-gettext

rpmlint now shows:

Checking: dl-0.13-3.fc20.noarch.rpm
          dl-0.13-3.fc20.src.rpm
dl.noarch: W: dangling-symlink /usr/share/dl/include/gettext /usr/share/php/gettext
dl.noarch: W: dangling-symlink /usr/share/dl/include/PasswordHash.php /usr/share/php/phpass/PasswordHash.php
dl.noarch: E: non-standard-dir-perm /var/spool/dl 0700L
2 packages and 0 specfiles checked; 1 errors, 2 warnings.

I think the 2 new dangling symlink warnings can be ignored, as these symlinks point to the system provided (and required) php-phpass and php-php-gettext files.

Spec URL: https://gbailey.fedorapeople.org/dl/0.13-3/dl.spec
SRPM URL: https://gbailey.fedorapeople.org/dl/0.13-3/dl-0.13-3.fc20.src.rpm

Comment 9 R P Herrold 2014-08-01 14:44:40 UTC
as to comment 8, (symlinks) manual 'Requires' for php-phpass and php-php-gettext, to ensure they are present may be sufficient -- and I note the .spec file contains such

ok as to this .. perhaps it could be a '%ghost' type member in the %files section

see example: http://fedoraproject.org/wiki/PackagingDrafts/Logfiles , 
http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-files-list-directives.html#S3-RPM-INSIDE-FLIST-GHOST-DIRECTIVE

just a thought

Comment 10 R P Herrold 2014-08-01 14:52:06 UTC
[herrold@centos-6 dl]$ diff -u dl.spec-3 dl.spec-4
--- dl.spec-3   2014-08-01 10:51:41.452383001 -0400
+++ dl.spec-4   2014-08-01 10:50:28.542383001 -0400
@@ -5,7 +5,7 @@
 Name:       dl
 Version:    0.13
 Group:      Applications/Internet
-Release:    3%{?dist}
+Release:    4%{?dist}
 License:    GPLv2+

 Source0:    http://www.thregr.org/~wavexx/software/dl/releases/dl-%{version}.zip
@@ -114,8 +114,12 @@
 %{_datadir}/dl
 %dir %attr(0700,apache,apache) %{_localstatedir}/spool/dl
 %dir %attr(0755,apache,apache) %{_localstatedir}/spool/dl/data
+%ghost %{_datadir}/dl/include/gettext

 %changelog
+* Fri Aug  1 2014 R P Herrold <info@owlriver.com> - 0.13-4
+- % ghost gettext link
+
 * Fri Aug  1 2014 Greg Bailey <gbailey@lxpro.com> - 0.13-3
 - Unbundle php-phpass and php-php-gettext

[herrold@centos-6 dl]$

Comment 11 R P Herrold 2014-08-01 14:53:16 UTC
not that this works ... wonder how to fix this

[herrold@centos-6 dl]$ rpmlint -V ; rpmlint /home/herrold/rpmbuild/SRPMS/dl-0.13-4.orc6.src.rpm /home/herrold/rpmbuild/RPMS/noarch/dl-0.13-4.orc6.noarch.rpm
rpmlint version 1.5 Copyright (C) 1999-2007 Frederic Lepied, Mandriva
dl.noarch: W: dangling-symlink /usr/share/dl/include/gettext /usr/share/php/gettext
dl.noarch: W: dangling-symlink /usr/share/dl/include/PasswordHash.php /usr/share/php/phpass/PasswordHash.php
dl.noarch: E: non-standard-dir-perm /var/spool/dl 0700L
2 packages and 0 specfiles checked; 1 errors, 2 warnings.
[herrold@centos-6 dl]$


Greg -- I also see a warning you do not get ---  is your rpmlint current?

Comment 12 R P Herrold 2014-08-01 14:55:04 UTC
next review item: License file in %doc

%doc COPYING

PASS

Comment 13 R P Herrold 2014-08-01 14:55:41 UTC
6 Spec file in American English

sight reviewed -- 

PASS

Comment 14 R P Herrold 2014-08-01 14:56:09 UTC
7. 'legible' spec file

sight reviewed -- nice and clean

PASS

Comment 15 R P Herrold 2014-08-01 15:01:57 UTC
sha256 verfication

no upstream sums, signed available

6ee2813b39b038624c7c9ae23d4adb62a2d4383dbe71594fdcdf407fe6fa37b7  dl-0.13.zip

inspected:
   http://www.thregr.org/~wavexx/software/dl/releases/
and webpage per URL

PASS

attaching: README.dl-0.13.zip.txt.asc which I clearsign

Comment 16 Greg Bailey 2014-08-01 15:02:30 UTC
(In reply to R P Herrold from comment #11)
> not that this works ... wonder how to fix this
> 

My opinion is that this is a valid warning to consider, which we did, so we can move on...

> Greg -- I also see a warning you do not get ---  is your rpmlint current?

[gbailey@localhost ~]$ rpm -q rpmlint
rpmlint-1.5-12.fc20.noarch

I see the same results as you do: 2 errors and 1 warning (comparing comment #8 and comment #11)

Comment 17 R P Herrold 2014-08-01 15:03:37 UTC
Created attachment 923333 [details]
clearsigned sha256sun of the sources

Comment 18 R P Herrold 2014-08-01 15:04:26 UTC
9. SRPM must build a binary

PASS (we were, after all able to run rpmlint against it

Comment 19 R P Herrold 2014-08-01 15:05:12 UTC
10. notation as to non-working arches

this yields a noarch file, so not applicable

PASS

Comment 20 R P Herrold 2014-08-01 15:05:41 UTC
11. call out all BR's save standard build environment members

no unusual requirements

PASS

Comment 21 R P Herrold 2014-08-01 15:07:35 UTC
12. locale handling

[herrold@centos-6 dl]$ grep locale dl.spec-3                                    
[herrold@centos-6 dl]$ rpm -qlp /home/herrold/rpmbuild/RPMS/noarch/dl-0.13-3.orc6.noarch.rpm | grep locale                                                      
/usr/share/dl/include/locale                                                    
/usr/share/dl/include/locale/cs_CZ                                              
/usr/share/dl/include/locale/cs_CZ/LC_MESSAGES                                  
/usr/share/dl/include/locale/cs_CZ/LC_MESSAGES/messages.mo                      
/usr/share/dl/include/locale/cs_CZ/LC_MESSAGES/messages.po                      
/usr/share/dl/include/locale/de_DE                                              
/usr/share/dl/include/locale/de_DE/LC_MESSAGES                                  
/usr/share/dl/include/locale/de_DE/LC_MESSAGES/messages.mo                      
/usr/share/dl/include/locale/de_DE/LC_MESSAGES/messages.po                      
/usr/share/dl/include/locale/es_ES                                              
/usr/share/dl/include/locale/es_ES/LC_MESSAGES                                  
/usr/share/dl/include/locale/es_ES/LC_MESSAGES/messages.mo                      
/usr/share/dl/include/locale/es_ES/LC_MESSAGES/messages.po
/usr/share/dl/include/locale/fr_FR
/usr/share/dl/include/locale/fr_FR/LC_MESSAGES
/usr/share/dl/include/locale/fr_FR/LC_MESSAGES/messages.mo
/usr/share/dl/include/locale/fr_FR/LC_MESSAGES/messages.po
/usr/share/dl/include/locale/it_IT
/usr/share/dl/include/locale/it_IT/LC_MESSAGES
/usr/share/dl/include/locale/it_IT/LC_MESSAGES/messages.mo
/usr/share/dl/include/locale/it_IT/LC_MESSAGES/messages.po
/usr/share/dl/include/locale/pt_BR
/usr/share/dl/include/locale/pt_BR/LC_MESSAGES
/usr/share/dl/include/locale/pt_BR/LC_MESSAGES/messages.mo
/usr/share/dl/include/locale/pt_BR/LC_MESSAGES/messages.po
/usr/share/doc/dl-0.13/client/thunderbird-filelink-dl/chrome/locale
/usr/share/doc/dl-0.13/client/thunderbird-filelink-dl/chrome/locale/en-US
/usr/share/doc/dl-0.13/client/thunderbird-filelink-dl/chrome/locale/en-US/compose.dtd
/usr/share/doc/dl-0.13/client/thunderbird-filelink-dl/chrome/locale/en-US/compose.properties
/usr/share/doc/dl-0.13/client/thunderbird-filelink-dl/chrome/locale/en-US/management.dtd
/usr/share/doc/dl-0.13/client/thunderbird-filelink-dl/chrome/locale/en-US/settings.dtd
[herrold@centos-6 dl]$


so seems to comply with the requirement

PASS

Comment 22 R P Herrold 2014-08-01 15:08:21 UTC
13. library handling

[herrold@centos-6 dl]$ rpm -qlp /home/herrold/rpmbuild/RPMS/noarch/dl-0.13-3.orc6.noarch.rpm | grep lib
[herrold@centos-6 dl]$


so not applicable

PASS

Comment 23 R P Herrold 2014-08-01 15:09:31 UTC
15. Reloactable / Prefix special casing

[herrold@centos-6 dl]$ grep -i prefix dl.spec-3
[herrold@centos-6 dl]$


not applicable

PASS

Comment 24 R P Herrold 2014-08-01 15:10:36 UTC
16. no bundled system libraries

no libraries at all 

also php module was unbundled earlier

PASS

Comment 25 R P Herrold 2014-08-01 15:11:22 UTC
17. if relocatable, justify special handling

as above - -not applicable 

PASS

Comment 26 R P Herrold 2014-08-01 15:16:37 UTC
ACTION ITEM

18. A package must own all directories that it creates

[herrold@centos-6 dl]$ grep -i mkdir dl.spec-3
mkdir -p ${RPM_BUILD_ROOT}%{_datadir}/dl
mkdir -p ${RPM_BUILD_ROOT}%{_sysconfdir}/dl
mkdir -p ${RPM_BUILD_ROOT}%{_sysconfdir}/httpd/conf.d
mkdir -p ${RPM_BUILD_ROOT}%{_localstatedir}/spool/dl
mkdir -p ${RPM_BUILD_ROOT}%{_localstatedir}/spool/dl/data
[herrold@centos-6 dl]$ grep -i install dl.spec-3
dl is usually installed as a "email attachments replacement" due to its
%install
# selinux: cleanup after uninstall
[herrold@centos-6 dl]$


Greg -- it seems that a:
Requires: httpd 
   ... or 'webserver'
and removal of that mkdir for %{_sysconfdir}/httpd/conf.d

is mandated here ...

https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#FileAndDirectoryOwnership

Comment 27 Greg Bailey 2014-08-01 15:21:02 UTC
(In reply to R P Herrold from comment #26)
> ACTION ITEM
> 
> 18. A package must own all directories that it creates
> 
> Greg -- it seems that a:
> Requires: httpd 
>    ... or 'webserver'
> and removal of that mkdir for %{_sysconfdir}/httpd/conf.d
> 
> is mandated here ...

There's already a "Requires: webserver" line in the spec file.

The directory "/etc/httpd/conf.d" is owned by the httpd package, so I believe that satisfies the requirement of not having unowned directories.

Comment 28 R P Herrold 2014-08-01 15:54:19 UTC
fair enough -- 18. A package must own

PASS

Comment 29 R P Herrold 2014-08-01 15:56:15 UTC
MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the 

rpnbuild flags these, and is not doing so here, so

PASS

Comment 30 R P Herrold 2014-08-01 15:57:11 UTC
20. Permissions on files must be set properly

rpmlint flags deviations, -- we have the one known and excepted deviation for privacy purposes

PASS

Comment 31 R P Herrold 2014-08-01 15:58:39 UTC
21. consistently use packaging macros

spec file sight reviewed and clean per prior steps

no variations from consistency noted

PASS

Comment 32 R P Herrold 2014-08-01 16:00:33 UTC
22. The package must contain code, or permissable content

'permissable' so spelled in the wiki -- hmmm

it does -- this is a CGI script add on, well licensed for software freedom

PASS

Comment 33 R P Herrold 2014-08-01 16:04:53 UTC
typo reported for fix

https://bugzilla.redhat.com/show_bug.cgi?id=1126029

Comment 34 R P Herrold 2014-08-01 16:05:38 UTC
23. Large documentation files must go in a -doc subpackage

inapplicable

PASS

Comment 35 R P Herrold 2014-08-01 16:06:23 UTC
24. %doc content must not affect the runtime

it does not

PASS

Comment 36 R P Herrold 2014-08-01 16:09:14 UTC
QUERY

actually as to # 24 it would appear that sub-packages for the CLI client, and for Thunderbird are appropriate

/usr/share/doc/dl-0.13/client/dl-cli.py                                         
/usr/share/doc/dl-0.13/client/dl-wx/dl-cli.py                                   
/usr/share/doc/dl-0.13/client/dl-wx/dl-icon.ico                                 
/usr/share/doc/dl-0.13/client/dl-wx/dl-wx.py                                    
/usr/share/doc/dl-0.13/client/dl-wx/dl.py                                       
/usr/share/doc/dl-0.13/client/dl-wx/newticket.xrc                               
/usr/share/doc/dl-0.13/client/dl-wx/preferences.xrc                             
/usr/share/doc/dl-0.13/client/dl-wx/upload.xrc   

Greg -- what think you here?

-- Russ

Comment 37 Greg Bailey 2014-08-01 17:12:19 UTC
(In reply to R P Herrold from comment #36)
> QUERY
> 
> actually as to # 24 it would appear that sub-packages for the CLI client,
> and for Thunderbird are appropriate
> 
> Greg -- what think you here?

I considered it.  My reasons for leaving those clients as documentation for now were that:

- I don't (yet) have much experience with them, how they're configured, etc., and didn't want that to delay getting the primary web application (which I suspect is what most people use) packaged.

- The primary target I'm personally interested in is RHEL 6 / CentOS 6 (EPEL), and the documentation (README.rst) states that those clients require Python 2.7, which would force me to go down the road of using SCL, which I'd like to avoid.

I think we could add subpackages for the CLI, the wx-based client, and the thunderbird addon in future revisions if there's sufficient interest.

Comment 38 R P Herrold 2014-08-01 18:53:34 UTC
as to splitting out; * nod *

The alternative answer would be to add a packaging conditional and skip around the sub-packages when 2.7 is not in the 'stock' environment

I see this, reviewing me .spec file collection

https://fedoraproject.org/wiki/Features/Python_2.7/MassRebuild

# Python major version.
%{expand: %%define pyver %(python -c 'import sys;print(sys.version[0:3])')}

also seen: 
%define pyver %(python -c 'import sys ; print sys.version[:3]')

%global python_ver %(%{__python} -c "import sys ; print sys.version[:3]")                                                                 

we can in all cases on RH derived assume the presence of a python in the base install of course, so it should be reasonably straightforward to stub in that code to conditionally add the python 2.7 stuff

I annex a patch to this effect for the first part

Comment 39 R P Herrold 2014-08-01 18:54:42 UTC
Created attachment 923401 [details]
patch, adding a conditional CLI sub-package

per prior comment

Comment 40 R P Herrold 2014-08-01 18:56:04 UTC
as to 24 (subpackages), either way, the present packaging is fine and so:

PASS

Comment 41 R P Herrold 2014-08-01 18:56:50 UTC
25. Static libraries must be in a -static package

not applicable

PASS

Comment 42 R P Herrold 2014-08-01 18:57:26 UTC
26. Development files must be in a -devel package

not applicable

PASS

Comment 43 R P Herrold 2014-08-01 18:58:55 UTC
27. devel packages must require the base package using a fully versioned
dependency

Not applicable here

that said, the proposed patch for the -cli subpackage also carries this

PASS

Comment 44 R P Herrold 2014-08-01 18:59:49 UTC
Packages must NOT contain any .la libtool archives

not applicable as a noarch package

PASS

Comment 45 R P Herrold 2014-08-01 19:00:55 UTC
29. GUI applications must include a %{name}.desktop file

presently not applicable; might become so if / when the wx or Thinderbird packages are added

PASS

Comment 46 R P Herrold 2014-08-01 19:05:21 UTC
30. Packages must not own files or directories already owned by other packages

seems to be fine

PASS

Comment 47 R P Herrold 2014-08-01 19:06:10 UTC
31. All filenames in rpm packages must be valid UTF-8

seems to be unexceptional and fine

PASS

Comment 48 R P Herrold 2014-08-01 19:08:32 UTC
At this point, all the MUST items on the checklist page seems to be satisfied.  I am not formally 'approved' in F circles to do reviews, but will ping for a sponsor to look this over.  I sort of fergit the ornate process and so had not fought my way through it

Comment 49 Greg Bailey 2014-08-01 19:16:29 UTC
(In reply to R P Herrold from comment #48)
> At this point, all the MUST items on the checklist page seems to be
> satisfied.  I am not formally 'approved' in F circles to do reviews, but
> will ping for a sponsor to look this over.  I sort of fergit the ornate
> process and so had not fought my way through it

Thanks for your informal review and feedback, Russ.  Much appreciated!

Comment 50 R P Herrold 2014-08-01 19:24:12 UTC
abadger1999 was kind enough to sponsor me into 'packager' so I think I can approve this now

now to figure out how to DO that in fact

Comment 51 R P Herrold 2014-08-01 19:26:47 UTC
flip on the satisfactory review flag

Comment 52 R P Herrold 2014-08-01 19:30:42 UTC
per
https://fedoraproject.org/wiki/Package_SCM_admin_requests

requesting VCS intervention

Comment 53 Greg Bailey 2014-08-01 19:35:40 UTC
New Package SCM Request
=======================
Package Name: dl
Short Description: Download Ticket Service
Upstream URL: http://www.thregr.org/~wavexx/software/dl/
Owners: gbailey herrold
Branches: f19 f20 f21 el6 epel7
InitialCC: gbailey herrold

Comment 54 R P Herrold 2014-08-01 19:39:01 UTC
thank you for the co-maintainer addition -- I will probably poke at the other two sub-packages once it gets into the stream

Comment 55 Gwyn Ciesla 2014-08-01 19:57:19 UTC
Git done (by process-git-requests).

Comment 56 Petr Šabata 2014-08-03 22:03:49 UTC
> 31. All filenames in rpm packages must be valid UTF-8
> 
> seems to be unexceptional and fine
> 
> PASS

Please, if possible, consider putting all the checks in one comment to lower the volume of bugzilla generated mails next time.  Thank you :)

Comment 57 Christopher Meng 2014-08-03 23:25:15 UTC
CAN YOU PUT ALL YOUR WORDS IN ONE COMMENT?

Comment 58 R P Herrold 2014-09-08 20:14:47 UTC
clear the flag


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