[WIP] Modify Python scripts to run on Python 2 and 3 #90
Conversation
|
I'll do a test build on OS X and Linux before merging. Assigning to myself. |
|
@gribozavr Thanks; I haven’t had a chance to set up a build system yet. |
|
@alexwlchan Sorry, but the OS X build failed when it tried to run gyb: |
|
@gribozavr Sorry, that’s probably not surprising. I should have tested it first. I’ll get myself a build system and keep trying. I’ll ping you when it’s done.
|
alexwlchan
changed the title
|
@alexwlchan: As a suggestion, you could create two |
|
@Mitame Please don't duplicate the whole script. If we need compatibility wrappers for certain APIs, we can declare them in the script itself. |
|
@gribozavr: I wasn't going to, it was, as i said, a suggestion. |
|
Also, |
|
@Mitame I think I can do without separate scripts here. It’s a fairly small difference, and easy to fix. The only difference is the method names:
But there’s a Python builtin I should have been a bit more careful when running six. I’m running an Ubuntu build now to see if I can hit the problem above; then I’ll drop in the fix and make sure I’ve really fixed it. Then onwards – there are probably more edge cases that I’ve missed. |
|
I was cosidering |
|
@Mitame For the RAM issue, please build with |
|
@gribozavr That info would be nice in the Readme if it's not already there. Also, the issue seems to arise when it gets to the linking part. When resuming it, it has ~80 things left if that helps at all. |
Another error running the build in python3 |
|
@dmshynk Thanks, looks good. I’ll get those merged into my branch ASAP. I’ve still broken Python 2 support in a few places, so I want to get that checked first before I squash and we get this merged. |
|
@alexwlchan I'm not a Python expert but I'm pretty sure this has to do with Python 3 because if I use Python 2 I don't get the error. Anyways when I run I know effectively nothing about Python but I will see if I can't track down what is causing this and how to fix it. |
|
@RLovelett So this error gets raised when the parser finds the same section more than once in the config file. In this case, the section if Fix is either to rearrange the config file, or run the parser in non-strict mode. I’ll try to include a fix. |
|
(I’m quite busy this weekend, so only have sporadic moments to dig into build problems. Hoping to get passing builds on Pythons 2 and 3 by the end of next week. I think I’ve squashed most of the bugs.) |
|
Thanks for the pointer. I've come up with this patch: From e25b9a954d2c0c34777a789b84f0565bf5528d6a Mon Sep 17 00:00:00 2001
From: Ryan Lovelett <ryan@lovelett.me>
Date: Sat, 5 Dec 2015 15:00:50 -0500
Subject: [PATCH 12/12] Allow parsing of old style on Python 3
---
utils/SwiftBuildSupport.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utils/SwiftBuildSupport.py b/utils/SwiftBuildSupport.py
index 5631d3b..7048924 100644
--- a/utils/SwiftBuildSupport.py
+++ b/utils/SwiftBuildSupport.py
@@ -120,7 +120,7 @@ def check_output(args, print_command=False, verbose=False):
def _load_preset_files_impl(preset_file_names, substitutions={}):
- config = ConfigParser.SafeConfigParser(substitutions, allow_no_value=True)
+ config = ConfigParser.SafeConfigParser(substitutions, allow_no_value=True, strict=False)
if config.read(preset_file_names) == []:
print_with_argv0(
"preset file not found (tried " + str(preset_file_names) + ")")
--
2.6.3Unfortunately, it doesn't work on Python 2 (it fails with |
|
@alexwlchan I've actually found another approach which was just to stream line the section declarations. I've submitted a stand-alone patch, #248, because it should be able to work on both 2 and 3. In case, it were to be merged before this one. |
|
I've spent a bit more time looking at this. Still failing on Python 3, at these few lines in GYBUnicodeDataUtils.py: code_point = 0x200b
code_point = ('\U%(cp)08x' % { 'cp': code_point }).decode('unicode_escape')
as_UTF8_bytes = code_point.encode('utf8')
as_UTF8_escaped = ''.join(['\\x%(byte)02x' % { 'byte': ord(byte) } for byte in as_UTF8_bytes])Specifically, I'm struggling to come up with an implementation that produces identical results on I'll continue to chip away at this, but any suggestions gratefully appreciated. I've also rebased against the latest master to keep up with the latest fixes. @dmshynk - I can't get your proposed patch for this to work. I get this error: Python 3.4.3+ (default, Oct 14 2015, 16:03:50)
[GCC 5.2.1 20151010] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> code_point=0x200b
>>> code_point = (b'\U{cp}08x'.format(cp=code_point)).decode('unicode_escape')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'bytes' object has no attribute 'format'Any ideas? |
|
Rebased to resolve merge conflicts. |
|
@alexwlchan I'm getting two test failures on OS X: |
|
This is great work! Thanks!! 💯 Would it make sense to split this pull request up? It would be easier to review and merge the changes if they were made in smaller chunks. Similar changes also need to be applied to |
| @@ -61,7 +61,7 @@ def __init__(self, obj): | |||
| self._obj = conf.lib.sourcekitd_request_dictionary_create( | |||
| POINTER(c_void_p)(), POINTER(c_void_p)(), 0) | |||
| self._as_parameter_ = self._obj | |||
| for k,v in obj.iteritems(): | |||
| for k,v in obj.items(): | |||
There was a problem hiding this comment.
Splitting hairs, but: you put a space in between k,v in your changes to utils/GYBUnicodeDataUtils.py. Why not place a space in between these two as well, to promote consistency?
|
@modocache Indeed, it’s become a bit unwieldy, and I‘ve let my fork fall behind. Rather than try to tidy this up, I’m going to break it up into smaller PRs and refer back to this one. |
|
@alexwlchan Would you mind resolving the conflicts and squashing into one commit? |
Python 3 made a number of changes to dicts and map/range/filter to be more efficient -- rather than returning a list, by default they use a memory-efficient iterator. This meant some methods were renamed, and sometimes we really do need a list. This commit (i) gets all the method names right and (ii) adds explicit casting to lists in cases where it really is needed.
|
@gribozavr Rebased against master and tidied up the commit history, so hopefully it’s a bit easier to review now. Pretty sure it’s still broken on Python 3 – I think there are some string subtleties that the automated conversion tools won’t pick up – but I think this time it should still be working on Python 2. I don’t have time to do a test right now; I’ll try to do one later. |
|
@akyrtzi How do I test |
|
@alexwlchan I have been reviewing your patch, and I think that in order to make progress it would be better to handle it slightly differently. For example, I don't know how to test all Python scripts (like the SourceKit Python bindings). The easiest thing to do would be to split the pull request into multiple:
It would be really easy to review and test the first PR because all scripts are exercised during the build. Other PRs will be reviewed in a distributed fashion by the people who wrote them or use them, so they should be able to verify that the change does not break them. I'm sorry for the back-and-forth here, but I'm concerned about breaking scripts that I don't use or even don't know how to use. |
|
@gribozavr That sounds sensible. I’m going to close this PR, and then reopen other PRs as you suggest. I didn’t realise quite how large a change this would be when I started. |
|
Thanks @alexwlchan! |
|
|
Thanks, @akyrtzi, good to know! |
…swiftlang#90, swiftlang#91. Address ABI FIXME swiftlang#68 by using same-type constraints directly on an associated type to describe the requirements on the Indices associated type of the Collection protocol. ABI FIXMEs swiftlang#89, swiftlang#90, swiftlang#91 are all in StdlibUnittest, and provoke warnings once swiftlang#68 is fixed, but it's nice to clear them out. Fixes SR-2121.
Some minor refactorings, NFC
…precommitcheck Rename check to project_precommit_check.
Done in swiftlang#90 but not updated here.
Debian packaging for Ubuntu 20.04
Some systems use Python 3 as their default interpreter (e.g. #83).
This is a collection of fixes that should get these scripts much closer to running on both Python 2 and 3. Obtained by running the six module over every .py file in the repo, and going through the proposed changes by hand.
There are probably still a few gotchas that will break in Python 3 – but this gets substantially closer to a fully working set.