Bug 197565
Summary: | Review Request: buildbot | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Michael J Knox <michael> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | besser82, giallu, j, Marcin.Dulak, smilner |
Target Milestone: | --- | Flags: | gwync:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-09-09 05:44:57 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Michael J Knox
2006-07-04 07:15:47 UTC
See bug 197608, one of these should be closed as a dupe. 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. 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 (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 *** Bug 197608 has been marked as a duplicate of this bug. *** (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. 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). Hey Gianluca, you weren't doing the review? 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. 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 (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. oops... s/Rex/Jason :) 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! 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. 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. 0.7.4 is out Updated: Spec URL: http://www.knox.net.nz/~michael/buildbot.spec SRPM URL: http://www.knox.net.nz/~michael/buildbot-0.7.4-1.src.rpm 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? 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. Updated: Spec URL: http://www.knox.net.nz/~michael/buildbot.spec SRPM URL: http://www.knox.net.nz/~michael/buildbot-0.7.4-2.src.rpm 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 Thanks! imported and built. I am going to unorphan this package; please replace old owner. Package Change Request ====================== Package Name: buildbot New Branches: EL-5 Owners: smilner giallu cvs done. 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 Git done (by process-git-requests). Package Change Request ====================== Package Name: buildbot New Branches: epel7 Owners: besser82 smilner giallu radez We need a build for EPEL7… ^^ Git done (by process-git-requests). Is anybody working on EPEL7 build? |