[Bug 234031] Review Request: eclipse-pydev - an Eclipse plugin for working with Python.

bugzilla at redhat.com bugzilla at redhat.com
Mon Apr 2 03:36:35 UTC 2007


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 at redhat.com  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.




More information about the Fedora-package-review mailing list