Bug 1527289 - Review Request: nototools - Noto fonts support tools and scripts plus web site generation
Summary: Review Request: nototools - Noto fonts support tools and scripts plus web sit...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1514274
TreeView+ depends on / blocked
 
Reported: 2017-12-19 06:28 UTC by Peng Wu
Modified: 2018-02-06 15:35 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-02-06 15:35:08 UTC
panemade: fedora-review+


Attachments (Terms of Use)

Description Peng Wu 2017-12-19 06:28:36 UTC
Spec URL: https://pwu.fedorapeople.org/fonts/nototools/nototools.spec
SRPM URL: https://pwu.fedorapeople.org/fonts/nototools/nototools-20170926-1.fc27.src.rpm
Description: Noto fonts support tools and scripts plus web site generation
Fedora Account System Username:pwu

Comment 1 Peter Oliver 2017-12-19 11:50:00 UTC
Thanks for working on this.  I noticed the following:

> Version:	20170926

https://fedoraproject.org/wiki/Packaging:Versioning#Upstream_has_never_chosen_a_version says:

“When upstream has never chosen a version, you MUST use `Version: 0`”

> License:	ASL 2.0

The files packaged into `/usr/lib/python2.7/site-packages/third_party/ucd` are under the Unicode licence.

> Source0:	nototools-0c99dff.tar.gz

Shouldn’t this be `https://github.com/googlei18n/%{name}/archive/%{commit0}.tar.gz#/%{name}-%{commit0}.tar.gz`, or similar?

Description is empty for python2-nototools.

There’s missing Requires and BuildRequires of `python2dist(fonttools)`.

Are we sure it’s okay to put files in /usr/lib/python2.7/site-packages/third_party?

File third_party/ucd/README.third_party describes where this content comes from.  I think we should probably add these as separate sources and pull them down ourselves.

Some scripts get installed into both /usr/bin and /usr/lib/python2.7/site-packages/nototools, but the copies in /usr/lib/python2.7/site-packages/nototools are not executable and have an incorrect #! line.

Comment 2 Neal Gompa 2017-12-19 13:54:11 UTC
Two things:

1. site-packages/third_party should not be done in any circumstance. That's just Google's vendoring thing going on, and you should replace that with proper modules.

2. Please default to Python 3 for binaries.

Comment 3 Neal Gompa 2017-12-19 14:02:03 UTC
(In reply to Neal Gompa from comment #2)
> 
> 1. site-packages/third_party should not be done in any circumstance. That's
> just Google's vendoring thing going on, and you should replace that with
> proper modules.
> 

So apparently all this stuff in "third_party" aside from spiro and fontcrunch are data files. The data files should be put in /usr/share/nototools/third_party and the scripts referencing them should be adjusted.

I'm not sure what you should do with spiro and fontcrunch.

Comment 4 Peter Oliver 2017-12-19 14:39:35 UTC
(In reply to Neal Gompa from comment #3)

> Please default to Python 3 for binaries.

These scripts don't (yet) work with Python 3.

> I'm not sure what you should do with spiro and fontcrunch.

If it turned out that we needed them, we could try to package their upstream versions directly.  However, I don't think we do need them.  They're not referenced elsewhere in nototools' source.

Comment 5 Jeremy Bicha 2017-12-20 00:59:54 UTC
Hi, I helped package nototools for Debian. I thought I'd leave some comments, although some of them are just confirming what you already figured out.

nototools explicitly does not support Python 3 yet.

In Debian I excluded all of third_party/ except for ucd/ (I see that's the same as is being done in the current .spec). The problem is that Google has made it clear that it will patch the Unicode data and so it's not really the same as the generic Unicode data that may already be in the distro.

The only real build dependency besides python I added was fonttools which was only used for build tests.

nototools wants to install several scripts to /usr/bin/ . So far, the only one I bother installing is add_vs_cmap (renamed from add_vs_cmap.py to comply with Debian recommendations. Note that renaming means you will need to patch the Noto Emoji build system and any other fonts that copy the build system for there.)

If you do install the other scripts, be aware that some of them have dependencies on other font related Python packages which might not be packaged in your distro yet.

Comment 6 Peng Wu 2017-12-20 08:36:47 UTC
Okay, I updated the spec file a bit.

Please help on how to improve the rest part of the spec file, thanks!

Spec URL: https://pwu.fedorapeople.org/fonts/nototools/nototools.spec
SRPM URL: https://pwu.fedorapeople.org/fonts/nototools/nototools-0-20170926.0c99dff.fc27.src.rpm

Comment 7 Peng Wu 2017-12-20 08:39:25 UTC
(In reply to Peter Oliver from comment #1)
> Some scripts get installed into both /usr/bin and
> /usr/lib/python2.7/site-packages/nototools, but the copies in
> /usr/lib/python2.7/site-packages/nototools are not executable and have an
> incorrect #! line.

I think the python scripts in /usr/lib/python2.7/site-packages/nototools with
"#!/usr/bin/env python" is okay.

Comment 8 Parag AN(पराग) 2017-12-26 11:30:00 UTC
sorry I am getting late here. I will review this by Friday.

Comment 9 Parag AN(पराग) 2018-01-01 10:27:55 UTC
If you follow the current guidelines of Python packaging then you can use the py2_* and py3_* macros.

Use snapshot release guideline for release tag which should be
Release:        0.20170926.git%{shortcommit0}%{?dist}

You may want to use this modified spec file here -> https://paste.fedoraproject.org/paste/dtiP95sW0puc46~R4z8e9w

Comment 10 Peng Wu 2018-01-02 06:40:40 UTC
Thanks for the help on the spec file!
Please review the package again!

Spec URL: https://pwu.fedorapeople.org/fonts/nototools/nototools.spec
SRPM URL: https://pwu.fedorapeople.org/fonts/nototools/nototools-0-0.20170926.git0c99dff.fc27.src.rpm

Comment 11 Parag AN(पराग) 2018-01-02 07:24:36 UTC
Thanks but one issue still remained. We want this package to support python2 as well as python3?

I will say good to provide only python3 package with same name binaries instead to go for renaming binaries for py2 as well as py3 variants.

If package is not supposed to be used on RHEL7/EPEL7 or its previous releases then just target python3 subpackage.

Comment 12 Peter Oliver 2018-01-02 14:45:41 UTC
(In reply to Parag AN(पराग) from comment #11)
> Thanks but one issue still remained. We want this package to support python2
> as well as python3?

These scripts don't (yet) work with Python 3.  Hence, only Python 2 packages will be built.

Comment 13 Parag AN(पराग) 2018-01-02 16:37:17 UTC
I really don't see any use of packaging only for python2 version whereas we are moving to python3 soon totally.

Let me see if this can be made compatible with python3 version.

Comment 14 Peter Oliver 2018-02-01 11:01:14 UTC
(In reply to Parag AN(पराग) from comment #13)
> I really don't see any use of packaging only for python2 version whereas we
> are moving to python3 soon totally.
> 
> Let me see if this can be made compatible with python3 version.

Well, the use would be that we could get this into Fedora sooner.  If you can port it to Python 3 that's great, but if you don't have time to work on it now, that's okay, Python 2 is going to be around for a couple more years yet.

Comment 15 Parag AN(पराग) 2018-02-01 14:14:44 UTC
I had already given my attempt to convert it to python3 compatible code but I could not make "third_party/spiro/*" code compatible with python3. Its very old and mathematical code. Also once this conversion completes we need to make sure code runs in python3 environment without error.

I am sorry I took some time here to work on this but could not actually make some progress.

Another thing, If I am not wrong then Fedora is moving to use python3 packages and soon in next few releases it will become default. I think currently F28 Live installation also pulling python3 modules.

Comment 16 Parag AN(पराग) 2018-02-01 14:29:21 UTC
I am approving the SRPM submitted in comment#10

Let's first have this (python2) package in Fedora.

APPROVED.

Comment 17 Gwyn Ciesla 2018-02-02 13:15:34 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/nototools. You may commit to the branch "f27" in about 10 minutes.

Comment 18 Fedora Update System 2018-02-05 09:38:42 UTC
google-noto-emoji-fonts-20170928-3.fc27 nototools-0-0.20170926.git0c99dff.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-4791ed67e5

Comment 19 Fedora Update System 2018-02-05 17:08:24 UTC
google-noto-emoji-fonts-20170928-3.fc27, nototools-0-0.20170926.git0c99dff.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-4791ed67e5

Comment 20 Fedora Update System 2018-02-06 15:35:08 UTC
google-noto-emoji-fonts-20170928-3.fc27, nototools-0-0.20170926.git0c99dff.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.


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