Bug 456972 - Review Request: eclipse-nls - Babel translations for Eclipse
Summary: Review Request: eclipse-nls - Babel translations for Eclipse
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jens Petersen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-07-29 00:12 UTC by Sean Flanigan
Modified: 2008-09-11 06:48 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-09-11 06:48:51 UTC
Type: ---
petersen: fedora-review+
petersen: fedora-cvs+


Attachments (Terms of Use)
eclipse-nls.spec.patch-1 (1.34 KB, patch)
2008-09-10 01:56 UTC, Jens Petersen
no flags Details | Diff
eclipse-nls.spec.patch-2 (1.59 KB, patch)
2008-09-10 08:00 UTC, Jens Petersen
no flags Details | Diff

Description Sean Flanigan 2008-07-29 00:12:29 UTC
Spec URL: http://seanf.fedorapeople.org/eclipse-nls/0.2.0-0.1/eclipse-nls.spec
SRPM URL: http://seanf.fedorapeople.org/eclipse-nls/0.2.0-0.1/eclipse-nls-0.2.0-0.1.20080720snap.fc10.src.rpm
Description: Babel language packs include translations for the Eclipse platform and other Eclipse-related packages.

This project obsoletes eclipse-sdk-nls 3.2.1.  The Babel project is essentially the successor to the old Eclipse language packs from IBM.

About the source: the Babel project hasn't made any releases yet, just an Eclipse update site with "nightly" snapshots.  I have written a script <http://seanf.fedorapeople.org/eclipse-nls/0.2.0-0.1/fetch-babel.sh> that fetches the latest version from the update site.  The source is 40MB, and will only be updated when I fetch a new snapshot, so you might not want to download every new SRPM during the review process.

rpmlint complains about my Obsoletes: line, but it looks like a bug to me: https://bugzilla.redhat.com/show_bug.cgi?id=456843

Comment 1 Jens Petersen 2008-07-29 00:22:32 UTC
Just to note that this is Sean's first package and I am sponsoring him. :)
But if someone with more experience on eclipse packaging wants to review that is
ok by me.

Andrew, if you have any initial comments or input on the packaging that would be
useful.


Comment 2 Andrew Overholt 2008-07-29 13:26:12 UTC
This isn't really Eclipse packaging since you're just dropping files into the
correct place, so I think it's fine for you to review it, Jens.  fetch-babel.sh
will obviously have to be updated once 3.4 is in rawhide (sorry, I'm working on
it ... stupid ppc64 is killing me) and will perhaps require an s/x86/%{arch} but
otherwise I think it's okay from the Eclipse side of things.  I don't know all
the fancy langpack specfile tricks you're using but if you say they're good,
I'll buy it :)

Comment 3 Sean Flanigan 2008-07-29 23:40:10 UTC
Ah, I see I left the wrong path for ECLIPSE_BIN in fetch-babel.sh, so yes, that
will need changing when 3.4 is in.  Was there something else, too?

As for s/x86/%{arch}, I don't believe Babel has different versions for different
architectures - I left in the "-p2.arch x86" because I assumed the update
manager required it.  (I don't suppose P2 understands noarch?).  I certainly
hope we don't need to download everything multiple times.  It already takes a
couple of hours to download 150MB.

Andrew, what do you think of the location I'm storing the langpacks in:
%{eclipse_base}/dropins/babel-$LOCALE/eclipse/{plugins,features}.  I gather from
your post to linux-distros-dev that we would probably use dropins for this sort
of thing, at least in Fedora 10.  I'm extremely new to P2, but I think it's
neater to use a subdir/subdirs of dropins, rather than dropins itself.   And it
makes the %files section nice and short!


Comment 4 Sean Flanigan 2008-07-30 00:15:10 UTC
I should probably mention that putting Babel into dropins (eg via my package),
and then installing Babel through the update manager too, tends to break
Ganymede: <https://bugs.eclipse.org/bugs/show_bug.cgi?id=242327>.  Eclipse's
current integration builds are okay though, so I'm hoping this will change.


Comment 5 Sean Flanigan 2008-08-01 04:14:36 UTC
I'm beginning to think that this little optimisation/hack in my spec file might
be a problem:
----------------------
# Disable repacking of jars, since it takes forever for all the little jars, 
# and we don't need multilib anyway:
%define __jar_repack %{nil}
----------------------

since it looks to me as if eclipse 3.4 is doing away with noarch support
(replacing %{_datadir}/eclipse/ with %{_libdir}/eclipse/).

I don't really understand why we repack jars, except that it's something to do
with multilib support.  I was hoping to avoid repacking 8,962 (really!) jars by
making eclipse-nls noarch.

Do I need to make eclipse-nls multi-arch?  And does anyone know if we really
need to be repacking jars in that case?


Comment 6 Jens Petersen 2008-08-01 04:24:44 UTC
> since it looks to me as if eclipse 3.4 is doing away with noarch support
> (replacing %{_datadir}/eclipse/ with %{_libdir}/eclipse/).

I think we need %{_datadir}/eclipse/.  Andrew?

> Do I need to make eclipse-nls multi-arch?  And does anyone know if we really
> need to be repacking jars in that case?

It should be noarch, surely, but may still need to repack jar?

Comment 7 Jens Petersen 2008-08-01 06:27:03 UTC
Sean pointed out that this is discussed in:

https://www.redhat.com/archives/fedora-devel-java-list/2008-June/msg00031.html

Comment 8 Sean Flanigan 2008-08-11 03:28:27 UTC
New Spec/SRPM/fetcher script are all here:

http://seanf.fedorapeople.org/eclipse-nls/0.2.0-0.2/

* Mon Aug 11 2008 Sean Flanigan <sflaniga@redhat.com> - 0.2.0-0.2.20080807snap
- Fixed version in changelog
- Updated snapshot of Babel translation plugins
- Changed code for Hebrew to he (not iw); changed fetch-babel.sh to compensate
- Renamed eclipse_base macro to eclipse_data

Updates and notes:
Andrew's latest package for Fedora Eclipse eclipse-*-3.4.0-18 has proper dropins support for %{_datadir}/eclipse/.

I noticed that eclipse.spec disables jar repacking, so I'm going to assume I can get away with it too, at least for eclipse-nls's very simple needs (no compilation).

https://bugs.eclipse.org/bugs/show_bug.cgi?id=242327 (mentioned previously) doesn't seem to happen with Fedora Eclipse, for some reason.  (The splash screen flashes on and off for about a minute after the update, but then it settles down.)

Also, I was having trouble getting Fedora Eclipse to pick up the translations from any directory at all, but then it started happening with old revisions (eg eclipse-*.3.4.0-15) that *used to work*.  I spent a lot of time trying to work out what was wrong, but got nowhere.  Today I tried the rawhide version of eclipse-*-3.4.0-18.fc10.i386.rpm and everything is working again, so I'd better move things along!

Comment 9 Jens Petersen 2008-09-08 08:04:39 UTC
rpmlint says:

eclipse-nls.src:36: W: unversioned-explicit-obsoletes \
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
eclipse-nls-uk.noarch: W: no-documentation
eclipse-nls-uk.noarch: W: obsolete-not-provided eclipse-sdk-nls-uk
eclipse-nls-fr.noarch: W: no-documentation
eclipse-nls-fr.noarch: W: obsolete-not-provided eclipse-sdk-nls-fr
eclipse-nls-pt.noarch: W: no-documentation
eclipse-nls-pt.noarch: W: obsolete-not-provided eclipse-sdk-nls-pt
eclipse-nls-nl.noarch: W: no-documentation
eclipse-nls-nl.noarch: W: obsolete-not-provided eclipse-sdk-nls-nl
eclipse-nls-no.noarch: W: no-documentation
eclipse-nls-no.noarch: W: obsolete-not-provided eclipse-sdk-nls-no
eclipse-nls-ar.noarch: W: no-documentation
eclipse-nls-ar.noarch: W: obsolete-not-provided eclipse-sdk-nls-ar
eclipse-nls-ko.noarch: W: no-documentation
eclipse-nls-ko.noarch: W: obsolete-not-provided eclipse-sdk-nls-ko
eclipse-nls-ro.noarch: W: no-documentation
eclipse-nls-ro.noarch: W: obsolete-not-provided eclipse-sdk-nls-ro
eclipse-nls-sv.noarch: W: no-documentation
eclipse-nls-sv.noarch: W: obsolete-not-provided eclipse-sdk-nls-sv
eclipse-nls-hu.noarch: W: no-documentation
eclipse-nls-hu.noarch: W: obsolete-not-provided eclipse-sdk-nls-hu
eclipse-nls-it.noarch: W: no-documentation
eclipse-nls-it.noarch: W: obsolete-not-provided eclipse-sdk-nls-it
eclipse-nls-ja.noarch: W: no-documentation
eclipse-nls-ja.noarch: W: obsolete-not-provided eclipse-sdk-nls-ja
eclipse-nls-fi.noarch: W: no-documentation
eclipse-nls-fi.noarch: W: obsolete-not-provided eclipse-sdk-nls-fi
eclipse-nls-zh.noarch: W: no-documentation
eclipse-nls-zh.noarch: W: obsolete-not-provided eclipse-sdk-nls-zh
eclipse-nls-pl.noarch: W: no-documentation
eclipse-nls-pl.noarch: W: obsolete-not-provided eclipse-sdk-nls-pl
eclipse-nls-el.noarch: W: no-documentation
eclipse-nls-el.noarch: W: obsolete-not-provided eclipse-sdk-nls-el
eclipse-nls-de.noarch: W: no-documentation
eclipse-nls-de.noarch: W: obsolete-not-provided eclipse-sdk-nls-de
eclipse-nls-es.noarch: W: no-documentation
eclipse-nls-es.noarch: W: obsolete-not-provided eclipse-sdk-nls-es
eclipse-nls-tr.noarch: W: no-documentation
eclipse-nls-tr.noarch: W: obsolete-not-provided eclipse-sdk-nls-tr
eclipse-nls-zh_TW.noarch: W: no-documentation
eclipse-nls-zh_TW.noarch: W: obsolete-not-provided eclipse-sdk-nls-zh_TW
eclipse-nls-he.noarch: W: no-documentation
eclipse-nls-he.noarch: W: obsolete-not-provided eclipse-sdk-nls-he
eclipse-nls-da.noarch: W: no-documentation
eclipse-nls-da.noarch: W: obsolete-not-provided eclipse-sdk-nls-da
eclipse-nls-bg.noarch: W: no-documentation
eclipse-nls-bg.noarch: W: obsolete-not-provided eclipse-sdk-nls-bg
eclipse-nls-ru.noarch: W: no-documentation
eclipse-nls-ru.noarch: W: obsolete-not-provided eclipse-sdk-nls-ru
eclipse-nls-pt_BR.noarch: W: no-documentation
eclipse-nls-pt_BR.noarch: W: obsolete-not-provided eclipse-sdk-nls-pt_BR
eclipse-nls-cs.noarch: W: no-documentation
eclipse-nls-cs.noarch: W: obsolete-not-provided eclipse-sdk-nls-cs
26 packages and 0 specfiles checked; 0 errors, 52 warnings.

I think these can probably be waived.

Comment 10 Sean Flanigan 2008-09-09 03:51:44 UTC
Minor housekeeping in the new version here:
http://seanf.fedorapeople.org/eclipse-nls/0.2.0-0.3/eclipse-nls.spec

* Mon Sep 9 2008 Sean Flanigan <sflaniga@redhat.com> - 0.2.0-0.3.20080807snap
- Added eclipse_version macro
- Changed the Obsoletes version to be slightly higher than the last release of 
  eclipse-sdk-nls

I have built another SRPM, but I haven't uploaded it.  If anyone wants it, just let me know!

Comment 11 Jens Petersen 2008-09-10 01:56:37 UTC
Created attachment 316267 [details]
eclipse-nls.spec.patch-1

minor suggested cleanup

Comment 12 Tony Fu 2008-09-10 03:07:32 UTC
requested by Jens Petersen (#27995)

Comment 13 Sean Flanigan 2008-09-10 04:03:26 UTC
Applied patch from Jens, result: http://seanf.fedorapeople.org/eclipse-nls/0.2.0-0.4/eclipse-nls.spec

Comment 14 Jens Petersen 2008-09-10 07:48:22 UTC
Here is the review:

 +:ok, =:needs attention

MUST Items:
[+] MUST: rpmlint must be run on every package. The output should be posted in the review.
[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[NA] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task.

The tarball is created with from upstream updates site with a script included, since there are other 400 .jar fragments per language.

[+] MUST: The package must successfully compile and build into binary rpms on at least one supported architecture.
[+] MUST: All build dependencies must be listed in BuildRequires
[+] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros
[+] MUST: The package must contain code, or permissable content.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[=] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.

It would be nice to have a license file from upstream to include in the packages.

[+] SHOULD: The reviewer should test that the package builds in mock.
[=] SHOULD: The reviewer should test that the package functions as described.

There are some know issues with eclipse plugins that are stopping this plugin from working in common cases, but it would be good to have this package included so that they can be straightened out.  A bug should be opened to track that issue is there isn't one already.

Andrew, or any other Java packager: is it ok to not to jar_repack since this is noarch anyway?

Package is APPROVED as packaged.

Comment 15 Jens Petersen 2008-09-10 08:00:45 UTC
Created attachment 316289 [details]
eclipse-nls.spec.patch-2

some very minor comment suggestions

Comment 16 Andrew Overholt 2008-09-10 13:18:03 UTC
(In reply to comment #14)
> Andrew, or any other Java packager: is it ok to not to jar_repack since this is
> noarch anyway?

I think we should run this by fedora-devel-java-list to get opinions and if people are generally okay with it, we can add it to the guidelines.  Can you please send an email to the list to get comments?  Thanks.

Comment 17 Sean Flanigan 2008-09-11 02:11:36 UTC
Another version here:

http://seanf.fedorapeople.org/eclipse-nls/0.2.0-0.5/eclipse-nls.spec

* Thu Sep 11 2008 Sean Flanigan <sflaniga@redhat.com> - 0.2.0-0.5.20080807snap
- Applied another tidy-up patch from Jens Petersen and added a comment
 about the licence doc files

Comment 18 Sean Flanigan 2008-09-11 02:19:22 UTC
New Package CVS Request
=======================
Package Name: eclipse-nls
Short Description: Babel translations for Eclipse
Owners: seanf
Branches: 
InitialCC: petersen

Comment 19 Jens Petersen 2008-09-11 02:42:00 UTC
(In reply to comment #16)
> I think we should run this by fedora-devel-java-list to get opinions and if
> people are generally okay with it, we can add it to the guidelines.

Okay, let's turn on jar repacking for now in builds until that gets cleared up.

Comment 20 Jens Petersen 2008-09-11 03:42:21 UTC
cvs admin done

Comment 21 Sean Flanigan 2008-09-11 04:36:15 UTC
(In reply to comment #20)
> cvs admin done

Thanks Jens!  I have re-enabled jar repacking for now, and told Koji to get to work:

http://koji.fedoraproject.org/koji/buildinfo?buildID=62602

So eclipse-nls-* should show up in rawhide some time in the next 24 hours.

Comment 22 Jens Petersen 2008-09-11 06:48:51 UTC
thanks


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