Bug 239892 - Review Request: eclipse-checkstyle - a checkstyle plugin for eclipse
Review Request: eclipse-checkstyle - a checkstyle plugin for eclipse
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-11 19:07 EDT by rob
Modified: 2007-11-30 17:12 EST (History)
1 user (show)

See Also:
Fixed In Version: 4.0.1-6.fc7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-25 19:24:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
overholt: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
specfile patch to use package build instead of canned ant file (3.52 KB, patch)
2007-05-14 18:34 EDT, Andrew Overholt
no flags Details | Diff

  None (edit)
Description rob 2007-05-11 19:07:59 EDT
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.
Comment 1 Andrew Overholt 2007-05-12 07:53:35 EDT
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.
Comment 2 rob 2007-05-12 15:31:15 EDT
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.
Comment 3 Andrew Overholt 2007-05-14 14:58:55 EDT
(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!
Comment 4 Andrew Overholt 2007-05-14 18:05:19 EDT
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
Comment 5 Andrew Overholt 2007-05-14 18:34:09 EDT
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
Comment 6 Andrew Overholt 2007-05-14 18:35:06 EDT
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.
Comment 7 rob 2007-05-15 13:26:44 EDT
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
Comment 8 Andrew Overholt 2007-05-15 16:20:53 EDT
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.
Comment 9 rob 2007-05-16 15:44:59 EDT
(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
Comment 10 Andrew Overholt 2007-05-16 15:51:21 EDT
(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.
Comment 11 rob 2007-05-16 16:10:13 EDT
(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.
Comment 12 Jason Tibbitts 2007-05-16 17:20:35 EDT
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.
Comment 13 rob 2007-05-18 16:48:00 EDT
New Package CVS Request
=======================
Package Name: eclipse-checkstyle
Short Description: Checkstyle plugin for Eclipse
Owners: rob.myers@gtri.gatech.edu,overholt@redhat.com
Branches: F-7
InitialCC: 
Comment 14 rob 2007-06-22 16:10:35 EDT
Package Change Request
======================
Package Name: eclipse-checkstyle
New Branches: EL-5
Comment 15 Kevin Fenzi 2007-06-25 15:32:18 EDT
branch done.
Comment 16 Fedora Update System 2007-06-25 19:24:08 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.