Bug 634515 - Review Request: python-zope-i18n - Zope Internationalization Support
Review Request: python-zope-i18n - Zope Internationalization Support
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Toshio Ernie Kuratomi
Fedora Extras Quality Assurance
:
Depends On: 632554 632808 634036
Blocks: 633138
  Show dependency treegraph
 
Reported: 2010-09-16 04:09 EDT by Robin Lee
Modified: 2010-09-20 05:23 EDT (History)
3 users (show)

See Also:
Fixed In Version: python-zope-i18n-3.7.4-2.fc15
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-09-20 05:23:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
a.badger: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
patch to zope.i18n to look for locale data files in FHS compliant location (895 bytes, patch)
2010-09-16 20:02 EDT, Toshio Ernie Kuratomi
no flags Details | Diff
Changes to spec file (1.68 KB, patch)
2010-09-16 20:03 EDT, Toshio Ernie Kuratomi
no flags Details | Diff
patch to zope.i18n to look for locale data files in FHS compliant location (782 bytes, patch)
2010-09-17 22:16 EDT, Robin Lee
no flags Details | Diff

  None (edit)
Description Robin Lee 2010-09-16 04:09:33 EDT
Spec URL: http://cheeselee.fedorapeople.org/python-zope-i18n.spec
SRPM URL: http://cheeselee.fedorapeople.org/python-zope-i18n-3.7.4-1.fc14.src.rpm
Description:
This package implements several APIs related to internationalization and
localization and also provides:

* Locale objects for all locales maintained by the ICU project.

* Gettext-based message catalogs for message strings.

* Locale discovery for Web-based requests.


rumlint results:
$ rpmlint ./python-zope-i18n.spec /home/cheese/rpmbuild/SRPMS/python-zope-i18n-3.7.4-1.fc14.src.rpm /home/cheese/rpmbuild/RPMS/noarch/python-zope-i18n-3.7.4-1.fc14.noarch.rpm 
./python-zope-i18n.spec: W: no-cleaning-of-buildroot %install
./python-zope-i18n.spec: W: no-cleaning-of-buildroot %clean
./python-zope-i18n.spec: W: no-buildroot-tag
./python-zope-i18n.spec: W: no-%clean-section
python-zope-i18n.src: W: no-cleaning-of-buildroot %install
python-zope-i18n.src: W: no-cleaning-of-buildroot %clean
python-zope-i18n.src: W: no-buildroot-tag
python-zope-i18n.src: W: no-%clean-section
2 packages and 1 specfiles checked; 0 errors, 8 warnings.


I prefer not to include the extra requirements.


The tests are not run because zope.testrunner is not yet in Fedora.
testrunner used to be provided by the package zope.testing(<3.10.0). But it has been split out since then.

All the tests run successfully in a buildout environment in f14:

[cheeselee@chade zope.i18n-3.7.4]$ bin/test-all
Running zope.testrunner.layer.UnitTests tests:
  Set up zope.testrunner.layer.UnitTests in 0.000 seconds.
  Ran 466 tests with 0 failures and 0 errors in 14.917 seconds.
Tearing down left over layers:
  Tear down zope.testrunner.layer.UnitTests in 0.000 seconds.
Comment 1 Toshio Ernie Kuratomi 2010-09-16 14:04:57 EDT
Things start to get messy here: The locale files: /usr/lib/python2.6/site-packages/zope/i18n/locales/data/*.xml (possibly other files in there) could arguably belong underneath %{_datadir}.... Will look at closer along with the full review.
Comment 2 Toshio Ernie Kuratomi 2010-09-16 19:56:06 EDT
Good:
* Package follows naming guidelines
* Spec's name matches package
* Spec is readable
* Source matches upstream
* Not a shared library
* No bundled libraries
* Not relocatable
* Includes all directories that it creates.
* Permissions set properly
* Consistent use of macros
* Code not content
* Not a GUI application
* Does not own files or directories from another package
* All filenames UTF8
* Builds in koji

Needswork:
* License should be ZPLv2.1 and UCD because of the Unicode license on part of the data
* UCD license text is in src/zope/i18n/locales/data/license.html which should be pulled into %doc
* Locales: language files aren't being marked as belonging to a specific language
* Locales: under /usr/lib rather than %{_datadir} -- FHS wants architecture independent data file in %{_datadir}

I'll attach some sample patches for these issues.

Cosmetic:

Notes:
* I've had to read up quickly on what configure.zcml files are.  If I read it
  correctly, they're configuration of underlying pieces of framework by the
  current code.  The files are not meant for system admins/end-users to edit to
  configure anything.  Is that correct?

* rpmlint:
python-zope-i18n.src: W: no-cleaning-of-buildroot %install
python-zope-i18n.src: W: no-cleaning-of-buildroot %clean
python-zope-i18n.src: W: no-buildroot-tag
python-zope-i18n.src: W: no-%clean-section

These are fine unless building on EPEL-5 or F12.
Comment 3 Toshio Ernie Kuratomi 2010-09-16 20:02:34 EDT
Created attachment 447865 [details]
patch to zope.i18n to look for locale data files in FHS compliant location

This simple patch is likely upstreamable  (I've had good luck with similar patches to other upstream projects).  The way it works is to first check for the directory that is standard in upstream.  In this case, %{python_sitelib}/zope/i18n/locales/data.  If that exists and is readable, then it's used.  If it's not, then we set the directory to search in for the locale files to %{_datadir}/zope/i18n/locales/data (You can change this if you think another location under %{_datadir} is better).

As developers, in eggs, and virtualenvs, the directory exists in the module and therefore, that is used just like now.  As system packagers, we move the directory into %{_datadir} so the system package uses that instead.
Comment 4 Toshio Ernie Kuratomi 2010-09-16 20:03:58 EDT
Created attachment 447866 [details]
Changes to spec file

This is a patch to the spec file.  It marks the locale files with the languages that they belong to, moves them to the %{_datadir}, and fixes the license.
Comment 5 Robin Lee 2010-09-16 22:35:50 EDT
(In reply to comment #2)
> Needswork:
> * License should be ZPLv2.1 and UCD because of the Unicode license on part of
> the data
Will fix.

> * UCD license text is in src/zope/i18n/locales/data/license.html which should
> be pulled into %doc
Will fix.

> * Locales: language files aren't being marked as belonging to a specific
> language
Those files provides locale instances rather than translations. From the point of view of function, they parallel the files /usr/share/i18n/locales/*, which are provided by glibc-common. Do we use the %lang directive only if the file is in a natural language?

> * Locales: under /usr/lib rather than %{_datadir} -- FHS wants architecture
> independent data file in %{_datadir}
Will fix, but the directory where to place those files should be considered carefully, or I will consult upstream.

> Notes:
> * I've had to read up quickly on what configure.zcml files are.  If I read it
>   correctly, they're configuration of underlying pieces of framework by the
>   current code.  The files are not meant for system admins/end-users to edit to
>   configure anything.  Is that correct?
Yes. Those zcml files are for registration of components. They just tell other Zope packages what capabilities this package provides and are not intended for users to edit.
Comment 6 Toshio Ernie Kuratomi 2010-09-16 23:42:25 EDT
(In reply to comment #5)
> (In reply to comment #2)
> > * Locales: language files aren't being marked as belonging to a specific
> > language
> Those files provides locale instances rather than translations. From the point
> of view of function, they parallel the files /usr/share/i18n/locales/*, which
> are provided by glibc-common. Do we use the %lang directive only if the file is
> in a natural language?
> 
That's a good question.  I thought we marked that type of file but checking glibc-common, those files are not marked.  I'll discuss with spot whether that's a bug in glibc-common or if we don't want to mark this type of file.

Note: Command to check what's marked on any given package:

  rpm -q --qf '[%{filelangs} %{filenames}\n]' glibc-common

> > * Locales: under /usr/lib rather than %{_datadir} -- FHS wants architecture
> > independent data file in %{_datadir}
> Will fix, but the directory where to place those files should be considered
> carefully, or I will consult upstream.

Sounds good.  I don't know the best directory, just that they go under %{_datadir} somewhere.
Comment 7 Robin Lee 2010-09-16 23:55:35 EDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #2)
> That's a good question.  I thought we marked that type of file but checking
> glibc-common, those files are not marked.  I'll discuss with spot whether
> that's a bug in glibc-common or if we don't want to mark this type of file.
The files /usr/share/i18n/locales/* are all in ASCII or American English. So I think they should not be marked.

And the main pratical usage of %lang marks should be that users can filter out translations or documents in languages which they don't know to make disk usage lower. But I think the locale instances/objects should not be filtered out in this way.
Comment 8 Toshio Ernie Kuratomi 2010-09-17 00:41:54 EDT
I agree that the reason to mark things %lang is so that the user who doesn't need support for certain languages can filter optional files from their install. The question is if this should only be marking translations or if we should also be marking language support files which are optional.  I know when I was making embedded systems that I had to find things similar to the glib-common /usr/share/i18n/locales directory and delete extraneous files from it manually.  I do not know if that's something that we want to support or not though.  I've emailed spot and we'll see if he thinks one thing or the other and if not, I'll draft up a proposal for the FPC.
Comment 9 Toshio Ernie Kuratomi 2010-09-17 10:49:34 EDT
Spot says:
"""
I think we would want to mark any files which are:

Optional to functionality (e.g. app still works if these files aren't
present) and language specfic.
"""

SO these files would fall under that defintion.
Comment 10 Robin Lee 2010-09-17 13:34:59 EDT
(In reply to comment #9)
> SO these files would fall under that defintion.
OK.




As my experience with Zope packages, upstream never separates data files from source tree. But most of those packages contains no data files. This should be the one which contains the greatest amount of data.

And if we change the source code but leave setup.py unchanged, upstream may not accept our patch. Though our patch is small, it will impress the packaging convention in the Zope world. So at this time I may prefer to just place the data to our conventional place %{_datadir}%{name}. Do you agree?
Comment 11 Toshio Ernie Kuratomi 2010-09-17 16:36:06 EDT
That placement is fine with me.
Comment 12 Robin Lee 2010-09-17 22:16:08 EDT
Created attachment 448156 [details]
patch to zope.i18n to look for locale data files in FHS compliant location

Spec URL: http://cheeselee.fedorapeople.org/python-zope-i18n.spec
SRPM URL:
http://cheeselee.fedorapeople.org/python-zope-i18n-3.7.4-2.fc14.src.rpm

Changes:
- Moved the data files to %%{_datadir}/%%{name}/ and applied a small
  complementary patch with help from Toshio Kuratomi <a.badger@gmail.com>
- Marked the data files with %%lang directives properly
- License revised to 'ZPL2.1 and UCD'

patch to zope.i18n to look for locale data files in FHS compliant location
Comment 13 Robin Lee 2010-09-17 22:17:02 EDT
3.7.4-2

Spec URL: http://cheeselee.fedorapeople.org/python-zope-i18n.spec
SRPM URL:
http://cheeselee.fedorapeople.org/python-zope-i18n-3.7.4-2.fc14.src.rpm

Changes:
- Moved the data files to %%{_datadir}/%%{name}/ and applied a small
  complementary patch with help from Toshio Kuratomi <a.badger@gmail.com>
- Marked the data files with %%lang directives properly
- License revised to 'ZPL2.1 and UCD'
Comment 14 Toshio Ernie Kuratomi 2010-09-18 01:28:10 EDT
All issues taken care of.  Current package builds in koji.

APPROVED.
Comment 15 Robin Lee 2010-09-18 04:23:44 EDT
Kanshashimasu

New Package SCM Request
=======================
Package Name: python-zope-i18n
Short Description: Zope Internationalization Support
Owners: cheeselee
Branches: el5 el6 f13 f14
InitialCC:
Comment 16 Kevin Fenzi 2010-09-19 15:27:10 EDT
Git done (by process-git-requests).

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