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 232710 - Review Request: eclipse-sdk-nls - Eclipse language packs for eclipse-sdk
Summary: Review Request: eclipse-sdk-nls - Eclipse language packs for eclipse-sdk
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Andrew Overholt
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 232709
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-03-16 18:59 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-04-01 15:56:33 UTC
overholt: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Kyu Lee 2007-03-16 18:59:41 UTC
Spec URL: http://sourceware.org/eclipse/nls/eclipse-sdk-nls-0.1.0-2.src.rpm
SRPM URL: http://sourceware.org/eclipse/nls/eclipse-sdk-nls.spec
Description: 

This eclipse-sdk-nls package contains multiple language translations for
Eclipse SDK.

Comment 1 Kyu Lee 2007-03-16 19:00:46 UTC
Ops, Spec and SRPM URL reversed

Spec URL: http://sourceware.org/eclipse/nls/eclipse-sdk-nls.spec
SRPM URL: http://sourceware.org/eclipse/nls/eclipse-sdk-nls-0.1.0-2.src.rpm

Comment 3 Andrew Overholt 2007-03-19 21:16:47 UTC
A few notes:

. Change the URL to eclipse.org since they're providing the translations
. You don't need to BuildRequire eclipse-rcp or eclipse-platform if
eclipse-nlspackager Requires eclipse-platform
. Add a Requires:  eclipse-rcp
. Summary:  "Eclipse language packs for eclipse-sdk" -> "Eclipse language packs
for the Eclipse SDK"
. Version should match that of the Eclipse SDK base version for which the
translations were done ... 3.2.1 in this case
. Description:  "This eclipse-sdk-nls package contains multiple language
translations for Eclipse SDK." -> "This package contains multiple language
translations for the Eclipse SDK.
. "Portuguese(and Brazil)" -> "Portuguese (and Brazilian Portuguese)"
. "Chinese(Simplified and Traditional)" -> "Chinese (Simplified and Traditional)"
. dump eclipse_name and just use "eclipse" in eclipse_base's definition
. Development/Languages is incorrect but just ignore that for now

Full review text with a few more little things below:

MUST:
* 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}
X verify source and patches (md5sum matches upstream, know what the patches do)
 - the md5sums of NLpack1-eclipse-SDK-3.2.1-gtk.zip and
   NLpack2a-eclipse-SDK-3.2.1-gtk.zip don't match my existing downloads from
   upstream.  I'm re-downloading to verify.  The other two are fine.
X skim the summary and description for typos, etc.
  -> see comments above
* correct buildroot
* %{?dist} used properly
* license text included in package and marked with %doc
* packages meets FHS (http://www.pathname.com/fhs/)
* rpmlint on eclipse-sdk-nls-0.1.0-3.src.rpm gives no output
* changelog format fine
* Packager tag not used
* Vendor tag not used
* Distribution tag not used
* License and not Copyright used
* Summary tag should not end in a period
* no PreReq
X specfile is legible
 - see comments above.
* package successfully compiles and builds on at least x86
X BuildRequires are proper
  - see comments above
X summary and description are fine
  - see comments above
X make sure lines are <= 80 characters
  - two of the lines in the nlspackager app call have extra spaces that put
    them over 80 characters
* specfile written in American English
* no -doc sub-package necessary
* no libraries
* no rpath
* no config files
* not a GUI app
* no -devel subpackage necessary
* macros used appropriately and consistently
* %makeinstall not used
* no locale data in the traditional sense
* cp -p used
* no Requires(pre,post)
* package is not relocatable
* package contains acceptable content
X package owns all directories and files
  - package needs to Require eclipse-rcp
* no %files duplicates
* file permissions fine
* %clean present
* %doc files do not affect runtime
* not a web app
? verify the final provides and requires of the binary RPMs
  . I have yet to do this
? run rpmlint on the binary RPMs
  . I have yet to do this

SHOULD:
* package should include license text in the package and mark it with %doc
* package should build on i386
? package should build in mock
  . haven't tried but I don't anticipate any problems

Comment 4 Kyu Lee 2007-03-19 21:30:48 UTC
MUST:
X verify source and patches (md5sum matches upstream, know what the patches do)
 - the md5sums of NLpack1-eclipse-SDK-3.2.1-gtk.zip and
   NLpack2a-eclipse-SDK-3.2.1-gtk.zip don't match my existing downloads from
   upstream.  I'm re-downloading to verify.  The other two are fine.

X skim the summary and description for typos, etc.
  -> see comments above

Done

X specfile is legible
 - see comments above.

Done

X BuildRequires are proper
  - see comments above

Done


X summary and description are fine
  - see comments above

Done


X make sure lines are <= 80 characters
  - two of the lines in the nlspackager app call have extra spaces that put
    them over 80 characters

Done


X package owns all directories and files
  - package needs to Require eclipse-rcp

Done

? verify the final provides and requires of the binary RPMs
  . I have yet to do this
? run rpmlint on the binary RPMs
  . I have yet to do this

? package should build in mock
  . haven't tried but I don't anticipate any problems

Comment 5 Andrew Overholt 2007-03-19 21:33:00 UTC
Thanks, just a few more things:

. The md5sums are still incorrect for the two zips.  You should get them again
and verify and exploded SRPM versus the upstream zips.

. You need to update the changelog to reflect the new version.

My build is almost done so I can verify the provides and requires and rpmlint of
the binary RPMs.

Comment 6 Andrew Overholt 2007-03-19 22:12:52 UTC
You need to add a Requires on eclipse-rcp for each of the language RPMs.  Also,
try running dos2unix over the html files.  Otherwise, things seem fine with the
build and with the binary RPMs themselves.

Remaining:

. add Requires on eclipse-rcp for each binary package
. add BR: dos2unix and run it over the problematic files
. update the changelog
. re-pack the SRPM with the updated zips.  This is what I get for md5sums:

3124c1065754acdfe81966f54f7da94c  NLpack1-eclipse-SDK-3.2.1-gtk.zip
bf3067667799953bb5f941c4a20a9c07  NLpack2a-eclipse-SDK-3.2.1-gtk.zip
8f142912fc05b121c8591a0ea2d4a10f  NLpack2-eclipse-SDK-3.2.1-gtk.zip
358891610a775f9e68f08b37c9a4dc07  NLpackBidi-eclipse-SDK-3.2.1-gtk.zip

Those should match what gets installed into rpmbuild/SOURCES after an rpm -i on
the SRPM.

Comment 7 Kyu Lee 2007-03-19 22:59:58 UTC
fixed.

. add Requires on eclipse-rcp for each binary package

Done

. add BR: dos2unix and run it over the problematic files

Done

. update the changelog

Done

. re-pack the SRPM with the updated zips.  

Done


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

Comment 8 Andrew Overholt 2007-03-19 23:21:58 UTC
APPROVED

Thanks, Kyu.  Don't forget to request the CVS with the flag and the standard text.

Comment 9 Kyu Lee 2007-03-20 14:17:36 UTC
New Package CVS Request
=======================
Package Name: eclipse-sdk-nls
Short Description: Eclipse language packs for eclipse-sdk
Owners: klee@redhat.com
Branches: devel, FC-6
InitialCC: overholt@redhat.com,bkonrath@redhat.com,jjohnstn@redhat.com

Comment 10 Kyu Lee 2007-03-20 14:20:07 UTC
(In reply to comment #8)
> APPROVED
> 
> Thanks, Kyu.  Don't forget to request the CVS with the flag and the standard text.

Can you set the fedora-cvs flag for me? My account has some problems.


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