Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add python 3 support #3

Merged
merged 2 commits into from Aug 15, 2017
Merged

Add python 3 support #3

merged 2 commits into from Aug 15, 2017

Conversation

ghost
Copy link

@ghost ghost commented Mar 16, 2015

One day, like that, I decide I'll port one of the tools on my system to Python 3, so I did.

The result is at here. I would have pushed it to gitorious, but the website has just been bought by gitlab and it seemed like a bad place to push.

All tests except test_IT_keep_entities_1 pass on both python 2.7 and python 3.3. The test_IT_keep_entities_1 was already failing before my changes, I couldn't figure why.

I've been very conservative with my changes and I didn't go beyond the strict necessary to make things work. As with most porting, the trickiest change is the unicode/str/bytes thing. I went with unicode-everything.

At two places, you'll see a if not PY3: encode to utf8, that's because strangely, libxml requires bytes when under python2 and refuses bytes when under python3...

Does this look like something you'd be willing to merge?

@shaunix
Copy link
Contributor

shaunix commented Mar 17, 2015

Definitely interested in this, thanks. And this is the right place to push. I'm in the process of moving itstool from gitorious to github. Website's not updated yet. Does libxml2 work with Python 3 now?

@ghost
Copy link
Author

ghost commented Mar 17, 2015

Yes, libxml2 runs on python 3. I ran itstool tests with both Python 2.7 and 3.3. Apparently, support for python 3 was added 2 years ago

@shaunix shaunix self-assigned this Jun 17, 2015
@Keruspe
Copy link

Keruspe commented Oct 16, 2015

Any updates about this?

@jjardon
Copy link
Contributor

jjardon commented Oct 28, 2015

This is interesting as itstool is one of the few modules that still depend on python2 in GNOME

Maybe other option is to use python-lxml instead the python-libxml2: http://lxml.de/ ?

@Keruspe
Copy link

Keruspe commented Oct 29, 2015

I don't think libxml2 is the problem here, the python bindings of libxml2 work just fine for python 3.4 and 3.5 at least

@jjardon
Copy link
Contributor

jjardon commented Oct 29, 2015

My system doesnt have python2 at all, so I built libxml2 against python3; itstool fails to build because a missing libxml2 python module

@ghost
Copy link
Author

ghost commented Oct 29, 2015

@jjardon oh, lucky you! I'm trying to get there as well, hence this PR.

I'm pretty sure that libxml2 supports python3 because with this PR, I could run itstool under py3 (I'm on Gentoo). Maybe it's only that python3 bindings of libxml2 is not available on your system? Then it would be a distro problem.

@juergbi
Copy link

juergbi commented Nov 9, 2015

The configure check in itstool is broken, it assumes the 'python' binary is what you want to use, instead of $PYTHON. The 'python' binary/symlink typically points to python2 and doesn't exist on pure python3 systems. Fixing this doesn't appear to be sufficient, though. itstool fails here trying to build evolution.

Traceback (most recent call last):
File "/usr/bin/itstool", line 1516, in
doc.apply_its_rules(not(opts.nobuiltins), params=params)
File "/usr/bin/itstool", line 724, in apply_its_rules
self.apply_its_file(os.path.join(itsdir, dfile), params=params)
File "/usr/bin/itstool", line 754, in apply_its_file
if not nss.has_key(nsdef.name):
AttributeError: 'dict' object has no attribute 'has_key'

@ghost
Copy link
Author

ghost commented Nov 9, 2015

@juergbi are you sure you're using a version of itstool with this PR applied? If you look at https://github.com/hsoft/itstool/blob/c2e1824671747030234dab895400699cbc1d99be/itstool.in#L772 , in apply_its_file(), you'll see that has_key(), the source of the crash here, is not used.

@juergbi
Copy link

juergbi commented Nov 9, 2015

No, I tested with the wrong version. Sorry about that. The configure issue still needs to be fixed, though.

@Keruspe
Copy link

Keruspe commented Dec 27, 2015

If this PR fixes at least most of the python3 compat while keeping python2 working, maybe it should be in a new release so that we can gather some testing and feedback?

@Keruspe
Copy link

Keruspe commented Jan 16, 2016

I've made some testing, with this patch applied, it still works just as before with python2.

When testing with python3, I hit a problem though.

Error: Could not merge translations:
must be str or None, not bytes

@Keruspe
Copy link

Keruspe commented Jan 16, 2016

I've tracked it down to the parent.addContent call in append_credits

841-            if val is not None:
842-                val = val.encode('utf-8')
843:                parent.addContent(val)

Any idea @hsoft @shaunix ?

@ghost
Copy link
Author

ghost commented Jan 16, 2016

@Keruspe I'm not well set up to test itstool right now. I'm guessing that wrapping the val = val.encode('utf-8') line in a if not PY3: would do the trick. Could you try that?

@Keruspe
Copy link

Keruspe commented Jan 16, 2016

@hsoft applied that, and now I'm getting another error, unicode related:

Traceback (most recent call last):
  File "/usr/bin/itstool", line 1553, in <module>
    fout.write(doc._doc.serialize('utf-8'))
UnicodeEncodeError: 'ascii' codec can't encode character '\xe8' in position 312: ordinal not in range(128)

This fixes three problems at once:

1. When fiddling with credits nodes, we would get an error about libxml2
expecting `str` rather than `bytes`. We could fix this by encoding the
value only when uner py2.

2. When writing the merged XML, our serlialized data would be `str`
under py3, which would cause implicit encoding problems when writing
that contents to the file.

3. `fout` would not be closed after writing, which would sometimes cause
the target file to end up with no contents at all (at least on my
machine.
@ghost
Copy link
Author

ghost commented Jan 16, 2016

I had problems reproducing the crash because I don't use itstool directly so I don't really know how to actually use it, but I could finally reproduce it by:

  1. Cloning evince (https://github.com/GNOME/evince)
  2. Go to help/C
  3. Run python /path/to/itstool/itstool.in -i /path/to/itstool/its/mallard.its -m ../fr/fr.mo textselection.page

Strangely though, when applying my first fix, I didn't have the same bug as yours, but a whole different one: my target file (textselection.page) became empty!

So, I fooled around and ended up fixing 3 bugs, in the commit reference above. See commit message for details.

@Keruspe does these fix solve your problem as well?

@ghost
Copy link
Author

ghost commented Jan 16, 2016

Oh, yes, one thing: ../fr/fr.mo doesn't actually exists in evince's repo, I msgfmt'd it manually first.

@Keruspe
Copy link

Keruspe commented Jan 16, 2016

@hsoft nice, I confirm it works like a charm now! (confirmed on all the packages that previously failed here)

@Keruspe
Copy link

Keruspe commented Jan 25, 2016

@shaunix Any chance you'd give it a go to merge and make a release?

@shaunix
Copy link
Contributor

shaunix commented Feb 15, 2016

So I finally decided to look into merging this, but first I need to get through shaving the yak of figuring out why test_IT_keep_entities_1 is failing.

@ghost ghost mentioned this pull request Apr 12, 2016
@Keruspe
Copy link

Keruspe commented Jun 27, 2016

@shaunix any luck with your tests?

@shaunix
Copy link
Contributor

shaunix commented Jul 8, 2016

The test failure is due to this libxml2 bug:
https://bugzilla.gnome.org/show_bug.cgi?id=762110

@Keruspe
Copy link

Keruspe commented Nov 30, 2016

@shaunix so it fails with python2 too, right?

If the only blocker is a test filaing and the same test is also already failing on master, is it really a blocker?

@Keruspe
Copy link

Keruspe commented Jul 14, 2017

Any news regarding this?

@shaunix
Copy link
Contributor

shaunix commented Aug 15, 2017

Finally got that ridiculous test failure fixed. Now maybe we can get on with Python 3 before Python 4 becomes a thing.

@shaunix shaunix merged commit 204ed57 into itstool:master Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants