Make it work both with Python2 and Python3 #488

mike-fabian · 2019-07-16T09:20:54Z

With this change (and some changes I did to the python code in https://github.com/googlei18n/noto-emoji.git I was able to build the "NotoColorEmoji.ttf" font using Python3.

googlebot · 2019-07-16T09:20:57Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-16T15:36:47Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

dougfelt · 2019-07-16T18:32:29Z

nototools/generate_sample_text_html.py

-    print >> f, '</table>'
-    print >> f, _HTML_FOOTER
+        f.write('<td>'.join(cols) + '\n')
+      f.write('<tr><td colspan=4>&nbsp;')


why no '\n' added in these lines?

meant to make that a review comment

Oh, you are right! adding these '\n' ...

dougfelt

Thanks a lot for this. Saw a few newlines I wasn't sure about.

dougfelt · 2019-07-16T18:35:48Z

nototools/noto_cmap_reqs.py

@@ -3285,10 +3285,10 @@ def _dump_primaries():
  for block in unicode_data.block_names():
    block_range = unicode_data.block_range(block)
    primary_script = _primary_script_for_block(block)
-    print '%13s %6s %s' % (
+    print('%13s %6s %s\n' % (


It seems the '\n' is not wanted here? Print already issues a newline, right?

You are right! Sorry!

dougfelt · 2019-07-16T18:37:35Z

nototools/sync_repos.py

@@ -84,17 +84,17 @@ def noto_checkout(
      failed_tags.append('%s: %s' % (r, t))

  if failed_tags:
-    print >> sys.stderr, 'failed to find:\n  %s' % '\n  '.join(failed_tags)
+    sys.stderr.write('failed to find:\n  %s' % '\n  \n'.join(failed_tags))


The added newline is in the wrong place.

dougfelt · 2019-07-16T18:39:38Z

nototools/unicode_data.py

@@ -784,7 +784,7 @@ def _read_emoji_data(lines):
      continue

    # discourage lots of redundant copies of seq_type


delete comment, no longer applies?

Yes, no longer applies when the intern() is gone!

dougfelt · 2019-07-16T18:42:17Z

nototools/generate_sample_text_html.py

-    print >> f, '</table>'
-    print >> f, _HTML_FOOTER
+        f.write('<td>'.join(cols) + '\n')
+      f.write('<tr><td colspan=4>&nbsp;')


meant to make that a review comment

googlebot · 2019-07-17T08:20:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

PureTryOut · 2019-07-17T15:05:40Z

So this will close #472 when merged then.

mike-fabian · 2019-07-18T07:29:45Z

Do I need to do something about the “Need a CLA for one or more commit authors”?

The commits have mfabian@redhat.com as the author e-mail, should I change that to my Google mail maiku.fabian@gmail.com because I used that to sign the CLA? I tried to add mfabian@redhat.com as an additional address to my google account, but that didn’t work because it is used in my work account and apparently one cannot add the same address to another google account.

dougfelt · 2019-07-18T22:56:10Z

Hmmm, I'm not sure about workarounds for this. If you can change the commit address that would likely work.

…

On Thu, Jul 18, 2019 at 12:29 AM Mike FABIAN ***@***.***> wrote: Do I need to do something about the “Need a CLA for one or more commit authors”? The commits have ***@***.*** as the author e-mail, should I change that to my Google mail ***@***.*** because I used that to sign the CLA? I tried to add ***@***.*** as an additional address to my google account, but that didn’t work because it is used in my work account and apparently one cannot add the same address to another google account. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#488>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACL2SDBALPYTJH6S2AQPRK3QAALWXANCNFSM4ID625HQ> .

…dded '\n' And remove a comment about intern() which no longer applies.

googlebot · 2019-07-19T08:35:18Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mike-fabian · 2019-07-19T08:41:23Z

Hmmm, I'm not sure about workarounds for this. If you can change the commit address that would likely work.

I have changed the commit address.

RoelN · 2019-09-05T08:31:11Z

I can't get pip3 install -r requirements.txt to install properly. I'm on macOS High Sierra. It errors out on:

Building wheels for collected packages: pyclipper
Building wheel for pyclipper (setup.py) ... error
[...]
  2 warnings and 1 error generated.
  error: command 'clang' failed with exit status 1
  ----------------------------------------
  ERROR: Failed building wheel for pyclipper
  Running setup.py clean for pyclipper
  Failed to build pyclipper
[...]

and then:

ERROR: ufo2ft 2.9.1 has requirement booleanOperations>=0.8.2, but you'll have booleanoperations 0.7.0 which is incompatible.
ERROR: ufo2ft 2.9.1 has requirement defcon>=0.6.0, but you'll have defcon 0.3.1 which is incompatible.
ERROR: mutatormath 2.1.2 has requirement defcon>=0.3.5, but you'll have defcon 0.3.1 which is incompatible.
ERROR: fontmake 1.10.1 has requirement booleanOperations>=0.8.2, but you'll have booleanoperations 0.7.0 which is incompatible.
ERROR: fontmake 1.10.1 has requirement defcon[lxml]>=0.6.0, but you'll have defcon 0.3.1 which is incompatible.

I eventually managed to get al requirements in order by manual installs through pip3 and brew. I was able to run:

sudo python3 setup.py develop

So while it's working now (a Noto Emoji build is happily purring in the background), I'm not sure if I'm doing something wrong, the requirements.txt need to be modified or something else is wrong. Any advice?

RoelN · 2019-09-16T08:34:21Z

If nobody else is running into problems, bite the bullet and go full python 3? @dougfelt

RoelN · 2019-10-08T08:13:17Z

googlefonts/noto-emoji#267 is waiting for this PR. Can we merge this?

anthrotype

Thanks a lot for this! LGTM

anthrotype · 2019-10-15T14:33:22Z

nototools/collect_cldr_punct.py

@@ -99,7 +99,7 @@ def _collect_script_to_punct(files):
      script = cldr_data.get_likely_script(filename)
      if script == 'Zzzz':
        if filename != 'root':
-          print >> sys.stderr, 'no script for %s' % filename
+          sys.stderr.write('no script for %s\n' % filename)


to use print with stderr you can do:

Suggested change

sys.stderr.write('no script for %s\n' % filename)

print('no script for %s' % filename, file=sys.stderr)

but sys.stderr.write works too

sys.stderr.write(foo) works in both python2 and python3, print(foo, file=sys.stderr) only works in python3.

You need to from __future__ import print_function.
If we haven't done that yet, we should make sure to add this as well as the other two canonical future imports (absolute_import, division) to all modules.

anthrotype · 2019-10-15T14:46:28Z

nototools/lint_config.py

@@ -468,7 +468,7 @@ def _process_data(data):
      arg_type = m.group(4)
      comment = m.group(5)

-      while line_indent <= indent[0]:
+      while len(line_indent) <= indent[0]:


good catch. Surprising how py2 would silently let you to compare a string with an int.. I wonder if this was working at all before

anthrotype · 2019-10-15T15:02:03Z

nototools/unicode_data.py

@@ -783,8 +783,7 @@ def _read_emoji_data(lines):
    if m.group(2):
      continue

-    # discourage lots of redundant copies of seq_type
-    seq_type = intern(m.group(3).strip().encode('ascii'))


FWIW the old intern builtin was moved to sys.intern: https://docs.python.org/3/library/sys.html?highlight=sys.intern#sys.intern

dougfelt reviewed Jul 16, 2019

View reviewed changes

dougfelt approved these changes Jul 16, 2019

View reviewed changes

mike-fabian mentioned this pull request Jul 17, 2019

Python3 port googlefonts/noto-emoji#267

Merged

dougfelt approved these changes Jul 17, 2019

View reviewed changes

mike-fabian added 3 commits July 19, 2019 10:34

Make it work both with Python2 and Python3

3aadc18

Replace two more xrange() with range()

45a061b

Add missing '\n', remove mistakenly added '\n', correct position of a…

39ff50d

…dded '\n' And remove a comment about intern() which no longer applies.

mike-fabian force-pushed the master branch from 11680cd to 39ff50d Compare July 19, 2019 08:35

anthrotype approved these changes Oct 15, 2019

View reviewed changes

anthrotype merged commit 1a6d1c4 into notofonts:master Oct 15, 2019

dasJ mentioned this pull request Nov 18, 2019

Add Python 3 support C1710/blobmoji#48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it work both with Python2 and Python3 #488

Make it work both with Python2 and Python3 #488

mike-fabian commented Jul 16, 2019

googlebot commented Jul 16, 2019

googlebot commented Jul 16, 2019

dougfelt Jul 16, 2019

dougfelt Jul 16, 2019

mike-fabian Jul 17, 2019

dougfelt left a comment

dougfelt Jul 16, 2019

mike-fabian Jul 17, 2019

dougfelt Jul 16, 2019

mike-fabian Jul 17, 2019

dougfelt Jul 16, 2019

mike-fabian Jul 17, 2019

dougfelt Jul 16, 2019

googlebot commented Jul 17, 2019

PureTryOut commented Jul 17, 2019

mike-fabian commented Jul 18, 2019

dougfelt commented Jul 18, 2019 via email

googlebot commented Jul 19, 2019

mike-fabian commented Jul 19, 2019

RoelN commented Sep 5, 2019

RoelN commented Sep 16, 2019

RoelN commented Oct 8, 2019

anthrotype left a comment

anthrotype Oct 15, 2019

plugwash Oct 16, 2019

anthrotype Oct 17, 2019

anthrotype Oct 15, 2019

anthrotype Oct 15, 2019

		@@ -784,7 +784,7 @@ def _read_emoji_data(lines):
		continue

		# discourage lots of redundant copies of seq_type

	sys.stderr.write('no script for %s\n' % filename)
	print('no script for %s' % filename, file=sys.stderr)

Make it work both with Python2 and Python3 #488

Make it work both with Python2 and Python3 #488

Conversation

mike-fabian commented Jul 16, 2019

googlebot commented Jul 16, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Jul 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougfelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jul 17, 2019

PureTryOut commented Jul 17, 2019

mike-fabian commented Jul 18, 2019

dougfelt commented Jul 18, 2019 via email

googlebot commented Jul 19, 2019

mike-fabian commented Jul 19, 2019

RoelN commented Sep 5, 2019

RoelN commented Sep 16, 2019

RoelN commented Oct 8, 2019

anthrotype left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment