Bug 487097 - Review Request: ReviewBoard - web based code review tool
Review Request: ReviewBoard - web based code review tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Dave Malcolm
Fedora Extras Quality Assurance
:
Depends On: 487098 488103
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-24 02:13 EST by ramez hanna
Modified: 2010-04-01 17:02 EDT (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-01-13 19:02:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
dmalcolm: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
SRPM for ReviewBoard 1.0.5.1 (496.07 KB, application/x-rpm)
2009-12-21 13:40 EST, Stephen Gallagher
no flags Details
Specfile for ReviewBoard 1.0.5.1 (3.17 KB, application/octet-stream)
2009-12-21 13:42 EST, Stephen Gallagher
no flags Details
rpmlint output for 1.0.5.1 RPM (2.05 KB, text/plain)
2009-12-22 10:35 EST, Stephen Gallagher
no flags Details
Specfile for ReviewBoard 1.0.5.1 (3.43 KB, application/octet-stream)
2009-12-22 14:19 EST, Stephen Gallagher
sgallagh: review? (dmalcolm)
Details

  None (edit)
Description ramez hanna 2009-02-24 02:13:08 EST
Spec URL: http://informatiq.org/files/reviewboard.spec
SRPM URL: http://informatiq.org/files/ReviewBoard-1.0alpha4-1.fc10.src.rpm

Description: 
I just packaged ReviewBoard, I'm fairly new in packaging and would appreciate a review of them
it was announced on the fedora-infrastructure mailing list that it is needed, so here it is.
Comment 1 Dan Young 2009-02-24 13:34:55 EST
Examining the output of rpmlint run against your SRPM/RPMs is always a good place to start.

Are you a Fedora Contributor and member of the packager group, or are you seeking sponsorship?
Comment 2 Dan Young 2009-02-24 16:31:58 EST
Review Board also has django-evolution as a dep.

Here's an initial spec:
http://files.mesd.k12.or.us/~dyoung/reviewboard/django-evolution.spec
Comment 3 Fabian Affolter 2009-03-08 11:28:37 EDT
Can you please open Review request for django-evolution?  This is the one for ReviewBoard ;-)
Comment 4 Dave Malcolm 2009-03-17 11:18:59 EDT
There's already a separate review for django-evolution, in APPROVED state: see bug 488103.
Comment 5 Fabian Affolter 2009-04-23 09:35:15 EDT
AUTHORS, COPYING, NEWS, and README should be placed in %doc.
Comment 6 Dave Malcolm 2009-05-07 12:05:38 EDT
Adding dependency on the review request you filed for Djblets (bug 487098) since this specfile has a Requires on Djblets.
Comment 7 Dave Malcolm 2009-05-07 17:55:40 EDT
I've taken Dan's work on top of Ramez's, updated it to the latest release candidate, and fixed some issues.  I renamed the "Djblets" dependency to "python-djblets" to reflect the change I proposed in bug 487098.
Hope this is all OK.

Updated specfile is here:
http://people.redhat.com/dmalcolm/python/ReviewBoard.spec

Updated SRPM is here:
http://people.redhat.com/dmalcolm/python/ReviewBoard-1.0-0.4.rc1.src.rpm

Output from rpmlint is clean on the SRPM, and on the built RPM gives the output:
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/post-lock.tmpl 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/pre-lock.tmpl 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/contrib/tools/post-commit 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/post-unlock.tmpl 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/post-revprop-change.tmpl 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/start-commit.tmpl 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/pre-commit.tmpl 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/contrib/conf/reviewboard.fcgi.in 0644
ReviewBoard.noarch: W: devel-file-in-non-devel-package /usr/lib/python2.4/site-packages/reviewboard/diffviewer/testdata/new_src/foo.c
ReviewBoard.noarch: E: zero-length /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/db/write-lock
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/post-commit.tmpl 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/pre-unlock.tmpl 0644
ReviewBoard.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/reviewboard/scmtools/testdata/svn_repo/hooks/pre-revprop-change.tmpl 0644
ReviewBoard.noarch: W: devel-file-in-non-devel-package /usr/lib/python2.4/site-packages/reviewboard/diffviewer/testdata/orig_src/foo.c

These appear to all be example files or test data, and thus I don't think they're real problems.

Caveat: I'm doing this all on a RHEL5 box, rather than specifically Fedora.

How's this looking?  Ramez, do you still want to own this package?  Dan?  I'd be happy to co-maintain.
Comment 8 ramez hanna 2009-05-13 04:47:52 EDT
(In reply to comment #7)

> How's this looking?  Ramez, do you still want to own this package?  Dan?  I'd
> be happy to co-maintain.  
Please join in, i am kinda busy at the moment.
Comment 9 Cristian Ciupitu 2009-06-15 08:32:44 EDT
Are you sure about the "BuildRequires: python-setuptools-devel" dependency? I was able to build the RPM without it.
Comment 10 Cristian Ciupitu 2009-06-24 14:44:24 EDT
By the way, Review Board 1.0 was just released (http://www.review-board.org/news/2009/06/20/review-board-10-released), so maybe a package for 1.0 should be created.
Comment 11 Jan Klepek 2009-09-11 07:20:07 EDT
any progress on packaging latest version?
Comment 12 Dave Malcolm 2009-09-11 11:28:02 EDT
Updated for 1.0.1; specfile is here:
http://people.redhat.com/dmalcolm/python/ReviewBoard.spec

and SRPM here:
http://people.redhat.com/dmalcolm/python/ReviewBoard-1.0.1-1.fc11.src.rpm

I dropped the "BuildRequires: python-setuptools-devel" as suggested in comment #9
Comment 13 Dave Malcolm 2009-10-01 10:13:52 EDT
(FWIW, my motivation here is that I'm hoping that Fedora Infrastructure can deploy an instance of reviewboard; see: https://fedorahosted.org/fedora-infrastructure/ticket/1196
"Deploy Review Board for use by hosted projects")
Comment 14 Stephen Gallagher 2009-10-01 11:04:54 EDT
Please re-add the "BuildRequires: python-setuptools-devel" to the spec. I was unable to build it cleanly in mock without this.

I attempted to build on a fully up-to-date F11 x86_64 box on mock using the fedora-11-x86_64 target.
Comment 15 Cristian Ciupitu 2009-10-01 11:38:15 EDT
(In reply to comment #14)
> Please re-add the "BuildRequires: python-setuptools-devel" to the spec. I was
> unable to build it cleanly in mock without this.
> 
> I attempted to build on a fully up-to-date F11 x86_64 box on mock using the
> fedora-11-x86_64 target.  

I'm running an up-to-date Fedora 11 x86_64 too I was able to rebuild ReviewBoard-1.0.1-1.fc11.src.rpm after removing the python-setuptools-devel package. What errors are you getting?
Comment 16 Jeffrey C. Ollie 2009-10-15 11:51:06 EDT
Ping?  Any recent action here?
Comment 17 Jeremy Katz 2009-10-16 15:28:22 EDT
There seems to be missing dependencies on python-pygments and django-evolution and a newer python-djblets is needed

But beyond that, it seems to be working in some quick setup and testing.
Comment 18 Dan Young 2009-10-16 19:57:02 EDT
(In reply to comment #15)
> (In reply to comment #14)
> > Please re-add the "BuildRequires: python-setuptools-devel" to the spec. I was
> > unable to build it cleanly in mock without this.
> > 
> > I attempted to build on a fully up-to-date F11 x86_64 box on mock using the
> > fedora-11-x86_64 target.  
> 
> I'm running an up-to-date Fedora 11 x86_64 too I was able to rebuild
> ReviewBoard-1.0.1-1.fc11.src.rpm after removing the python-setuptools-devel
> package. What errors are you getting?  

A mock build barfs without a BuildRequires on python-setuptools, specifically:

+ /usr/bin/python setup.py build
Traceback (most recent call last):
  File "setup.py", line 15, in <module>
    from setuptools import setup, find_packages
ImportError: No module named setuptools

I've added "BuildRequires: python-setuptools" to fix the above and "Requires: python-pygments\nRequires: django-evolution" per jkatz in comment #17:
http://files.mesd.k12.or.us/~dyoung/reviewboard/ReviewBoard.spec
http://files.mesd.k12.or.us/~dyoung/reviewboard/ReviewBoard-1.0.1-2.fc11.src.rpm
Comment 19 Stephen Gallagher 2009-12-21 13:40:57 EST
Created attachment 379673 [details]
SRPM for ReviewBoard 1.0.5.1
Comment 20 Stephen Gallagher 2009-12-21 13:42:55 EST
Created attachment 379674 [details]
Specfile for ReviewBoard 1.0.5.1

I updated Dan Young's specfile from http://files.mesd.k12.or.us/~dyoung/reviewboard/ReviewBoard.spec to build ReviewBoard 1.0.5.1, now that python-djblets and Django 1.1.1 are available in Fedora.

I needed to update to Django 1.1.1, as upstream ReviewBoard requires it for security fixes.
Comment 21 Stephen Gallagher 2009-12-22 10:35:57 EST
Created attachment 379845 [details]
rpmlint output for 1.0.5.1 RPM

Scratch build successfully performed for Rawhide:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1886173

Scratch build successfully performed for Fedora 12:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1886175

rpmlint output attached.
Comment 22 Dave Malcolm 2009-12-22 11:19:14 EST
I'll have a go at reviewing this
Comment 23 Stephen Gallagher 2009-12-22 11:36:45 EST
For the record, this SRPM also builds successfully against EPEL5 ( http://koji.fedoraproject.org/koji/taskinfo?taskID=1886304 ), though we would certainly need to build its dependencies as well.
Comment 24 Dave Malcolm 2009-12-22 12:45:45 EST
This is looking good, with 4 issues that need attention (see below).

As I understand things, as this review was opened by Ramez, he would be the initial owner as things stand.  However it appears from comment #8 that he may be rather busy.

I spoke with Stephen today and he's keen to get this into Fedora ASAP.

Stephen: are you happy to maintain this?

Ramez: are you still interested in maintaining this package within Fedora?
Dan: are you interested in (co)maintaining it?

So we may want to complete the "review" part of the review, have Stephen open a fresh review request, close this one as dup of the new, and "grandfather in" the work done here.  Does that sound OK?

= Issues needing attention =
(i) installation issue on F-12
Stephen's F12 scratch build doesn't install on my laptop F-12 with updates enabled, but not updates-testing:
Error: Missing Dependency: Django >= 1.1.1 is needed by package ReviewBoard-1.0.5.1-1.fc12.noarch (/ReviewBoard-1.0.5.1-1.fc12.noarch)
Error: Missing Dependency: python-djblets >= 0.5-0.1.rc1 is needed by package ReviewBoard-1.0.5.1-1.fc12.noarch (/ReviewBoard-1.0.5.1-1.fc12.noarch)
- latest version of Django in F-12 updates is Django-1.1-4.fc12
- python-djblets doesn't seem to actually be in fedora-updates for f12 yet

Stephen: do you have this installed and working on a machine?

(ii) Source0: URL is 404; need to be changed to:
http://downloads.reviewboard.org/releases/%{name}/1.0/%{name}-%{version}.tar.gz
(see http://downloads.reviewboard.org/releases/ReviewBoard/1.0/ ; I notice there's a 1.1 directory as well FWIW)

(iii) desktop files: the rb-site executable has a PyGTK GUI, so would normally require us to ship a .desktop file.  However it can only be run when supplied a directory as a command-line argument, hence it wouldn't be meaningful to create a .desktop file for it.  So this is OK, but please add a comment about the exception to the specfile.

(iv) Does the package embed all of the requirements for the various SCM backends?  (How well does this work with git?)  (not easy to check this without a working install)

= Notes =
Filesystem layout: upstream have structured this code as a library and supporting tools that can be used to create (potentially) multiple local instances of ReviewBoard on a host, each stored in an arbitrary directory on the filesystem.  All information for a specific instance (e.g. config, logs, tmp) goes below a particular directory for that instance (rather than e.g. /etc).  I think it's acceptable for our package to reflect how upstream have structured this.

= Reviewed items =
- naming: name matches that of upstream tarball
- specfile name is good
- packaging guidelines:
  - N-V-R looks good
  - licensing "MIT" in spec matches that of README and of setup.py
  - spec is legible
  - spec follow python norms
  - changelog: OK
  - tags: OK
  - buildroot path uses 2nd recommendation in guidelines
  - buildroot is cleaned
  - %clean is present and correct
  - buildrequirements: successfully scratch-built in Koji
  - textual documentation present in built RPM below /usr/share/doc/ReviewBoard-1.0.5.1
  - compiler flags/debuginfo packages/devel packages: N/A
  - pkgconfig: N/A
  - shared libraries: N/A
  - packaging static libraries: N/A
  - dup of system libraries: doesn't seem to
  - rpath: N/A for pure python code
  - config files: see note about FHS above
  - initscripts: N/A
  - macros: OK
  - locale handling: no translations present in upstream source
  - scriptlets: N/A
  - code vs content: OK
  - file and dir ownership: OK
  - users and groups: doesn't have its own user
  - web app: uses /usr/lib/python for its data, which seems reasonable
  - /srv: OK
  - patches: none yet
  - epochs: OK
  - Python-specific guidelines: OK
- license: OK
- specfile is legible
- MD5sum: OK
  - tarball in srpm:
16947ddda7ec9df41f243949ec83a950  ReviewBoard-1.0.5.1.tar.gz
  - tarball from upstream:
16947ddda7ec9df41f243949ec83a950  ReviewBoard-1.0.5.1.tar.gz
- rest of the MUST items covered above
- I've tested an earlier version of the rpm and it functions
Comment 25 Stephen Gallagher 2009-12-22 14:19:40 EST
Created attachment 379897 [details]
Specfile for ReviewBoard 1.0.5.1

I am willing to co-maintain ReviewBoard (but I do not want to be its exclusive maintainer).

I don't think there's really a need to go through the trouble of opening a second review request, but if that's The Way It's Done, sure.

Issues:
(i) Dgango 1.1.1-2 is in updates-testing, python-djblets I installed from this koji build: http://koji.fedoraproject.org/koji/buildinfo?buildID=147983 (I have submitted this package for updates-testing in F-12, so it should be present tomorrow)

(ii) Source location fixed. For the record, the 1.1 branch is their unstable development branch, and I do not intend at the moment to import that.

(iii) Desktop file comment added.

(iv) The package contains all the files necessary to support git, mercurial, bazaar, clearcase, cvs, perforce and subversion at least. I am unable to test the suitability of all of them. I have a test environment working with git successfully using this RPM.
Comment 26 Stephen Gallagher 2009-12-22 14:20:51 EST
This specfile built successfully in Koji here: http://koji.fedoraproject.org/koji/taskinfo?taskID=1886848

The SRPM and built noarch RPM is also available there.
Comment 28 Dave Malcolm 2009-12-22 14:40:27 EST
Thanks; I think several of us want to comaintain this package, so I think that should be OK.

A minor issue: you didn't update the %changelog in your latest specfile

Other than that, this looks good to go.

ACCEPTED.

Setting "fedora-review" flag

Please fix the %changelog before importing the specfile - thanks!
Comment 29 Stephen Gallagher 2009-12-22 15:13:37 EST
New Package CVS Request
=======================
Package Name: ReviewBoard
Short Description: Web-based code review tool
Owners: sgallagh dmalcolm
Branches: F-12
InitialCC:
Comment 30 Dan Young 2009-12-22 18:53:33 EST
(In reply to comment #24)
> Dan: are you interested in (co)maintaining it?

Sure, I can co-maintain. I'll request the ACL once ReviewBoard lands in CVS/pkgdb.
Comment 31 Dan Young 2009-12-22 19:01:59 EST
(In reply to comment #25)
> (i) Dgango 1.1.1-2 is in updates-testing, python-djblets I installed from this
> koji build: http://koji.fedoraproject.org/koji/buildinfo?buildID=147983 (I have
> submitted this package for updates-testing in F-12, so it should be present
> tomorrow)

I submitted python-djblets in bodhi shortly after building:
https://admin.fedoraproject.org/updates/python-djblets-0.5.6-0.fc12

I assume I just did that too late in the day yesterday for RelEng to sign it for updates-testing today.
Comment 32 Dennis Gilmore 2009-12-23 14:49:33 EST
CVS Done
Comment 33 Stephen Gallagher 2010-01-04 09:23:55 EST
ReviewBoard built in Rawhide:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1900941
Comment 34 Stephen Gallagher 2010-01-04 14:52:07 EST
Package Change Request
======================
Package Name: ReviewBoard
New Branches: EL-5
Owners: sgallagh

ReviewBoard is being considered for inclusion in the Fedora Hosted infrastructure. We need to branch this package to EL-5 for this support.
Comment 35 Kevin Fenzi 2010-01-04 15:14:29 EST
cvs done.
Comment 36 Dave Malcolm 2010-01-13 19:02:44 EST
Looking at https://admin.fedoraproject.org/pkgdb/packages/name/ReviewBoard I see that this has been imported and built; belatedly closing this review out.
Comment 37 Fedora Update System 2010-01-15 15:44:06 EST
ReviewBoard-1.0.5.1-2.1.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/ReviewBoard-1.0.5.1-2.1.el5
Comment 38 Fedora Update System 2010-04-01 17:02:30 EDT
ReviewBoard-1.0.5.1-2.1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

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