Bug 470792 - Review Request: eclipse-shelled - Shell script editor plugin for Eclipse
Summary: Review Request: eclipse-shelled - Shell script editor plugin for Eclipse
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Orcan Ogetbil
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-11-10 10:32 UTC by Alexander Kurtakov
Modified: 2008-12-03 17:35 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-12-03 17:35:03 UTC
Type: ---
Embargoed:
oget.fedora: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Alexander Kurtakov 2008-11-10 10:32:50 UTC
Spec URL:
http://akurtakov.fedorapeople.org/eclipse-shelled.spec
SRPM URL:
http://akurtakov.fedorapeople.org/eclipse-shelled-1.0.3-1.fc9.src.rpm

Description: ShellEd is a shell script editor for Eclipse. 
The great benefit of this plugin is the integration of man page information
 for content assist and text hover.

Comment 1 Orcan Ogetbil 2008-11-25 06:46:21 UTC
I made a quick check. There is this issue that needs to be corrected before we can do a full review:

* Source0 must be given in full URL from upstream's website.


A few other things that I caught were:

* For the Group tag pick something from
   /usr/share/doc/rpm-*/GROUPS
Development/Tools or Development/Languages maybe?

* %eclipse_base must be
   %define eclipse_base %{_datadir}/eclipse
Check: http://fedoraproject.org/wiki/Packaging/EclipsePlugins
Otherwise the package will not build.

* No %doc files? Check the source thoroughly. Please list every applicable file in %doc.

* Use CPL for the license

* There shouldn't be both spaces and tabs in the same SPEC file. Please use one or the other. rpmlint is a good application which will warn you about such things and many other issues.

Comment 2 Alexander Kurtakov 2008-11-25 09:40:34 UTC
Updated with the comments 

Spec URL:
http://akurtakov.fedorapeople.org/eclipse-shelled.spec
SRPM URL:
http://akurtakov.fedorapeople.org/eclipse-shelled-1.0.3-1.fc9.src.rpm


(In reply to comment #1)
> I made a quick check. There is this issue that needs to be corrected before we
> can do a full review:
> 
> * Source0 must be given in full URL from upstream's website.
Done: Fetch script added.

> 
> 
> A few other things that I caught were:
> 
> * For the Group tag pick something from
>    /usr/share/doc/rpm-*/GROUPS
> Development/Tools or Development/Languages maybe?
Done: Development/Tools

> 
> * %eclipse_base must be
>    %define eclipse_base %{_datadir}/eclipse
> Check: http://fedoraproject.org/wiki/Packaging/EclipsePlugins
> Otherwise the package will not build.
Not valid for Fedora 10. 

> 
> * No %doc files? Check the source thoroughly. Please list every applicable file
> in %doc.
There is really no doc provided.

> 
> * Use CPL for the license
Done.

> 
> * There shouldn't be both spaces and tabs in the same SPEC file. Please use one
> or the other. rpmlint is a good application which will warn you about such
> things and many other issues.
Fixed. Rpmlint do not show any warning.

Comment 3 Orcan Ogetbil 2008-11-26 02:11:51 UTC
* Whenever you are uploading a new release, don't forget the bump the release number. This applies even during the review process.

* rpmlint says
     eclipse-shelled.noarch: W: no-documentation
     eclipse-shelled.src: W: strange-permission fetch-shelled.sh 0764

   ** At least these files need to go to %doc. 
         ./com.something.eclipse.shelled-feature/license.html
         ./com.something.eclipse.shelled-feature/cpl-v10.html
         ./com.something.eclipse.shelled.resources/about.html
     (Check the sample spec file at the eclipse guidelines)
     The other about.html files have the same content. 
     Other than this, what are all those manpages for?

   ** Afaik, we use 644 for source files.

* You do not need
     BuildRequires:    zip
     BuildRequires:    lzma

* We prefer %defattr(-,root,root,-)

* Use the %{version} macro whenever applicable (e.g. source0).

Comment 4 Alexander Kurtakov 2008-11-26 09:36:34 UTC
Updated :

Spec URL:
http://akurtakov.fedorapeople.org/eclipse-shelled.spec
SRPM URL:
http://akurtakov.fedorapeople.org/eclipse-shelled-1.0.3-2.fc9.src.rpm

(In reply to comment #3)
> * Whenever you are uploading a new release, don't forget the bump the release
> number. This applies even during the review process.
> 
> * rpmlint says
>      eclipse-shelled.noarch: W: no-documentation
>      eclipse-shelled.src: W: strange-permission fetch-shelled.sh 0764
> 
>    ** At least these files need to go to %doc. 
>          ./com.something.eclipse.shelled-feature/license.html
>          ./com.something.eclipse.shelled-feature/cpl-v10.html
>          ./com.something.eclipse.shelled.resources/about.html
>      (Check the sample spec file at the eclipse guidelines)
>      The other about.html files have the same content. 
>      Other than this, what are all those manpages for?
> 
>    ** Afaik, we use 644 for source files.
Fixed.

> 
> * You do not need
>      BuildRequires:    zip
>      BuildRequires:    lzma
Fixed.

> 
> * We prefer %defattr(-,root,root,-)
Fixed.

> 
> * Use the %{version} macro whenever applicable (e.g. source0).
Fixed.

Comment 5 Alexander Kurtakov 2008-11-27 00:05:10 UTC
Updated :

Spec URL:
http://akurtakov.fedorapeople.org/eclipse-shelled.spec
SRPM URL:
http://akurtakov.fedorapeople.org/eclipse-shelled-1.0.3-3.fc9.src.rpm

FIx %%doc handling.

Comment 6 Orcan Ogetbil 2008-11-27 14:41:26 UTC
Thanks! Good to go.

---------------------------------------------------------
This package (eclipse-shelled) has been APPROVED by oget.
---------------------------------------------------------

Comment 7 Alexander Kurtakov 2008-11-27 15:03:41 UTC
New Package CVS Request
=======================
Package Name: eclipse-shelled
Short Description: Eclipse Shell script editor
Owners: akurtakov
Branches: F-10
InitialCC: akurtakov

Comment 8 Kevin Fenzi 2008-12-01 20:53:38 UTC
cvs done.


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