Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Review Request: eclipse-pydev - an Eclipse plugin for working with Python. https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=234031 ------- Additional Comments From ifoox@xxxxxxxxxx 2007-04-01 23:36 EST ------- New files: (people.redhat.com doesn't seem to respond right now, so I'm uploading these files elsewhere and will move them together with the other ones once it's back) http://www.igorfoox.com/misc/fedora/eclipse-pydev-1.3.1-2.src.rpm http://www.igorfoox.com/misc/fedora/eclipse-pydev.spec (In reply to comment #6) > Lines beginning with an X need to be fixed. They're all minor - thanks! :) > > MUST: > * package is named appropriately > * it is legal for Fedora to distribute this > * license field matches the actual license. > * license is open source-compatible. > - I really wish they packed the license text in their VCS. Can you please > ask them to do so? I won't hold up the review on it but it would be great > if it was more prominent than just in their "new in 0.9.8.4" section of > their website. I will ask them to include the license. > * specfile name matches %{name} > X verify source and patches (md5sum matches upstream, know what the patches do) > - I can't duplicate the md5sum of the tarball, but the contents match except > for some timestamps of the generation time > - can we get some comments for the patches? they could also be re-numbered > if you feel like it :) Done. > - should we file the references to 0.9.7.1 issue upstream? I've brought this up before, this seems to be their preferred way of doing things, strange as it is. > * summary and description fine > X correct buildroot > - should be: > %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) Done. > ? %{?dist} isn't used ... should it be? I see no reason why it shouldn't be. Done. > X license text included in package and marked with %doc > - upstream doesn't include license text outside of feature.xml and I don't > want to mark that as %doc; we're okay here > * package meets FHS (http://www.pathname.com/fhs/) > * rpmlint on SRPM (see earlier bug comments) > * changelog format fine > * Packager tag not used > * Vendor tag not used > * Distribution tag not used > * License used and not Copyright > * Summary tag does not end in a period > * if possible, replace PreReq with Requires(pre) and/or Requires(post) > X specfile is legible > - there is an extraneous '#' in the comment about how to generate the tarball > - the instructions for generating the tarball should have an updated VCS tag > (I know it says "substitute the correct version number" but get rid of that > comment and fix the actual tag) > - speaking of the tarball-generating instructions, can we clean them up a > bit? Let's drop the "following the Eclipse Releng process" bit Simplified the whole comment and put the appropriate tag. > - remove pkg_summary and eclipse_name and just type them in directly Done. > * package successfully compiles and builds on at least x86 > X BuildRequires are proper > - why don't we just have BR: eclipse-pde? You're right, that would be better. Done. > X make sure lines are <= 80 characters > - can we split the commons-codec symlinking line? > - also, the lines with symlinking in %install are too long I've split them up, although some are still slightly over 80 chars. > * specfile written in American English > * no -doc sub-package necessary > * no libraries > * no rpath > * no config files > * not a GUI app > * no -devel sub-package necessary > * macros used appropriately and consistently > * no %makeinstall > * install section must begin with rm -rf $RPM_BUILD_ROOT or %{buildroot} > * no locale data > * no cp usage so no need to worry about -p > * split Requires(pre,post) into two separate lines > * package not relocatable > * package contains code > * package owns all directories and files > * no %files duplicates > * file permissions fine > ? %defattrs present > - should there be another '-' after the 'root,root'? I'm not sure, some other specs I've looked at have four components, some have 3. So I've added it, since it seems that's what most do. > * %clean present > * %doc files do not affect runtime > * not a web app > * verify the final provides and requires of the binary RPMs > $ rpm -q --requires -p eclipse-pydev-1.3.1-1.i386.rpm > /usr/bin/rebuild-gcj-db > /usr/bin/rebuild-gcj-db > commons-codec >= 1.3 > eclipse-platform > java-1.5.0-gcj >= 1.5.0 > java-1.5.0-gcj >= 1.5.0 > junit >= 3.8.1 > jython >= 2.2 > libc.so.6 > libc.so.6(GLIBC_2.1.3) > libdl.so.2 > libgcc_s.so.1 > libgcc_s.so.1(GCC_3.0) > libgcj_bc.so.1 > libm.so.6 > libpthread.so.0 > librt.so.1 > libz.so.1 > python > rpmlib(CompressedFileNames) <= 3.0.4-1 > rpmlib(PayloadFilesHavePrefix) <= 4.0-1 > rtld(GNU_HASH) > > $ rpm -q --provides -p eclipse-pydev-1.3.1-1.i386.rpm > ast.jar.so > core.jar.so > parser.jar.so > pydev-debug.jar.so > pydev-jython.jar.so > pydev.jar.so > refactoring.jar.so > eclipse-pydev = 1:1.3.1-1 > > * run rpmlint on the binary RPMs > - see previous bug comments > > SHOULD: > X package should include license text in the package and mark it with %doc > * package should build on i386 > ? package should build in mock > - I haven't tried, but I don't think it'll be a problem (In reply to comment #7) > I did a test build of the SRPM, if failed, after installing Jython*, the the > build went fine, maybe Jython should be a build requirement. You're right, added. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug, or are watching the QA contact. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review