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@xxxxxxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review- ------- Additional Comments From overholt@xxxxxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review