Bug 444512 - (eclipse-eclemma) Review Request: eclipse-eclemma - EMMA plugin for Eclipse
Review Request: eclipse-eclemma - EMMA plugin for Eclipse
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On: 444511
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-28 15:53 EDT by Andrew Overholt
Modified: 2009-01-06 09:21 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-06 09:21:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Andrew Overholt 2008-04-28 15:53:23 EDT
Spec URL: http://fedorapeople.org/~overholt/eclipse-eclemma.spec
SRPM URL: http://fedorapeople.org/~overholt/eclipse-eclemma-1.3.1-1.fc8.src.rpm
Description: EclEmma is a Java code coverage tool for Eclipse based on the EMMA Java code coverage tool.  Features include launching from within the IDE,
coverage analysis summaries, and highlighting in Java source code
editors.
Comment 1 Anthony Green 2008-11-10 17:12:45 EST
I get the following when I try to build on F10...

/var/tmp/rpm-tmp.1r9udd: line 29: /usr/share/eclipse/buildscripts/pdebuild: No such file or directory

AG
Comment 2 Andrew Overholt 2008-11-12 09:02:28 EST
Alex updated this for me and I apparently never uploaded it.  Builds for me with 3.4 and matches new packaging guidelines for file locations:

Spec URL: http://fedorapeople.org/~overholt/eclipse-eclemma.spec
SRPM URL: http://fedorapeople.org/~overholt/eclipse-eclemma-1.3.2-1.fc9.src.rpm

There are some rpmlint warnings but I believe they're all waivable.
Comment 3 Orcan Ogetbil 2008-12-03 00:18:16 EST
The review arrived finally. Nothing serious, just few small things:

* First you should close bug #444511

* This package is only for F-10+, right?

* To simplify the code, you can use
    %define install_loc  %{_datadir}/eclipse/dropins/eclemma
and update everything accordingly. This is a suggestion, not a requirement.

* You are now not owning the directory
    %{install_loc}/eclemma
With the above suggestion you can just use
    %{install_loc}
in the files section.

* rpmlint says:
    eclipse-eclemma.noarch: W: no-documentation
       Please add those about.html files (rename them), and at least the license.html and faq.html files to %doc
    eclipse-eclemma.noarch: W: dangling-symlink /usr/share/eclipse/dropins/eclemma/eclipse/plugins/com.mountainminds.eclemma.core_1.3.2/emma.jar /usr/share/java/emma.jar
        This can be ignored.
    eclipse-eclemma.noarch: W: symlink-should-be-relative /usr/share/eclipse/dropins/eclemma/eclipse/plugins/com.mountainminds.eclemma.core_1.3.2/emma.jar /usr/share/java/emma.jar
        This should be fixed.
    eclipse-eclemma.noarch: W: hidden-file-or-dir /usr/share/eclipse/dropins/eclemma/eclipse/plugins/com.mountainminds.eclemma.core_1.3.2/.options
        Is this file required?
    eclipse-eclemma.src: W: strange-permission get-eclemma.sh 0775
        Please use 644 for source files.

* The file 
    ./com.mountainminds.eclemma.core/emma.jar
needs to be removed in the %prep

* The license file says:
    "The user documentation contains example code taken from the Apache Jakarta Commons project, provided under the terms and conditions of the Apache License Version 2.0. "
       Shall we include ASL 2.0 in the license tag?

* Each package must consistently use macros, as described in the macros section of Packaging Guidelines .
You are using $RPM_BUILD_ROOT at certain points and %{buildroot} on others. You should stay consistent.
Comment 4 Andrew Overholt 2008-12-03 11:25:01 EST
(In reply to comment #3)
> The review arrived finally. Nothing serious, just few small things:

Thanks!

> * First you should close bug #444511

Done.
 
> * This package is only for F-10+, right?

Yes.  I updated the BRs/Rs on eclipse stuff to make it F-10+.

> * To simplify the code, you can use
>     %define install_loc  %{_datadir}/eclipse/dropins/eclemma
> and update everything accordingly. This is a suggestion, not a requirement.

Thanks, done.

> * You are now not owning the directory
>     %{install_loc}/eclemma
> With the above suggestion you can just use
>     %{install_loc}
> in the files section.

Yup, done.

> * rpmlint says:
>     eclipse-eclemma.noarch: W: no-documentation
>        Please add those about.html files (rename them), and at least the
> license.html and faq.html files to %doc

Done.

>     eclipse-eclemma.noarch: W: dangling-symlink
> /usr/share/eclipse/dropins/eclemma/eclipse/plugins/com.mountainminds.eclemma.core_1.3.2/emma.jar
> /usr/share/java/emma.jar
>         This can be ignored.
>     eclipse-eclemma.noarch: W: symlink-should-be-relative
> /usr/share/eclipse/dropins/eclemma/eclipse/plugins/com.mountainminds.eclemma.core_1.3.2/emma.jar
> /usr/share/java/emma.jar
>         This should be fixed.

I think we'd be better off fixing build-jar-repository.  If you want, I'll make it a big long ln -s ../../../java/emma.jar instead, but build-jar-repository seems cleaner.

>     eclipse-eclemma.noarch: W: hidden-file-or-dir
> /usr/share/eclipse/dropins/eclemma/eclipse/plugins/com.mountainminds.eclemma.core_1.3.2/.options
>         Is this file required?

It may be just a build-time requirement but it doesn't harm anything to keep it.

>     eclipse-eclemma.src: W: strange-permission get-eclemma.sh 0775
>         Please use 644 for source files.

This is a shell script to fetch the source.  Do you still want it changed?

> * The file 
>     ./com.mountainminds.eclemma.core/emma.jar
> needs to be removed in the %prep

Since I was symlinking over it I didn't think it was a big deal but I've added an explicit removal before the ln line now.

> * The license file says:
>     "The user documentation contains example code taken from the Apache Jakarta
> Commons project, provided under the terms and conditions of the Apache License
> Version 2.0. "
>        Shall we include ASL 2.0 in the license tag?

I don't know.  I guess it can't hurt :)  I've added it.

> * Each package must consistently use macros, as described in the macros section
> of Packaging Guidelines .
> You are using $RPM_BUILD_ROOT at certain points and %{buildroot} on others. You
> should stay consistent.

Fixed.

Updated spec and SRPM:

http://fedorapeople.org/~overholt/eclipse-eclemma.spec
http://fedorapeople.org/~overholt/eclipse-eclemma-1.3.2-2.fc10.src.rpm
Comment 5 Orcan Ogetbil 2008-12-03 13:21:24 EST
(In reply to comment #4)
> (In reply to comment #3)
> 
> >     eclipse-eclemma.noarch: W: dangling-symlink
> > /usr/share/eclipse/dropins/eclemma/eclipse/plugins/com.mountainminds.eclemma.core_1.3.2/emma.jar
> > /usr/share/java/emma.jar
> >         This can be ignored.
> >     eclipse-eclemma.noarch: W: symlink-should-be-relative
> > /usr/share/eclipse/dropins/eclemma/eclipse/plugins/com.mountainminds.eclemma.core_1.3.2/emma.jar
> > /usr/share/java/emma.jar
> >         This should be fixed.
> 
> I think we'd be better off fixing build-jar-repository.  If you want, I'll make
> it a big long ln -s ../../../java/emma.jar instead, but build-jar-repository
> seems cleaner.
> 

I never saw a usage of build-jar-repository in the %install section before. Well, the guidelines state that the symlink must be relative. I know it's ugly but that's the way it should be.

> 
> >     eclipse-eclemma.src: W: strange-permission get-eclemma.sh 0775
> >         Please use 644 for source files.
> 
> This is a shell script to fetch the source.  Do you still want it changed?
> 

I don't think this is a MUST. But I recommend this in reviews. You can always call the fetch script via
   sh get-eclemma.sh

> > * The license file says:
> >     "The user documentation contains example code taken from the Apache Jakarta
> > Commons project, provided under the terms and conditions of the Apache License
> > Version 2.0. "
> >        Shall we include ASL 2.0 in the license tag?
> 
> I don't know.  I guess it can't hurt :)  I've added it.
> 

So this is my biggest concern. The ASL 2.0 license clearly states that (clause 4.1):
   "You must give any other recipients of the Work or Derivative Works a copy
    of this License"
This package does not include the ASL 2.0 license. I think you should 
EITHER
download it and put it as a source in the SPEC file and then include it in the %doc[1] 
OR just remove 
   com.mountainminds.eclemma.doc/pages/images/annotations.png
and the relevant bits from
   com.mountainminds.eclemma.doc/pages/annotations.html

But in either case, upstream should be notified about this issue.

[1] http://www.apache.org/licenses/LICENSE-2.0
Comment 6 Andrew Overholt 2008-12-03 13:36:36 EST
http://fedorapeople.org/~overholt/eclipse-eclemma.spec
http://fedorapeople.org/~overholt/eclipse-eclemma-1.3.2-3.fc10.src.rpm

All issues fixed and I included a copy of the ASL 2.0 license text.  I'll send an email upstream about it.
Comment 7 Orcan Ogetbil 2008-12-03 14:05:17 EST
Thanks for the update! Review done.

--------------------------------------------------------
This package (eclipse-eclemma) has been APPROVED by oget
--------------------------------------------------------
Comment 8 Andrew Overholt 2008-12-03 15:06:46 EST
New Package CVS Request
=======================
Package Name: eclipse-eclemma
Short Description: Java code coverage tool plugin for Eclipse
Owners: overholt
Branches:
InitialCC:
Comment 9 Kevin Fenzi 2008-12-03 19:57:35 EST
cvs done.
Comment 10 Alexander Kurtakov 2009-01-06 09:21:13 EST
This package has been reviewed and imported.

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