Bug 197565 - Review Request: buildbot
Review Request: buildbot
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
: 197608 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-07-04 03:15 EDT by Michael J Knox
Modified: 2014-11-25 05:38 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-09 01:44:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Michael J Knox 2006-07-04 03:15:47 EDT
Spec URL: http://www.knox.net.nz/~michael/buildbot.spec
SRPM URL: http://www.knox.net.nz/~michael/buildbot-0.7.3-1.src.rpm

Description: 
The BuildBot is a system to automate the compile/test cycle required by
most software projects to validate code changes. By automatically
rebuilding and testing the tree each time something has changed, build
problems are pinpointed quickly, before other developers are
inconvenienced by the failure.
Comment 1 Ville Skyttä 2006-07-04 11:39:16 EDT
See bug 197608, one of these should be closed as a dupe.
Comment 2 Gianluca Sforna 2006-07-05 05:07:15 EDT
A quick review for your package, based on differences from what I used in mine.

1. Group is valid, but I preferred Development/Build Tools

2. Source0: can't find it right now, but I read somewhere the download location
from sourceforge pacakges should be set to http://download.sourceforge.net/ and
not on of the mirrors

3. the require on python-abi seems to be redundant on FC4 onwards. If you are
_not_ going to build this also for FC3, you can safely remove the Req and the
second line on the spec.

4. I used Requires: python-twisted >= 1.3.0 (from the buildbot web site); it's
probably safe to omit that unless some repo has an older version.

5. Requires: python-cvstoys is not _really_ required. I know we lack a
"Enhances" tag, so that's your choice. I am generally against pushing in
packages if not strictly necessary.

6. I think it's better to run %{__pyhton} instead of hardcoding python

7. if you feel comfortable with maintaining the full list of files, leave it
that way. Otherwise, the I found the solution drafted at
http://fedoraproject.org/wiki/Packaging/Python being quite smart

None of this points seems to be blockers, but I hope you can consider them
carefully.



Comment 3 Michael J Knox 2006-07-06 19:11:54 EDT
Thanks for the review.

1) used I changed it to Development/Tools as Development/Build Tools is not a
valid group. 
2) fixed
3) fixed
4) fixed
5) I am choosing to leave this one it. buildbot is 100x more functional with it.
6) fixed
7) its only a small files list, so in this case I will leave it. If it were
bigger, then I would certainly use the method suggested on the wiki.  

Updated:

Spec URL: http://www.knox.net.nz/~michael/buildbot.spec
SRPM URL: http://www.knox.net.nz/~michael/buildbot-0.7.3-2.src.rpm
Comment 4 Gianluca Sforna 2006-07-07 12:21:55 EDT
(In reply to comment #3)
> 1) used I changed it to Development/Tools as Development/Build Tools is not a
> valid group. 
That's good, but AFAIK "Development/Build Tools" _is_ a valid group. At least it
is listed in http://fedoraproject.org/wiki/RPMGroups

> 5) I am choosing to leave this one it. buildbot is 100x more functional with it.
OK

> 7) its only a small files list, so in this case I will leave it. If it were
> bigger, then I would certainly use the method suggested on the wiki.
Good


I can't see any other blockers. rpmlint is silent on the srpm while on the
actual rpm gives 3 warnings I would ignore:
W: buildbot wrong-file-end-of-line-encoding
/usr/share/doc/buildbot-0.7.3/contrib/windows/buildbot2.bat
W: buildbot doc-file-dependency
/usr/share/doc/buildbot-0.7.3/contrib/run_maxq.py /usr/bin/env
W: buildbot doc-file-dependency
/usr/share/doc/buildbot-0.7.3/contrib/svn_buildbot.py /usr/bin/env

Comment 5 Gianluca Sforna 2006-07-07 12:38:50 EDT
*** Bug 197608 has been marked as a duplicate of this bug. ***
Comment 6 Ville Skyttä 2006-07-08 05:46:56 EDT
(In reply to comment #4)

> That's good, but AFAIK "Development/Build Tools" _is_ a valid group. At least 
> it is listed in http://fedoraproject.org/wiki/RPMGroups

I think that page is somewhat misleading and its intention is not to say that
all values for the group tag found in FC4 (the latter list, where
Development/Build Tools is) are ok.  /usr/share/doc/rpm-*/GROUPS is the list of
group tag values generally considered valid.
Comment 7 Gianluca Sforna 2006-07-08 06:11:18 EDT
Thanks for the info bout valid groups.
So, I can't see anything else preventing this from being committed. I hope
someone will accept it soon (I'm deplyoing a buildbot based infrastructure at work).
Comment 8 Michael J Knox 2006-07-08 15:04:42 EDT
Hey Gianluca, you weren't doing the review? 
Comment 9 Jason Tibbitts 2006-07-26 15:18:39 EDT
It doesn't like Gianluca has the access necessary for doing reviews; I'll try to
take a look at this today or tomorrow if I can find the time and if nobody gets
to it before me.

From reading the above comments (as I haven't built the package), the rpmlint
warnings stand out:

W: buildbot wrong-file-end-of-line-encoding
/usr/share/doc/buildbot-0.7.3/contrib/windows/buildbot2.bat

What's the point of including the windows bits at all?

W: buildbot doc-file-dependency
/usr/share/doc/buildbot-0.7.3/contrib/run_maxq.py /usr/bin/env
W: buildbot doc-file-dependency
/usr/share/doc/buildbot-0.7.3/contrib/svn_buildbot.py /usr/bin/env

Documentation shouldn't be executable.
Comment 10 Michael J Knox 2006-07-26 16:11:35 EDT
Hi Jason. 

I left the Windows batch script there, largely because I work with buildbot in a
mixed eviroment. It was useful to me when I initially got buildbot working on
Windows. I figured it might be useful to others. Is no big deal, I can nuke it. 

Will fix the permissions on the contribs. 

Thanks
Comment 11 Gianluca Sforna 2006-07-26 17:41:13 EDT
(In reply to comment #9)
> It doesn't like Gianluca has the access necessary for doing reviews; 

Sorry for lurking on this. 
Rex is true: AFAIK official reviews leading to ACCEPTED status are meant to be
made from any current package owner or SPONSORS (in case this is your first
package).
Unfortunately, I do not belong to either group :(

> 
> From reading the above comments (as I haven't built the package), the rpmlint
> warnings stand out:
> 
> W: buildbot wrong-file-end-of-line-encoding
> /usr/share/doc/buildbot-0.7.3/contrib/windows/buildbot2.bat
> 
> What's the point of including the windows bits at all?
> 
> W: buildbot doc-file-dependency
> /usr/share/doc/buildbot-0.7.3/contrib/run_maxq.py /usr/bin/env
> W: buildbot doc-file-dependency
> /usr/share/doc/buildbot-0.7.3/contrib/svn_buildbot.py /usr/bin/env
> 
> Documentation shouldn't be executable.

maybe the whole "contrib" directory can go in /usr/share/buildbot (along the
line of /usr/share/cvs/contrib/), since those are support scripts and not really
documentation.

One last remark: I can not recall where I read it, but I believe it is
recommended to not repeat the name of the package in the Summary field.
Comment 12 Gianluca Sforna 2006-07-26 17:42:45 EDT
oops... s/Rex/Jason :)
Comment 13 Michael J Knox 2006-07-27 17:19:23 EDT
Updated:

Spec URL: http://www.knox.net.nz/~michael/buildbot.spec
SRPM URL: http://www.knox.net.nz/~michael/buildbot-0.7.3-3.src.rpm

Moved the contribs to /usr/share/buildbot/contribs

Thanks for that suggestion!
Comment 14 Jason Tibbitts 2006-08-12 22:13:30 EDT
Well, the python guidelines have been changed to get rid of the "ghost the .pyo
files" bit.  So your files section should shrink a bit.  (In any case, you
didn't ghost the .pyo files in the contrib directory.)

This builds fine in mock; rpmlint has the following to say about the SRPM:

W: buildbot mixed-use-of-spaces-and-tabs
  You use spaces everywhere except for Patch0: and BuildArch:.  Not a really big
deal.

There are many subdirectories under python_sitelyb/buildbot that you don't own.
 Getting rid of the %ghost bits should fix this as well.

There's something that looks like a test suite in buildbot/test.  Is this
something that could be run at package build time?  Should it really be included
with the installed package?

Review:
* source files match upstream:
   7be16fe13f173e46df711ed51648e750  buildbot-0.7.3.tar.gz
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text not included upstream.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (development, x86_64).
X rpmlint is silent.
* final provides and requires are sane:
   buildbot = 0.7.3-3.fc6
  =
   /usr/bin/env
   /usr/bin/python
   python(abi) = 2.4
   python-cvstoys
   python-twisted >= 1.3.0
? %check is not present but there might be a test suite.
* package is not relocatable.
X owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.
Comment 15 Michael J Knox 2006-08-20 20:05:17 EDT
Hey, just a quick ping to let you know that I am alive, just still in the trows
of unpacking/new job/etc etc. I hope to tidy this review up before/by the end of
the week. Thanks for your patience. 
Comment 16 Gianluca Sforna 2006-08-24 04:01:22 EDT
0.7.4 is out
Comment 18 Jason Tibbitts 2006-09-08 17:24:58 EDT
You still have several unowned directories under %{python_sitelib}/buildbot. 
Why can't you just have a %files section like:

%files
%defattr(-,root,root,-)
%doc ChangeLog NEWS README docs
%{_bindir}/buildbot
%{python_sitelib}/buildbot
%{_datadir}/%{name}

instead of all of those globs.

Also, could you comment on the test suite issue I brought up in comment 14?
Comment 19 Michael J Knox 2006-09-08 17:44:19 EDT
Sorry. Missed that. 

The test stuff is for running tests on a source tree (if availible), like "make
test". Its part of buildbot's process. Its not a test for buildbot itself. 

Will wrap up a new srpm shortly.
Comment 21 Jason Tibbitts 2006-09-08 21:14:23 EDT
OK, everything looks fine now. THe directories all look to be properly owned,
rpmlint is silent and what I thought might be a test suite isn't.

APPROVED
Comment 22 Michael J Knox 2006-09-09 01:44:57 EDT
Thanks! imported and built. 
Comment 23 Gianluca Sforna 2007-02-20 18:46:07 EST
I am going to unorphan this package; please replace old owner.
Comment 24 Steve Milner 2010-05-12 11:20:12 EDT
Package Change Request
======================
Package Name: buildbot
New Branches: EL-5
Owners: smilner giallu
Comment 25 Kevin Fenzi 2010-05-12 13:11:47 EDT
cvs done.
Comment 26 Gianluca Sforna 2012-03-16 06:00:10 EDT
Package Change Request
======================
Package Name: buildbot
New Branches: el6
Owners: smilner giallu

Steve, I hope it is ok for you to co-maintain also the el6 branch, if not just let me know
Comment 27 Jon Ciesla 2012-03-16 08:15:09 EDT
Git done (by process-git-requests).
Comment 28 Björn "besser82" Esser 2014-08-10 07:42:07 EDT
Package Change Request
======================
Package Name: buildbot
New Branches: epel7
Owners: besser82 smilner giallu radez

We need a build for EPEL7…  ^^
Comment 29 Jon Ciesla 2014-08-11 08:20:49 EDT
Git done (by process-git-requests).
Comment 30 marcindulak 2014-11-25 05:38:15 EST
Is anybody working on EPEL7 build?

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