Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 232709 - Review Request: eclipse-nlspackager - Eclipse NLS package generator
Summary: Review Request: eclipse-nlspackager - Eclipse NLS package generator
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thomas Fitzsimmons
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: 232710
TreeView+ depends on / blocked
 
Reported: 2007-03-16 18:56 UTC by Kyu Lee
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-03-31 23:48:56 UTC
fitzsim: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Kyu Lee 2007-03-16 18:56:57 UTC
Spec URL: http://sourceware.org/eclipse/nls/eclipse-nlspackager.spec
SRPM URL: http://sourceware.org/eclipse/nls/eclipse-nlspackager-0.1.2-1.src.rpm
Description: 

This nlspackager takes in single/multiple language pack zips from eclipse.org
and creates features/plugins such that each feature only contains plugins for
just one language.

Comment 1 Thomas Fitzsimmons 2007-03-16 22:31:06 UTC
Some of my comments apply to the upstream project; since you are the upstream
maintainer, it's best to just fix the problems there.

The gcj_support macro usually controls native-compilation support.  If you just
want to build noarch then remove the gcj_support stuff and leave this line:

BuildRequires: java-devel >= 1.4.2

That said, it's preferable to add full support for native compilation:

http://fedoraproject.org/wiki/NativeJava

MUST:
* package is named appropriately
* it is legal for Fedora to distribute this
X license field matches the actual license.

  - the distributed zip file should contain a license and each source file
    should likely have a license header

  - it's cleaner if the zip file expands to a versioned directory, in this case:
    nlspackager-0.1.2

* license is open source-compatible.
* specfile name matches %{name}
? source and patches verified

  - where does the zip file come from?  I couldn't find a link to it
    from http://wiki.eclipse.org/index.php/Linux_Distributions_Project

* summary and description okay

  - the description could be clearer; try to eliminate the this/that options

  - there's a trailing space in the Summary field

* correct buildroot
* %{?dist} used properly
X license text included in package and marked with %doc
* packages meets FHS (http://www.pathname.com/fhs/)
X rpmlint on <this package>.srpm gives no output

  $ rpmlint /home/fitzsim/rpmbuild/SRPMS/eclipse-nlspackager-0.1.2-1.src.rpm
  W: eclipse-nlspackager non-standard-group Translations

  - use a standard group (see output of rpmlint -i on this package)

  $ rpmlint
/home/fitzsim/rpmbuild/RPMS/noarch/eclipse-nlspackager-0.1.2-1.noarch.rpm 
  W: eclipse-nlspackager non-standard-group Translations
  W: eclipse-nlspackager no-documentation

  - include ChangeLog and LICENSE and probably a README file, marked with %doc

X changelog fine

  - trailing space at the end of the %changelog entry

* Packager tag not used
* Vendor tag not used
* Distribution tag not used
* License and not Copyright used
* Summary tag does not end in a period
* if possible, replace PreReq with Requires(pre) and/or Requires(post)
X specfile is legible

  - why is the copy-platform line commented out?  this should be removed or
    explained

  - pushd blocks are easier to read if they're indented:

    pushd $RPM_BUILD_ROOT%{eclipse_base}
      mkdir plugins
    popd

  - the install section should just use install commands rather than mkdir,
    pushd, popd, cp

  - there's no need for the globbing in the %files section; just specify the
    single file, org.eclipse.linuxtools.nlspackager_%{version}.jar

* package successfully compiles and builds on at least x86
X BuildRequires are proper

  - it would be nice if there were an explicit BuildRequires line for
    eclipse-platform, which provides the eclipse binary (even though
    eclipse-platform is pulled in by eclipse-rcp)

* summary is a short and concise description of the package
* description expands upon summary
* make sure lines are <= 80 characters

  - a few lines in the %build and %install sections should be wrapped

  - you may want to use two-space indentation rather than tabs

* specfile written in American English
* no -doc sub-package necessary
* no static libs
* no rpath
* config files should marked with %config(noreplace)
* not a GUI app
* sub-packages fine
* macros used appropriately and consistently
 - ie. %{buildroot} and %{optflags} vs. $RPM_BUILD_ROOT and $RPM_OPT_FLAGS
* %makeinstall not used
* no locale data
* Requires(pre,post) fine
* package not relocatable
* package contains code
* package owns all directories and files
* no %files duplicates
X file permissions okay; %defattrs present

  - take out the space before the first root

* %clean present
* %doc files do not affect runtime
* not a webapp
* verify the final provides and requires of the binary RPMs

SHOULD:
X package should include license text in the package and mark it with %doc
* package should build on i386
* package should build in mock


Comment 2 Kyu Lee 2007-03-19 16:16:14 UTC
All fixed, execpt

? source and patches verified

  - where does the zip file come from?  I couldn't find a link to it
    from http://wiki.eclipse.org/index.php/Linux_Distributions_Project

- Since this package does not have any official website yet, so the link to a
Zip file will be added to the wiki soon.


Modified files can be found at:


Spec URL: http://sourceware.org/eclipse/nls/eclipse-nlspackager.spec
SRPM URL: http://sourceware.org/eclipse/nls/eclipse-nlspackager-0.1.3-1.src.rpm


Comment 3 Thomas Fitzsimmons 2007-03-19 17:01:41 UTC
(In reply to comment #2)
> All fixed, execpt
> 
> ? source and patches verified
> 
>   - where does the zip file come from?  I couldn't find a link to it
>     from http://wiki.eclipse.org/index.php/Linux_Distributions_Project
> 
> - Since this package does not have any official website yet, so the link to a
> Zip file will be added to the wiki soon.

Is the zip file already hosted somewhere?

> 
> 
> Modified files can be found at:
> 
> 
> Spec URL: http://sourceware.org/eclipse/nls/eclipse-nlspackager.spec
> SRPM URL: http://sourceware.org/eclipse/nls/eclipse-nlspackager-0.1.3-1.src.rpm
> 

I still see these problems:

- a gcj_support fragment: it should be either removed or filled out to full GCJ
  support

- whitespace problems: at the default tab width of 8 characters, several lines
  many lines are longer than 80 characters.  You should wrap them using '\' at
  the end of a line.  For example:

install -p SDK/plugins/org.eclipse.linuxtools.nlspackager_%{version}.jar \
  $RPM_BUILD_ROOT%{eclipse_base}/plugins/

  emacs's untabify mode may help you here.  The summary still has a trailing
  space.

- the description could be clearer

New problems:

- you can tag documentation files relative to the build root.  So you %doc lines
  should be one shorter line:

%doc LICENSE ChangeLog

  Then you can remove this install line:

install -p -m 644 nlspackager/ChangeLog nlspackager/LICENSE
$RPM_BUILD_ROOT%{eclipse_base}/features/org.eclipse.linuxtools.nlspackager_%{version}

  and rpm will do the right thing.

- changelog entries are separated by spaces


Comment 5 Thomas Fitzsimmons 2007-03-19 19:19:13 UTC
The description is much better now, thanks.  Before you build you should remove
this line, since it is unneeded now:

%define gcj_support	1

APPROVED.


Comment 6 Kyu Lee 2007-03-19 20:10:48 UTC
New Package CVS Request
=======================
Package Name: eclipse-nlspackager
Short Description: Eclipse NLS package generator
Owners: klee@redhat.com
Branches: devel, FC-6
InitialCC: overholt@redhat.com,bkonrath@redhat.com,jjohnstn@redhat.com

Comment 7 Warren Togami 2007-03-19 23:34:24 UTC
Please keep ASSIGNED to the reviewer, not the owner.


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