[Bug 239892] Review Request: eclipse-checkstyle - a checkstyle plugin for eclipse

Mon May 14 22:05:22 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-checkstyle - a checkstyle plugin for eclipse

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=239892

overholt at redhat.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review-

------- Additional Comments From overholt at redhat.com  2007-05-14 18:05 EST -------
Thanks for the great submission!  There are just a few minor things to clean
up before this can be approved.  They are listed below on lines beginning with
'X'.  The lines beginning with '*' are fine and the ones beginning with '?'
indicate that I have a question.

As we discussed on IRC, it would be nice to move to PDE Build at some point
but I couldn't get it work in my first few attempts.

Ben:  can you take a look at this SRPM with the soon-to-be-attached .spec
patch to see what I did wrong with my package-build call?  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.
* specfile name matches %{name}
X verify source and patches (md5sum matches upstream, know what the patches do)
 - can we include the fetching script in the SRPM directly?
 - I don't think we need the cvs login command and without it it can work
   headless
 - if you use cvs export instead of cvs co it won't include the CVS
   directories
 - I can't do an md5sum match on my own generated tarball but let's see if I
   can after we swith to cvs export
X skim the summary and description for typos, etc.
 - you use both "plugin" and "plug-in" - I'd prefer if we had just one but I'm
   not going to dictate what you use :)
* buildroot fine
* %{?dist} used appropriately
X license text included in package and marked with %doc
 - you should mark the license file(s) with %doc since they're included
* packages meets FHS (http://www.pathname.com/fhs/)
* rpmlint on srpm gives no output
? changelog should be in one of these formats:
 - I think rpmlint might be complaining about the changelog (see below) due to
   the epoch
* Packager tag not used
* Vendor tag not used
* Distribution tag not used
* License used and not Copyright 
* Summary tag should not end in a period
* no PreReq
X specfile is legible
 - it would be nice if we could use wildcards instead of hard versions for
   Eclipse plugin dependencies
 - it would be nice if we could use -D-style properties instead of patching
   the .properties or build.xml files directly
 - please put a little comment above each patch explaining the reasoning
   behind it
 - it would be nice if we didn't have to explicitly refer to
   /usr/share/eclipse but instead could pass it in; perhaps via a -D property
X package successfully compiles and builds on at least x86
 - I had to change the eclipse-pde BR to be >= 3.2.2-whatever.  After that,
   things were fine
* BuildRequires are proper
 - you don't really need all of the BuildRequires as "higher" packages will
   bring some of them in, but they aren't hurting anything
* summary should be a short and concise description of the package
* description expands upon summary
X make sure lines are <= 80 characters
 - most of the lines that are longer could be split with line-continuation \'s
* specfile written in American English
* no -doc sub-package necessary
* packages including libraries should exclude static libraries if possible
* no static libs, no rpath, no config files
* not a standalone GUI app
* no -devel sub-package necessary
X macros used appropriately and consistently
 - you use %{buildroot} and ${TOPDIR} - it would be nice to stick with one
   style
* %makeinstall not used
* install section must begin with rm -rf $RPM_BUILD_ROOT or %{buildroot}
* no locale data
* no use of cp so no need to worry about cp -p
* split Requires(pre,post) into two separate lines
* package should probably not be relocatable
* package contains code
* package owns all directories and files
* there should be no %files duplicates
* file permissions should be okay; %defattrs should be present
* %clean should be present
* %doc files should not affect runtime
* not a webapp
* verify the final provides and requires of the binary RPMs

$ rpm -qp --requires
rpmbuild/RPMS/i386/eclipse-checkstyle-4.0.1-4.fc7.i386.rpm
/bin/sh  
/bin/sh  
antlr  
checkstyle = 0:4.1
checkstyle-optional = 0:4.1
eclipse-platform >= 1:3.2
jakarta-commons-beanutils  
jakarta-commons-logging  
java-gcj-compat >= 1.0.33
java-gcj-compat >= 1.0.33
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  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)  

$ rpm -qp --provides
rpmbuild/RPMS/i386/eclipse-checkstyle-4.0.1-4.fc7.i386.rpm
CheckstylePlugin.jar.so  
NLS.jar.so  
eclipse-checkstyle = 4.0.1-4.fc7

X run rpmlint on the binary RPMs
W: eclipse-checkstyle no-documentation

 - this can be fixed by marking at least the license file(s)

W: eclipse-checkstyle dangling-symlink
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[antlr].jar
/usr/share/java/antlr.jar
W: eclipse-checkstyle symlink-should-be-relative
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[antlr].jar
/usr/share/java/antlr.jar
W: eclipse-checkstyle dangling-symlink
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[commons-logging].jar
/usr/share/java/commons-logging.jar
W: eclipse-checkstyle symlink-should-be-relative
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[commons-logging].jar
/usr/share/java/commons-logging.jar
W: eclipse-checkstyle dangling-symlink
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[commons-beanutils-core].jar
/usr/share/java/commons-beanutils-core.jar
W: eclipse-checkstyle symlink-should-be-relative
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[commons-beanutils-core].jar
/usr/share/java/commons-beanutils-core.jar
W: eclipse-checkstyle dangling-symlink
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[checkstyle-optional-4.1].jar
/usr/share/java/checkstyle-optional-4.1.jar
W: eclipse-checkstyle symlink-should-be-relative
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[checkstyle-optional-4.1].jar
/usr/share/java/checkstyle-optional-4.1.jar
W: eclipse-checkstyle dangling-symlink
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[checkstyle-4.1].jar
/usr/share/java/checkstyle-4.1.jar
W: eclipse-checkstyle symlink-should-be-relative
/usr/share/eclipse/plugins/com.atlassw.tools.eclipse.checkstyle_4.0.1/[checkstyle-4.1].jar
/usr/share/java/checkstyle-4.1.jar

 - I think these are okay to ignore

W: eclipse-checkstyle incoherent-version-in-changelog 0:4.0.1-4 4.0.1-4.fc7

 - see my comment above

SHOULD:
X package should include license text in the package and mark it with %doc
 - needs to be marked with %doc
* package should build on i386
* package should build in mock
 - I didn't try myself but Rob says it does for him

-- 
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.