Bug 754094 - Review Request: restauth - Web-service providing shared authentication, authorization and preferences
Summary: Review Request: restauth - Web-service providing shared authentication, autho...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jaroslav Škarvada
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-15 11:56 UTC by Jan Kaluža
Modified: 2011-12-12 22:05 UTC (History)
4 users (show)

Fixed In Version: restauth-0.5.2-3.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-12 22:05:55 UTC
Type: ---
jskarvad: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Jan Kaluža 2011-11-15 11:56:59 UTC
Spec URL: http://jkaluza.fedorapeople.org/restauth.spec
SRPM URL: http://jkaluza.fedorapeople.org/restauth-0.5.2-1.fc15.src.rpm
Description:

Hi,
I've finished packaging of restauth server implementation. This module
collects various code used in RestAuth server/client implementations.

It depends on python-mimeparse and python-restauth-common:
 - https://bugzilla.redhat.com/show_bug.cgi?id=754064
 - https://bugzilla.redhat.com/show_bug.cgi?id=754088
Consider reviewing also python-mimeparse and python-restauth-common before reviewing this package please.

$ rpmlint restauth-0.5.2-1.fc15.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 1 Jaroslav Škarvada 2011-11-21 15:17:35 UTC
[NO] rpmplint is silent
restauth.noarch: W: conffile-without-noreplace-flag /etc/restauth/localsettings.py
  The noreplace flag would be probably worth here.
restauth.noarch: E: zero-length /usr/lib/python2.7/site-packages/RestAuth/common/models.py
restauth.noarch: E: zero-length /usr/lib/python2.7/site-packages/RestAuth/common/urls.py
  Empty files shouldn't be packages unless there is special need for this. It would be good to forward this to upstream.
restauth.noarch: E: non-readable /etc/restauth/localsettings.py 0640L
  Probably OK to hide the DB secrets.
restauth.noarch: W: no-manual-page-for-binary restauth-group
restauth.noarch: W: no-manual-page-for-binary restauth-import
restauth.noarch: W: no-manual-page-for-binary restauth-service
restauth.noarch: W: no-manual-page-for-binary restauth-manage
restauth.noarch: W: no-manual-page-for-binary restauth-user
  It would be good to have the man pages. It seems they are already included,
  but not build and packaged.

[YES] Package meets naming guidelines.
[YES] Package meets packaging guidelines.
  Maybe missing version requirement for python-restauth-common.
[YES] Spec file matches base package name.
[YES] License file is present, matching with spec file.
[YES] Licensing Guidelines are met.
[YES] Spec file is legible and in American English.
[YES] Sources match upstream.
[YES] Package builds OK.
[!] BuildRequires are correct.
  I cannot find the python-setuptools-devel in rawhide.
  Is the python-setuptools really needed? It seems to build OK without it.

[YES] Package doesn't bundle copies of system libraries.
[YES] Package owns all the directories it creates.
[YES] Package has no duplicity in %files.
[!] Permission on files are set properly.
  The /etc/restauth/localsettings.py, maybe OK as commented above.
[NO] Spec file has consistant macro usage.
  Please use %{optflags} instead of $RPM_OPT_FLAGS or $RPM_BUILD_ROOT instead of %{buildroot}.
[YES] Package is code or permissible content.
[YES] %doc files don't affect runtime.
[YES] Package doesn't own files/directories that other packages own.
[YES] All files are valid UTF-8.

Should items:
[YES] Package builds in mock.
[YES] Package uses sane scriptlets.
[NO] Package contains man pages.
[YES] Very simple functionality test passed.

Some more comments:
There is extra space in the second %doc (only cosmetic issue :)

%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib()")}
is probably not needed any more.

The defattr is also not needed.

AFAIK the above have only sense if it is planned to package for RHEL-5 EPEL. If so there should be also more additions (e.g. %clean section, ...).

Shouldn't you go with brp-python-bytecompile script and only remove the
/etc/restauth/localsettings.pyo and /etc/restauth/localsettings.pyc before packaging?

It would be good to build and package the man pages. There are also several
docs, that could be useful for end users, please consider packaging them.

What about the Python3? Is this supported? Upstream states Python 2.6 or later. If it is supported please also consider packaging for python3.

Comment 2 Jaroslav Škarvada 2011-11-22 08:23:38 UTC
(In reply to comment #1)

> Shouldn't you go with brp-python-bytecompile script and only remove the
> /etc/restauth/localsettings.pyo and /etc/restauth/localsettings.pyc before
> packaging?
> 
Probably impossible without more hacks, please ignore the above comment.

Comment 3 Jan Kaluža 2011-11-22 12:43:49 UTC
Fixed spec file and srpm:

Spec URL: http://jkaluza.fedorapeople.org/restauth.spec
SRPM URL: http://jkaluza.fedorapeople.org/restauth-0.5.2-2.fc16.src.rpm

> restauth.noarch: W: conffile-without-noreplace-flag
> /etc/restauth/localsettings.py
>   The noreplace flag would be probably worth here.

Added.

> restauth.noarch: E: zero-length
> /usr/lib/python2.7/site-packages/RestAuth/common/models.py
> restauth.noarch: E: zero-length
> /usr/lib/python2.7/site-packages/RestAuth/common/urls.py
>   Empty files shouldn't be packages unless there is special need for this. It
> would be good to forward this to upstream.

I've added comment to those files, so they should not be empty anymore. Those files are needed there, because Django autodetects Django application if those files are present. Upstream will put license there and comment similar to one I used.

> restauth.noarch: E: non-readable /etc/restauth/localsettings.py 0640L
>   Probably OK to hide the DB secrets.

Yes, that's why those permissions are set.

> restauth.noarch: W: no-manual-page-for-binary restauth-group
> restauth.noarch: W: no-manual-page-for-binary restauth-import
> restauth.noarch: W: no-manual-page-for-binary restauth-service
> restauth.noarch: W: no-manual-page-for-binary restauth-manage
> restauth.noarch: W: no-manual-page-for-binary restauth-user

I included manpages from doc/ directory. According to upstream restuath-manage manpage is not finished yet and it's in TODO.

>   Maybe missing version requirement for python-restauth-common.

It should not be needed to depend on particular python-restauth-common version according to upstream.

> [!] BuildRequires are correct.
>   I cannot find the python-setuptools-devel in rawhide.
>   Is the python-setuptools really needed? It seems to build OK without it.

Fixed as with python-restauth-common package.

> [NO] Spec file has consistant macro usage.
>   Please use %{optflags} instead of $RPM_OPT_FLAGS or $RPM_BUILD_ROOT instead
> of %{buildroot}.

Fixed.

> Shouldn't you go with brp-python-bytecompile script and only remove the
> /etc/restauth/localsettings.pyo and /etc/restauth/localsettings.pyc before
> packaging?

It's not possible, pyc and pyo are generated after %install section. There's no way to remove those files.

> It would be good to build and package the man pages. There are also several
> docs, that could be useful for end users, please consider packaging them.

Added restauth-doc package. Man pages are included too.

> What about the Python3? Is this supported? Upstream states Python 2.6 or later.

According to upstream "restauth" itself works only with python-2.x because of Django, which does not support python-3.x so far...

Comment 4 Jaroslav Škarvada 2011-11-28 11:20:30 UTC
Some more warnings now:
restauth-doc.noarch: W: hidden-file-or-dir /usr/share/doc/restauth-doc-0.5.2/html/.buildinfo
restauth.noarch: W: spurious-executable-perm /usr/share/man/man1/restauth-import.1.gz
restauth.noarch: W: spurious-executable-perm /usr/share/man/man1/restauth-service.1.gz
restauth.noarch: W: spurious-executable-perm /usr/share/man/man1/restauth-group.1.gz
restauth.noarch: E: non-readable /etc/restauth/localsettings.py 0640L
restauth.noarch: W: spurious-executable-perm /usr/share/man/man1/restauth-user.1.gz
restauth.noarch: W: no-manual-page-for-binary restauth-manage

I wouldn't package the doctrees cache.

Comment 5 Jan Kaluža 2011-11-28 13:29:10 UTC
Fixed spec file and srpm:

Spec URL: http://jkaluza.fedorapeople.org/restauth.spec
SRPM URL: http://jkaluza.fedorapeople.org/restauth-0.5.2-3.fc16.src.rpm

After discussion with upstream we decided to keep localsettings world-readable, so there should be no rpmlint error this time.

Comment 6 Jaroslav Škarvada 2011-11-29 15:17:36 UTC
Looks good for me, giving fedora-review +

Comment 7 Jan Kaluža 2011-11-30 07:00:02 UTC
New Package SCM Request
=======================
Package Name: restauth
Short Description: Web-service providing shared authentication, authorization and preferences
Owners: jkaluza
Branches: f15 f16
InitialCC: jskarvad

Comment 8 Gwyn Ciesla 2011-11-30 10:54:52 UTC
Git done (by process-git-requests).

Comment 9 Fedora Update System 2011-12-01 09:13:47 UTC
restauth-0.5.2-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/restauth-0.5.2-3.fc16

Comment 10 Fedora Update System 2011-12-04 02:42:26 UTC
restauth-0.5.2-3.fc16 has been pushed to the Fedora 16 testing repository.

Comment 11 Fedora Update System 2011-12-12 22:05:55 UTC
restauth-0.5.2-3.fc16 has been pushed to the Fedora 16 stable repository.


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