Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 199780
Summary: | Review Request: dstat | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Scott Baker <scott> |
Component: | Package Review | Assignee: | Toshio Kuratomi <toshio> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | adrianalves, paul, toshio |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 0.6.3-5 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-09-21 21:55:09 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
Scott Baker
2006-07-21 21:46:52 UTC
Okay. To start, a few links: [1]_ General Guidelines the package must follow: http://www.fedoraproject.org/wiki/Packaging/Guidelines [2]_ Python Specific Packaging Hints: http://www.fedoraproject.org/wiki/Packaging/Python [3]_ Fedora Extras CVS. Look for examples of how other people do things here: http://cvs.fedora.redhat.com/viewcvs/rpms/?root=extras Some python applications you can look through there are qa-assistant, rpmlint, and gramps. The first thing that leaps out at me when I look at the spec is that the python files aren't bytecompiled. In addition to installing, you want to generate the .pyc and .pyo byte compiled files and install them. That way the package will know that it needs to remove the files as well. [1] and [3] should help you get through that. Looking over a spec or two from [3] and comparing to your rpm would be another good thing to do. (And now you can jump back on IRC and ask specific questions and we'll be better able to help ;-) The spec file should be dstat.spec, not dstat-fedora.spec. Also setup needs -q . Added setup -q and renamed spec file to dstat.spec http://www.perturb.org/tmp/dstat.spec I will look into the python compiling to bytecode etc. Updated the spec and the src RPM to bytecode compile the python. Spec URL: http://www.perturb.org/tmp/dstat.spec SRPM URL: http://www.perturb.org/tmp/dstat-0.6.3-2.src.rpm What's the next step in getting this reviewed? Whoever is assigned to this bug reviews the fixes you've made, test builds, runs rpmlint over the packages created, compiles in mock, installs and runs rpmlint over the installed package to ensure nothing untoward happens. This isn't an instant process - some bugs can be open for ages! Looking at the spec file you've got there, I can't see anything which bytecode compiles the python. Remember a # before anything is just counted as a comment and rpmbuild skips past it as if it wasn't there. A couple of other niggles In the Source: you have .../dstat/dstat-%{version}... This can be substituted with .../%{name}/%{name}-%{version}. I know on packages I've had reviewed on, this has been brought up. The make install line can just be make DESTDIR=%{buildroot} install - you don't need the quotes around the buildroot Does the package come with any language translation files? Quick note as I start reviewing: You should always bump the release number, even when you are making new spec files for review. This lets me easily reference the -2 release as having Problem A) and the -3 release fixed that but introduced problem B). It makes it easier to figure out what problems are still outstanding against a package and which have been fixed. Thanks. MD5sums: 72917aa5eed385464d70ec731bd6d2b1 dstat-0.6.3-2.src.rpm a2df5d7fecc0115f8eef84141a068e86 dstat-0.6.3.tar.bz2 Blocker: * Your patch adds #!/usr/bin/python to the modules files. Since the modules are not intended to be run from the command line, this is improper behaviour. You should make the files non-executable instead. * It's customary to put the defattr line first in the files section. I believe that rpm uses the defattr line even on lines that preceed it in the file listings but I don't know if that's documented behaviour. If that behaviour ever changes, then you could suddenly have files with incorrect ownership and permissions. * Remove the execute permissions from the scripts in the examples directory. * Package needs to own the dstat directory like this: %dir %{_datadir}/dstat Cosmetic: * It looks like you're patching out dos line endings for html files. This is okay if you're going to submit the change upstream. As a general rule (for instance, if upstream were to not accept the change and you had to continue to carry it around in the package) it's better to use sed or dos2unix to fix this. Otherwise your patch becomes filled with whole files where the only difference is the EOL character. * Some people still use the default rpm defined topdir, sourcedir, etc. When this is so, installing the source rpm places the files in the same directory as a multitude of other rpms. So your patch: patch-to-clean-for-fedora.patch should really have a less generic name. Something like dstat-eol.patch or dstat-eol-cleanup.pach. * I'd remove the commented out code as it doesn't add any value to the spec. * You don't need to Require: python; this is being picked up automatically. Good: * Source matches upstream. * Package Name follows the Naming Guidelines. * Spec file follows the %{name}.spec format. * License is GPL, matches the license field, and is included in the rpm. * Package built on x86_64 as a noarch package. * All BuildRequires not listed in the exceptions have been satisfied. * No locale files, so no need to use %find_lang. * No libraries or pkgconfig files. * Package is not relocatable. * No duplicate files. * Package has a proper %clean section. * Package uses proper macros from Packaging Guidelines. * Code, not content. * No large doc files. * Doc files do not affect the runtime behaviour. * Not a GUI application. * Does not own directories that belong to another package. * No scriptlets. * Package built in mock. After you fix the listed issues, I can run a last rpmlint check and make sure the program runs and then APPROVE the package. Since you need a sponsor, I need to know that you understand the guidelines before SPONSORing you. This package was a good indication of understanding on your part. If you do a review of someone else's package that continues to demonstrate your knowledge of the Fedora Guidelines in particular and rpm packaging in general, I'll sponsor you as well. Ok I bumped the release number to 3, and re-uploaded it. I removed my patch, and did the work in the spec file. Sed is used to convert the documentation line endings, and I chmoded the .py as requested. Moved defattr to the top, and added the %dir lines. Commented out code was removed as was the require python. rpmlint is only giving out warnings about the line endings as well as the a stale symlink. I've done my best to handle that in the .spec file (see the rm and sed lines) but it's just not working. I'm over my head. SPEC: http://www.perturb.org/tmp/dstat.spec SRC: http://www.perturb.org/tmp/dstat-0.6.3-3.src.rpm * The files don't get moved into _docdir until after %install. So instead of operating on %{buildroot}/%{_docdir}/* you want to operate on the files in your source tree. * You've commented out the %clean section. You'll want to put that back. * There are other pieces of commented code that could be removed: #BuildRequires:.. #Requires:.... #/usr/lib/rpm/brp-python-bytecompile.... #%config(noreplace) %{_sysconfdir}/dstat.conf.... You can leave them in with explanations if you think they make the spec clearer and easier to understand (I see that you have commented on two of those listed already. The choice is yours.) * For every release bump, there must also be a changelog entry. So instead of just bumping the release number on the changelog line, add a whole new entry that tells what you've changed. Oh lame... I posted last week and it didn't make it. Spec: http://www.perturb.org/tmp/dstat.spec SRPM: http://www.perturb.org/tmp/dstat-0.6.3-4.src.rpm - Removed some commeted lines in the .spec file that weren't needed - Changed the permissions on the examples/* scripts - Converted the HTML documentation to unix line endings - Removed the erroneous commenting of the %clean section MD5sums: ed346ba8fb71514e5be2202d50a17568 ../dstat-0.6.3-4.src.rpm a2df5d7fecc0115f8eef84141a068e86 dstat-0.6.3.tar.bz2 0862411cacd97a31182dd2b0199c512f dstat.spec Most previously noted problems fixed. Here's what remains: * Remove the execute permissions from the scripts in the examples directory. * macros (Like %clean) are expanded even when they're in the changelog section. You'll want to escape the macro in your changelog with an additional "%":: - Removed the erroneous commenting of the %%clean section After you fix these issues, I can do a last check and APPROVE the package. You will also need to be sponsored so you can import the package into the repository. I am willing to do this if you show your understanding of the packaging guidelines. This is best done by reviewing someone else's package according to the packaging guidelines and lettting me know so I can watch the process. You can refer to these two documents for help:: http://www.fedoraproject.org/wiki/Extras/HowToGetSponsored http://www.fedoraproject.org/wiki/Packaging/ReviewGuidelines SPEC: http://www.perturb.org/tmp/dstat.spec SRPM: http://www.perturb.org/tmp/dstat-0.6.3-5.src.rpm I fixed the above mentioned things. rpmlint is clean on both the srpm and the binary rpm. I don't know how much time I'll have to review another project however. Plus I'm not sure I have the expertise. e3bc24955e52a78166ff297f1a5e14ee dstat-0.6.3-5.src.rpm f833f3e0f8bb34ed50bf1388b7281cb7 dstat.spec a2df5d7fecc0115f8eef84141a068e86 dstat-0.6.3.tar.bz2 You can get rid of this line if you'd like: /usr/lib/rpm/brp-python-bytecompile # FC4 and FC5 run this automatically as you've noted, it gets run automatically on FC4+. If you are building for FC3, that script won't be present at all so the spec file will fail to build. All blockers have been resolved. I'm ready to ACCEPT this package, we just need to get you sponsored. As for reviewing and sponsorship. You get sponsored after you show an understanding for the guidelines. Understanding of the guidelines is all it takes to be able to do reviews. And since you're in the sponsorship process, I'll be watching over your shoulder so I can correct anything that you do wrong. (And you already know how to get into #fedora-extras on IRC which is a great resource for asking questions that might come up during the review process.) Basically, once you are sponsored, you will be able to make changes to any package in the CVS tree and be able to approve other people's packages. So having some examples showing you have the knowledge to do that well is essential. So what's next? All I want to do is get this package in the extras tree. You need sponsorship. The best way to do that is submit a couple of packages then those who have the power to do so can correctly evaluate if you understand the packaging rules. (In reply to comment #16) > So what's next? All I want to do is get this package in the extras tree. And qcomicbook? :-) You can also do reviews. This is explained in Comment #13, Comment #15, and http://www.fedoraproject.org/wiki/Extras/HowToGetSponsored If you'd like, I can unassign this bug and you can seek someone else on fedora-extras to sponsor you. They'll probably review qcomicbook and look at this bug to judge how knowledgable you are and then decide if they want you to do reviews or anything else to show your knowledge. Your call. I'd prefer not to do reviews, I'm not sure if my knowledge is ready for that. Congratulations! APPROVING *** Bug 462034 has been marked as a duplicate of this bug. *** |