Bug 232710 - Review Request: eclipse-sdk-nls - Eclipse language packs for eclipse-sdk
Review Request: eclipse-sdk-nls - Eclipse language packs for eclipse-sdk
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Package Reviews List
:
Depends On: 232709
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-16 14:59 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-04-01 11:56:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
overholt: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Kyu Lee 2007-03-16 14:59:41 EDT
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 15:00:46 EDT
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 17:16:47 EDT
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 17:30:48 EDT
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 17:33:00 EDT
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 18:12:52 EDT
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 18:59:58 EDT
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 19:21:58 EDT
APPROVED

Thanks, Kyu.  Don't forget to request the CVS with the flag and the standard text.
Comment 9 Kyu Lee 2007-03-20 10:17:36 EDT
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 10:20:07 EDT
(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.