Python3 support - Githubissues

gyst commented 6 years ago

I'm porting plone.recipe.codeanalysis to python3. This is a dependency that needed porting also.

mauritsvanrees commented 6 years ago

O, when I ask the msgfmt utility to check the results, it fails too:

$ msgfmt -c locales/zh/LC_MESSAGES/Poi.po
locales/zh/LC_MESSAGES/Poi.po:15:18: invalid control sequence
locales/zh/LC_MESSAGES/Poi.po:15:24: invalid control sequence
...
locales/zh/LC_MESSAGES/Poi.po:125:10: invalid control sequence
msgfmt: too many errors, aborting

Screen shot of Plone UI in Chinese with the Poi locales done using your branch:

gyst commented 6 years ago

I actually started with the various print statements, then later squashed my work to make review easier. What you see now is the first variant that passes tests on both python3.6 and python2.7. I'm personally actually less interested in python2.7 than I am in python3.6, so my sequence is to get it to work in python3.6 and then get it to work again in python2.7...

I'll see if I can reproduce the non-ascii message strings in a test. I've caught various bugs around those already. I'll also address your other comments.

gforcada commented 6 years ago

@gyst I just enabled coveralls for i18ndude, would you mind updating travis configuration to also run coveralls?

Question: should we merge tox.ini and .travis.yml ? I don't see one using the other, is already complex enough to have to maintain a system, let alone two...

gyst commented 6 years ago

@gforcada we should definitely not merge tox.ini and travis.cfg, they're different configs for different toolchains even if they serve the same purpose. It's actually quite easy to keep them in sync manually, I've done that for many grokcore.* packages now it's not a big deal.

I'll try to see if your coveralls fix can be integrated into tox and travis.

gyst commented 6 years ago

I cannot reproduce your bug report @mauritsvanrees. I've checked out the 2.x branch of Products.Poi but you're referencing a rebuild_i18n.sh script that is not part of the repo. I've cobbled one together from another repo but that breaks on HTML parse errors in the Poi code. See the branch I pushed on Products.Poi https://github.com/collective/Products.Poi/tree/i18ndude_reproduce.

I need i18ndude to have a working find-untranslated that works in python3 so I can lint my code. That it does. Everything beyond is lower priority for me.

I'm happy to fix any bugs in my work if I get a reproduction, ideally in the form of a test but a fully scripted build with a set of actions is OK too. But I'm not prepared to dive into a stack I don't really know, while having to guess what kind of toolchain and version (rebuild_i18n.sh, msgfmt, show me the buildout parts please) the bugs are reported from.

gyst commented 6 years ago

@gforcada I don't use coveralls myself and don't know how it's used. If you want to add it to the .travis.yml on the python3 branch that's fine with me. The tox configuration already runs with coverage reporting.

mauritsvanrees commented 6 years ago

@gyst The rebuild_i18n.sh script is in the Products/Poi directory. See https://github.com/collective/Products.Poi/blob/2.x/Products/Poi/rebuild_i18n.sh It is basically the same on the 2.x and the master branch. I fixed a few i18n issues on master today.

The errors that you saw, are because the i18ndude html parser does not do well on templates that are meant for RSS. So the rebuild_i18n.sh script excludes a few templates. On 2.x:

i18ndude rebuild-pot --pot locales/poi.pot --create Poi --exclude="poi-my-issues-rss.xml.pt poi-orphaned-issues-rss.xml.pt poi-issue-search-rss.xml.pt" .

and this on master:

i18ndude rebuild-pot --pot locales/poi.pot --create Poi --exclude="poi-my-issues-rss.xml.pt poi-orphaned-issues-rss.xml.pt poi-issue-search-rss.xml.pt poi-my-submitted-issues-rss.xml.pt" .

The script expects the i18ndude script to be on the path, which is at least true for my computer.

mauritsvanrees commented 6 years ago

Ah, actually, the rebuild-pot part seems fine. It is the sync part that gives wrong results for non-ascii. So within the Products/Poi directory try this:

i18ndude sync --pot locales/Poi.pot locales/cs/LC_MESSAGES/Poi.po

Same is true for plone.app.locales. Go to the plone/app/locales/locales directory and try this command:

i18ndude sync --pot plone.pot cs/LC_MESSAGES/plone.po

With the i18ndude master branch, this gives no change, with your branch it writes back the msgstrs wrongly:

 #: plone.app.dexterity/plone/app/dexterity/interfaces.py:97
 msgid "# of items"
-msgstr "počet položek"
+msgstr "po\u010det polo\u017eek"

gyst commented 6 years ago

Sorry, I can't reproduce this. It would be helpful to know what kind of environment/buildout you're using. I'm using Products.Poi branch i18ndude_reproduce but maybe you're using Plone coredev? Or what? With which source checkouts? On which branch? With which modifications?

If I run your commands I get:

i18ndude_reproduce  ~/Products.Poi/Products/Poi
app@thor(minddistrict):Poi$ ../../bin/i18ndude sync --pot locales/Poi.pot locales/cs/LC_MESSAGES/Poi.po
locales/cs/LC_MESSAGES/Poi.po: 0 added, 0 removed

~/Products.Poi/parts/omelette/plone/app/locales/locales
app@thor(minddistrict):locales$ ~/Products.Poi/bin/i18ndude sync --pot plone.pot cs/LC_MESSAGES/plone.po
cs/LC_MESSAGES/plone.po: 0 added, 0 removed

Sorry to be obnoxious. All I'm asking for is a reproducible set of steps that gives me a reproduction of the problem you're seeing, which I'm sure is real. Though, did you try and test whether the changes I pushed yesterday maybe fix them already? https://github.com/collective/i18ndude/pull/48/commits/593da2d22cfcf49d3e12ad62729a3705d2afabf6

mauritsvanrees commented 6 years ago

You are not obnoxious. I am glad you are working on this! 👍

I did already try with your changes from yesterday. Let me try again. I am doing it like this:

Go to a git clone of i18ndude.
Fetch your remote and do git checkout gyst/master.
For good measure get a fresh buildout, with the commands that .travis.yml uses. In this case I use Python 2.7: virtualenv-2.7 . && bin/pip install -r requirements.txt && bin/buildout. (I tried with Python 3.6.4 too. Oh, that gives a different error, let me report that further on in this comment.)

Resulting script:

$ cat bin/i18ndude 
#!/Users/maurits/tools/src/i18ndude/bin/python2.7

import sys
sys.path[0:0] = [
  '/Users/maurits/tools/src/i18ndude/src',
  '/Users/maurits/shared-eggs/cp27m/ordereddict-1.1-py2.7.egg',
  '/Users/maurits/shared-eggs/cp27m/zope.tal-4.3.0-py2.7.egg',
  '/Users/maurits/shared-eggs/cp27m/zope.interface-4.4.3-py2.7-macosx-10.12-x86_64.egg',
  '/Users/maurits/shared-eggs/cp27m/zope.i18nmessageid-4.1.0-py2.7-macosx-10.12-x86_64.egg',
  '/Users/maurits/shared-eggs/cp27m/lxml-4.1.1-py2.7-macosx-10.12-x86_64.egg',
  '/Users/maurits/tools/src/i18ndude/lib/python2.7/site-packages',
  ]

import i18ndude.script

if __name__ == '__main__':
    sys.exit(i18ndude.script.main())

Go to a checkout of let's say plone.app.locales master, and go to the plone/app/locales/locales directory.
<path to the above i18ndude script> sync --pot plone.pot cs/LC_MESSAGES/plone.po.
This gives as output cs/LC_MESSAGES/plone.po: 0 added, 0 removed, just like you have.
Problem: several msgstr lines are wrong:

$ git diff
diff --git a/plone/app/locales/locales/cs/LC_MESSAGES/plone.po b/plone/app/locales/locales/cs/LC_MESSAGES/plone.po
index 9024f42b1..b2a4fbf69 100644
--- a/plone/app/locales/locales/cs/LC_MESSAGES/plone.po
+++ b/plone/app/locales/locales/cs/LC_MESSAGES/plone.po
@@ -43,7 +43,7 @@ msgstr ""

 #: plone.app.dexterity/plone/app/dexterity/interfaces.py:97
 msgid "# of items"
-msgstr "počet položek"
+msgstr "po\u010det polo\u017eek"

When you create a Plone Site with this language, you will see those ugly messages literally in the browser, so it really is a problem.

When I use a Python 3.6.4 virtualenv, and try the same sync command, I get this error:

$ ~/tools/src/i18ndude/bin/i18ndude sync --pot plone.pot cs/LC_MESSAGES/plone.po
Traceback (most recent call last):
  File "/Users/maurits/tools/src/i18ndude/bin/i18ndude", line 16, in <module>
    sys.exit(i18ndude.script.main())
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/script.py", line 715, in main
    errors = arguments.func(arguments)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/script.py", line 456, in sync
    writer.write(msgstrToComment=False, sync=True)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 519, in write
    self._write_header()
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 530, in _write_header
    self._printToFile(f, '# %s' % line)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 514, in _printToFile
    file.write(self._encode(string))
TypeError: write() argument must be str, not bytes

The rebuild-pot command also gives a traceback on Python 3.6.4, which could be a problem in zope.tal or maybe in the way that i18ndude calls it. zope.tal 4.3.0 claims to be Python 3.6 compatible, but given the traceback I doubt it. To reproduce, go to a master checkout of Products.CMFPlone and go to the Products.CMFPlone/Products/CMFPlone/browser directory. First call i18ndude on one template:

$ ~/tools/src/i18ndude/bin/i18ndude rebuild-pot --pot plone.pot --create plone templates/search.pt 
There was an error processing templates/search.pt
Traceback (most recent call last):
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/extract.py", line 534, in tal_strings
    metal=False)()
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 272, in __call__
    self.interpret(self.program)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 344, in interpret
    handlers[opcode](self, args)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 879, in do_useMacro
    self.interpret(block)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 344, in interpret
    handlers[opcode](self, args)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 927, in do_fillSlot
    self.interpret(block)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 344, in interpret
    handlers[opcode](self, args)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 830, in do_loop_tal
    self.interpret(block)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 344, in interpret
    handlers[opcode](self, args)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 644, in do_insertI18nText_tal
    text = self.translate(text)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talgettext.py", line 78, in translate
    default = getattr(msgid, 'default', unicode(msgid))
NameError: name 'unicode' is not defined

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/extract.py", line 546, in tal_strings
    metal=False)()
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 272, in __call__
    self.interpret(self.program)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 344, in interpret
    handlers[opcode](self, args)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 879, in do_useMacro
    self.interpret(block)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 344, in interpret
    handlers[opcode](self, args)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 927, in do_fillSlot
    self.interpret(block)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 344, in interpret
    handlers[opcode](self, args)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 830, in do_loop_tal
    self.interpret(block)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 344, in interpret
    handlers[opcode](self, args)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talinterpreter.py", line 644, in do_insertI18nText_tal
    text = self.translate(text)
  File "/Users/maurits/shared-eggs/cp36m/zope.tal-4.3.0-py3.6.egg/zope/tal/talgettext.py", line 78, in translate
    default = getattr(msgid, 'default', unicode(msgid))
NameError: name 'unicode' is not defined
Traceback (most recent call last):
  File "/Users/maurits/tools/src/i18ndude/bin/i18ndude", line 16, in <module>
    sys.exit(i18ndude.script.main())
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/script.py", line 715, in main
    errors = arguments.func(arguments)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/script.py", line 349, in rebuild_pot
    writer.write(msgstrToComment=True)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 519, in write
    self._write_header()
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 530, in _write_header
    self._printToFile(f, '# %s' % line)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 514, in _printToFile
    file.write(self._encode(string))
TypeError: write() argument must be str, not bytes

And for a different error, call it on some Python files in the same directory:

$ ~/tools/src/i18ndude/bin/i18ndude rebuild-pot --pot plone.pot --create plone *py
Traceback (most recent call last):
  File "/Users/maurits/tools/src/i18ndude/bin/i18ndude", line 16, in <module>
    sys.exit(i18ndude.script.main())
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/script.py", line 715, in main
    errors = arguments.func(arguments)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/script.py", line 290, in rebuild_pot
    pyreader.read()
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 749, in read
    exclude=self.exclude + ('tests', ))
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/extract.py", line 475, in py_strings
    eater(ttype, tstring, stup, etup, line)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/extract.py", line 289, in __call__
    self.__state(ttype, tstring, stup[0])
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/extract.py", line 337, in __openseen
    self.__addentry(msgid, default)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/extract.py", line 387, in __addentry
    idx = messages.index(msg)
AttributeError: 'dict_keys' object has no attribute 'index'

With Python 2.7 both rebuild-pot commands run fine.

gyst commented 6 years ago

OK that looks on first sight like something I will be able to reproduce, and the zope.tal traceback indeed looks like a broken python3 code path. I'll investigate further, thanks.

gyst commented 6 years ago

Hi @mauritsvanrees I managed to fix all the bugs you reported. If you read my inline comments you'll notice I have by now depleted my motivation to invest time in this package... I hope this is now mergeable and releasable.

mauritsvanrees commented 6 years ago

I can indeed imagine that you have had it by now... Thanks for sticking with it so far.

Let me try. On plone.app.locales when I sync the plone.pot file to plone.po in all languages, I see a change in Afrikaans, which looks like an improvement, so that is good:

-msgstr "Verwyder gekose lÃªer"
+msgstr "Verwyder gekose lêer"
...
-msgstr "Behou huidige lÃªer"
+msgstr "Behou huidige lêer"

Okay, when I do the same for the widgets, I get an error:

$ ~/tools/src/i18ndude/bin/i18ndude sync --pot widgets.pot */*/widgets.po .
Traceback (most recent call last):
  File "/Users/maurits/tools/src/i18ndude/bin/i18ndude", line 16, in <module>
    sys.exit(i18ndude.script.main())
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/script.py", line 715, in main
    errors = arguments.func(arguments)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/script.py", line 452, in sync
    added_msgids, removed_msgids = po.sync(pot_ctl)
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 316, in sync
    for msgid in self.accept_ids(msgctl.keys())]
  File "/Users/maurits/tools/src/i18ndude/src/i18ndude/catalog.py", line 343, in accept_ids
    for key in self.keys():
RuntimeError: OrderedDict mutated during iteration

Okay, I had a look at what current master did here. Master had no problems. But I did see that a manual search-and-replace was called for in these po files, which I did (https://github.com/collective/plone.app.locales/commit/ce55662d8abb27c1fd844cc63d4d87be796fe7ad), and that luckily fixed the sync error with your branch too. Should be fixed before release, but I am fine with doing that in a separate issue. (Whether you or I or someone else picks that up, is another question.)

The NameError: name 'unicode' is not defined is still there on Python 3, but that is pending your PR in zope.tal https://github.com/zopefoundation/zope.tal/pull/11. If I quickly fix it locally, the problem goes a way.

Overall: great job, thank you! I will merge.

mauritsvanrees commented 6 years ago

Note: I have created branch 4.x before merging this.

mauritsvanrees commented 6 years ago

Okay, I have fixed the remaining problem and have released 5.0.0. Aaaand... 5.0.1 which is released as universal wheel.

collective / i18ndude

Python3 support #48