[Bug 239892] Review Request: eclipse-checkstyle - a checkstyle plugin for eclipse
bugzilla at redhat.com
bugzilla at redhat.com
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.
More information about the package-review
mailing list