Bug 464016 - Review Request: eclipse-findbugs - Eclipse plugin for FindBugs
Review Request: eclipse-findbugs - Eclipse plugin for FindBugs
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Extras Quality Assurance
:
Depends On: 464014
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-25 17:27 EDT by Jerry James
Modified: 2009-03-13 14:36 EDT (History)
3 users (show)

See Also:
Fixed In Version: 1.3.7-4.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-13 14:36:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
overholt: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jerry James 2008-09-25 17:27:31 EDT
Spec URL: http://jjames.fedorapeople.org/eclipse-findbugs/eclipse-findbugs.spec
SRPM URL: http://jjames.fedorapeople.org/eclipse-findbugs/eclipse-findbugs-1.3.5-1.src.rpm
Description: An Eclipse plugin for the FindBugs Java bug detector.
Comment 3 Andrew Overholt 2009-02-10 10:44:44 EST
Hi Jerry,

This package generally looks okay but it needs to be updated for the new Eclipse plugin packaging guidelines for post-3.4 (we're using the p2 dropins functionality now).  When you have time to update it, let me know and I'll do a formal review.

Andrew
Comment 4 Jerry James 2009-02-10 14:10:46 EST
I have updated it:

http://jjames.fedorapeople.org/eclipse-findbugs/eclipse-findbugs.spec
http://jjames.fedorapeople.org/eclipse-findbugs/eclipse-findbugs-1.3.7-2.src.rpm

The package isn't set up to make use of pdebuild possible out of the box, and I'm apparently not smart enough to figure out what to do about that, so I'm sticking with the ant build for now.

Note that this package depends on the findbugs application itself (bz #464014), which depends on a library named jFormatString (bz #475603).  Both of those are still awaiting review.
Comment 5 Andrew Overholt 2009-02-10 14:20:02 EST
pdebuild was designed to make it easy for us to build packages where no upstream build mechanism is exposed (or easily re-usable outside of upstream's build environment).  If this project provides a usable ant script that works, by all means, use it :)  It's better to stick with the way upstream does it.

I can review this package and I'll see what I can do about the other packages.
Comment 6 Andrew Overholt 2009-03-06 16:39:37 EST
Thanks for the submission.  Here's the review.  Lines beginning with X need attention; those beginning with * are okay.  Other than a few minor things, I think we're good to go once the dependencies are in.

* verify the final provides and requires of the binary RPMs
X make sure lines are <= 80 characters
  - please add a line continuation on line 34 to fix this
* package successfully compiles and builds
* BuildRequires are proper
 - the BR on rcp is unnecessary if pde is already there
 - I recommend dropping the gcj bits 'cause the underlying Eclipse RPMs don't have them and they won't make much of a difference for just this plugin
* macros fine
* 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}
* md5sum matches upstream
* skim the summary and description for typos, etc.
* summary and description good
* correct buildroot
* %{?dist} used correctly
X license text included in package and marked with %doc
  - since upstream doesn't do this, it's not necessary to force it, but maybe you could ask upstream to do so in the future?
X packages meets FHS (http://www.pathname.com/fhs/)
 - this should probably be in %{_datadir}/eclipse/dropins not %{_libdir}/eclipse/dropins
X rpmlint on <this package>.srpm gives no output
 - this seems odd:
  
  findbugs.src:128: E: hardcoded-library-path in ../../lib/findbugs-tools.jar

* changelog format okay
* Summary tag does not end in a period
* no PreReq
* specfile is legible
* specfile written in American English
* no -doc sub-package necessary
* not native, so no rpath, static linking, etc.
* no config files
* not a GUI app
* no -devel necessary
* install section begins with rm -rf $RPM_BUILD_ROOT or %{buildroot}
* no translations so no locale handling
* Requires(pre,post) fine but recommend removing gcj bits so will not be necessary
* package not relocatable
* package contains code
* package owns all directories and files
* no %files duplicates
* file permissions fine
* %clean present
* %doc files do not affect runtime
* not a web app
X run rpmlint on the binary RPMs => no output

 - there are a lot of warnings about non-relative symlinks.  You could fix this by making the symlinks to the stuff in /usr/share/java ../../../ (or whatever) instead
 - there's one warning about a . file:
 
eclipse-findbugs.x86_64: W: hidden-file-or-dir /usr/lib64/eclipse/dropins/findbugs/plugins/edu.umd.cs.findbugs.plugin.eclipse_1.3.7.20081230/.options

 - is that file necessary?

* I verified that it installs and that the findbugs feature is available (after restarting with -clean ... sigh).  I look forward to using it!
Comment 7 Jerry James 2009-03-06 18:42:02 EST
Thanks for the review, Andrew.  I'm excited about getting this into Fedora.

> X make sure lines are <= 80 characters
>   - please add a line continuation on line 34 to fix this

When I do that, I get an embedded newline in the definition of %plugin_dir, which causes the symbolic link commands to break.  If you know of some way to do this without picking up the embedded newline, please let me know.

> * BuildRequires are proper
>  - the BR on rcp is unnecessary if pde is already there

Ah, missed that.  Thanks.

>  - I recommend dropping the gcj bits 'cause the underlying Eclipse RPMs don't
> have them and they won't make much of a difference for just this plugin

Okay, they're gone.

> X license text included in package and marked with %doc
>   - since upstream doesn't do this, it's not necessary to force it, but maybe
> you could ask upstream to do so in the future?

Sure, I'll ask.  This upstream is funny.  My typical interaction with them is to ask a question, which goes unanswered for months.  Then I'll remember that I never got an answer, follow up to it on their mailing list asking for responses, and THEN I'll get a response.

> X packages meets FHS (http://www.pathname.com/fhs/)
>  - this should probably be in %{_datadir}/eclipse/dropins not
> %{_libdir}/eclipse/dropins

I didn't even pick up on the existence of %{_datadir}/eclipse/dropins from the Eclipse guidelines.  I'll have to go look at them again.  Fixed.

> X rpmlint on <this package>.srpm gives no output
>  - this seems odd:
> 
>   findbugs.src:128: E: hardcoded-library-path in ../../lib/findbugs-tools.jar

??  That's from the findbugs SRPM, not the eclipse-findbugs SRPM, right?

X run rpmlint on the binary RPMs => no output

>  - there are a lot of warnings about non-relative symlinks.  You could fix this
> by making the symlinks to the stuff in /usr/share/java ../../../ (or whatever)
> instead

Fixed.

>  - there's one warning about a . file:
> 
> eclipse-findbugs.x86_64: W: hidden-file-or-dir
> /usr/lib64/eclipse/dropins/findbugs/plugins/edu.umd.cs.findbugs.plugin.eclipse_1.3.7.20081230/.options
> 
>  - is that file necessary?

Only at build time.  I don't know why it is getting packaged up.  I have now removed it from the final binary RPM.

New versions are here:
http://jjames.fedorapeople.org/eclipse-findbugs/eclipse-findbugs.spec
http://jjames.fedorapeople.org/eclipse-findbugs/eclipse-findbugs-1.3.7-3.fc10.src.rpm
Comment 8 Andrew Overholt 2009-03-09 09:35:02 EDT
(In reply to comment #7)
> Thanks for the review, Andrew.  I'm excited about getting this into Fedora.
> 
> > X make sure lines are <= 80 characters
> >   - please add a line continuation on line 34 to fix this
> 
> When I do that, I get an embedded newline in the definition of %plugin_dir,
> which causes the symbolic link commands to break.  If you know of some way to do
> this without picking up the embedded newline, please let me know.

Bah, just leave it > 80 :)

> > X license text included in package and marked with %doc
> >   - since upstream doesn't do this, it's not necessary to force it, but maybe
> > you could ask upstream to do so in the future?
> 
> Sure, I'll ask.  This upstream is funny.  My typical interaction with them is to
> ask a question, which goes unanswered for months.  Then I'll remember that I
> never got an answer, follow up to it on their mailing list asking for responses,
> and THEN I'll get a response.

Thanks.  This isn't a blocker.

> > X packages meets FHS (http://www.pathname.com/fhs/)
> >  - this should probably be in %{_datadir}/eclipse/dropins not
> > %{_libdir}/eclipse/dropins
> 
> I didn't even pick up on the existence of %{_datadir}/eclipse/dropins from the
> Eclipse guidelines.  I'll have to go look at them again.  Fixed.

Great, thanks.

> > X rpmlint on <this package>.srpm gives no output
> >  - this seems odd:
> >
> >   findbugs.src:128: E: hardcoded-library-path in ../../lib/findbugs-tools.jar
> 
> ??  That's from the findbugs SRPM, not the eclipse-findbugs SRPM, right?

:)  Sorry.  Your new SRPM gives no rpmlint output.

> X run rpmlint on the binary RPMs => no output
> 
> >  - there are a lot of warnings about non-relative symlinks.  You could fix
> this
> > by making the symlinks to the stuff in /usr/share/java ../../../ (or whatever)
> > instead
> 
> Fixed.

I still get "dangling-relative-symlink" but I think these are all correctly relative now.

Okay, this package is ready to go.  Once the dependencies are in, this can go in.  I'll mark it accepted but obviously it won't build until findbugs is in.  Thanks, Jerry!
Comment 9 Jerry James 2009-03-09 15:27:34 EDT
New Package CVS Request
=======================
Package Name: eclipse-findbugs
Short Description: Eclipse plugin for FindBugs
Owners: jjames
Branches: F-10
InitialCC:
Comment 10 Dennis Gilmore 2009-03-10 16:10:12 EDT
CVS Done
Comment 11 Fedora Update System 2009-03-11 18:51:16 EDT
eclipse-findbugs-1.3.7-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/eclipse-findbugs-1.3.7-4.fc10
Comment 12 Fedora Update System 2009-03-13 14:35:58 EDT
eclipse-findbugs-1.3.7-4.fc10 has been pushed to the Fedora 10 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.