Bug 232709 - Review Request: eclipse-nlspackager - Eclipse NLS package generator
Review Request: eclipse-nlspackager - Eclipse NLS package generator
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Thomas Fitzsimmons
Fedora Package Reviews List
:
Depends On:
Blocks: 232710
  Show dependency treegraph
 
Reported: 2007-03-16 14:56 EDT by Kyu Lee
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-31 19:48:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
fitzsim: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Kyu Lee 2007-03-16 14:56:57 EDT
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 18:31:06 EDT
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 12:16:14 EDT
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 13:01:41 EDT
(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 15:19:13 EDT
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 16:10:48 EDT
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 19:34:24 EDT
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.