Spec URL: http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/eclipse-checkstyle.spec SRPM URL: http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/eclipse-checkstyle-4.0.1-1.fc7.src.rpm Description: The Eclipse Checkstyle plug-in integrates the Checkstyle Java code auditor into the Eclipse IDE. The plug-in provides real-time feedback to the user about violations of rules that check for coding style and possible error prone code constructs. Note that you will need this script to build the tar: http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/checkout_and_build_tarball.sh This is my first package and am seeking a sponsor.
I'll take this one. I won't have time to do a full review until Monday but here are a few quick thoughts based on a glace at the specfile: - remove Epoch - change Group to something else (yes, I know the eclipse package has that one but run rpmlint on your SRPM and you'll see the W: Wrong Group (or whatever) ... paste the group error message into rpmlint -I <error message here> and you'll see acceptable groups. Just pick one as the Group tag is currently not used for anything useful but this way we can shut up rpmlint.) - remove BuildArch - change the BuildRoot to match the Fedora Guidelines (that may not longer be a mandatory requirement ... just check the guidelines) - I think a lot of your BuildRequires can go away if you pick something like eclipse-pde since it'll bring in most of the others ... building in mock will help flush out problems here - we'll have to get some better way to include the Eclipse jars since the names are so fragile. For SWT especially you'll have to use a symlinked one (I think we ship one) and not the x86_64-only one. - we should add the gcj bits like aot-compile at the end of %install and then the bits in %files ... but that can easily be added later Like I said, I'll do a full review on Monday. I'll assign to myself then.
updated spec: http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/eclipse-checkstyle.spec updated srpm: http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/eclipse-checkstyle-4.0.1-4.fc7.src.rpm - i believe i implemented all of your suggestions and they successfully built in mock with fedora-devel-x86_64-core and fedora-devel-i386-core configurations. - i couldn't find any symlinked jars to use, so i used a wildcard (*) where the architecture specific bit of the eclipse jar is. - it would be very nice to be able to write less brittle eclipse plugin specfiles. symlinks might be one way to do it, and wildcards might be another alternative. are there other options? if we can settle on the best way to do it we should document that methodology in a "Packaging of add-ons for Eclipse" or similar wiki page.
(In reply to comment #2) > - i believe i implemented all of your suggestions and they successfully built in > mock with fedora-devel-x86_64-core and fedora-devel-i386-core configurations. Great! I'll start the review now. > - i couldn't find any symlinked jars to use, so i used a wildcard (*) where the > architecture specific bit of the eclipse jar is. %{_libdir}/eclipse/eclipse/swt-gtk-3.2{,.2}.jar > - it would be very nice to be able to write less brittle eclipse plugin > specfiles. Agreed, but most don't have this problem if they use PDE build. > symlinks might be one way to do it, and wildcards might be another > alternative. are there other options? Other than PDE build, I can't think of anything OTTOMH. > if we can settle on the best way to do > it we should document that methodology in a "Packaging of add-ons for Eclipse" > or similar wiki page. Agreed!
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
Created attachment 154694 [details] specfile patch to use package build instead of canned ant file Ben: this is the output of rpmbuild run on Rob's specfile when patched with the attached patch. preGenerate: [exec] preparing files in /home/andrew/rpmbuild/BUILD/eclipse-checkstyle-4.0.1/CheckstylePlugin for buildfile generation ... [exec] making the 'features' and 'plugins' directories [exec] making symlink: /home/andrew/rpmbuild/BUILD/eclipse-checkstyle-4.0.1/CheckstylePlugin/build/features/com.atlassw.tools.eclipse.checkstyle -> /home/andrew/rpmbuild/BUILD/eclipse-checkstyle-4.0.1/CheckstylePlugin/feature [exec] making symlink: /home/andrew/rpmbuild/BUILD/eclipse-checkstyle-4.0.1/CheckstylePlugin/build/plugins/com.atlassw.tools.eclipse.checkstyle -> /home/andrew/rpmbuild/BUILD/eclipse-checkstyle-4.0.1/CheckstylePlugin/ [exec] done allElements: init: generateScript: [eclipse.buildScript] Some inter-plug-in dependencies have not been satisfied. [eclipse.buildScript] Bundle org.eclipse.cdt.source.linux.gtk.x86: [eclipse.buildScript] Host plug-in org.eclipse.cdt.source_3.1.2.200702271925 has not been found. [eclipse.buildScript] Bundle org.eclipse.cdt.core.linux.x86: [eclipse.buildScript] Host plug-in org.eclipse.cdt.core_[3.1.2,4.0.0) has not been found. [eclipse.buildScript] Bundle org.eclipse.pde.build: [eclipse.buildScript] Another singleton version selected: org.eclipse.pde.build_3.2.1.r321_v20060823 postGenerate: clean: allElements: init: cleanElement: [echo] /home/andrew/rpmbuild/BUILD/eclipse-checkstyle-4.0.1/CheckstylePlugin/build/features/com.atlassw.tools.eclipse.checkstyle BUILD FAILED /usr/share/eclipse/plugins/org.eclipse.pde.build/scripts/build.xml:24: The following error occurred while executing this line: /usr/share/eclipse/plugins/org.eclipse.pde.build/scripts/build.xml:67: The following error occurred while executing this line: /usr/share/eclipse/plugins/org.eclipse.pde.build/templates/package-build/customTargets.xml:80: The following error occurred while executing this line: /usr/share/eclipse/plugins/org.eclipse.pde.build/templates/package-build/customTargets.xml:33: The following error occurred while executing this line: /usr/share/eclipse/plugins/org.eclipse.pde.build/templates/package-build/customTargets.xml:17: The following error occurred while executing this line: /usr/share/eclipse/plugins/org.eclipse.pde.build_3.2.1.r321_v20060823/scripts/genericTargets.xml:57: The following error occurred while executing this line: java.io.FileNotFoundException: /home/andrew/rpmbuild/BUILD/eclipse-checkstyle-4.0.1/CheckstylePlugin/build/features/com.atlassw.tools.eclipse.checkstyle/build.xml (No such file or directory) Total time: 5 seconds
Sorry, I forgot to actually CC Ben. Ben: please take a look at the previous comment/attachment and the comment before that (the top of the review). Thanks.
updated spec: http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/eclipse-checkstyle.spec updated srpm: http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/eclipse-checkstyle-4.0.1-5.fc7.src.rpm - sticking with ant for now since i can't get pde build to work - some lines just won't go shorter than 80 characters - ignoring rpmlint symlink warnings since i think build-jar-repository is the way to go - built in mock fedora-devel-i386-core.cfg fedora-devel-x86_64-core.cfg - works in eclipse
Thanks for making the changes! (In reply to comment #7) > - sticking with ant for now since i can't get pde build to work That's cool. > - some lines just won't go shorter than 80 characters Understood. > - ignoring rpmlint symlink warnings since i think build-jar-repository is the > way to go Agreed. > - built in mock fedora-devel-i386-core.cfg fedora-devel-x86_64-core.cfg Great. > - works in eclipse Works for me, too. I can't duplicate the md5sum of the tarball, but the contents are the same. I'm not going to hold up the review on this, but I'm curious as to why they're not the same. There is just one more thing: remove the epoch from your changelog entries to shut rpmlint up. Once that's done, this will be ready to go.
(In reply to comment #8) <snip> > I can't duplicate the md5sum of the tarball, but the contents are the same. I'm > not going to hold up the review on this, but I'm curious as to why they're not > the same. the cvs export seems to use the current time for directories it creates, but maintains timestamps for the files it creates. i do not know how to fix this. > There is just one more thing: remove the epoch from your changelog entries to > shut rpmlint up. Once that's done, this will be ready to go. oops, that should be fixed now. http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/eclipse-checkstyle.spec http://www.stl.gtri.gatech.edu/rmyers/eclipse-checkstyle/eclipse-checkstyle-4.0.1-6.fc7.src.rpm
(In reply to comment #9) > (In reply to comment #8) > > I can't duplicate the md5sum of the tarball, but the contents are the same. > > [...] > > the cvs export seems to use the current time for directories it creates, but > maintains timestamps for the files it creates. i do not know how to fix this. Okay, don't worry about it. > > There is just one more thing: remove the epoch from your changelog entries to > > shut rpmlint up. Once that's done, this will be ready to go. > > oops, that should be fixed now. Great, thanks. APPROVED! I believe the next step is you need to set the fedora-cvs flag to ?. Thanks, Rob.
(In reply to comment #10) > > Great, thanks. APPROVED! > > I believe the next step is you need to set the fedora-cvs flag to ?. Thanks, Rob. i get this error when i try to set that flag: Flag Modification Denied You tried to request fedora-cvs. Only an authorized user can make this change.
Have you applied for cvsextras access? See http://fedoraproject.org/wiki/PackageMaintainers/Join Once you've been sponsored into the cvsextras group, you'll gain the permissions necessary to set flags on bugzilla tickets.
New Package CVS Request ======================= Package Name: eclipse-checkstyle Short Description: Checkstyle plugin for Eclipse Owners: rob.myers.edu,overholt Branches: F-7 InitialCC:
Package Change Request ====================== Package Name: eclipse-checkstyle New Branches: EL-5
branch done.
eclipse-checkstyle-4.0.1-6.fc7 has been pushed to the Fedora 7 stable repository. If problems still persist, please make note of it in this bug report.