Repository navigation
Upgrade PLY to 3.6 - #4003
Upgrade PLY to 3.6#4003
Conversation
|
Since we keep the PLY-generated files in the repo it's fine if it doesn't work on Python 2.6 I guess. 👍 |
|
It might conflict, but I can always fix later... |
There was a problem hiding this comment.
Heh. Turns out this function was the cause of the reference cycle I've been trying to track down in #4002 . I need to double check but I'm almost certain this should fix it.
There was a problem hiding this comment.
Yep, confirmed. This is all it was.
There was a problem hiding this comment.
Well, there was also #4012, so there were actually 2 reference cycles involved in this. But I think this is the one that I was actually being affected by.
There was a problem hiding this comment.
The old version of this code is PLY is some serious voodoo magic.
There was a problem hiding this comment.
It's a fairly nice trick to avoid having to use sys._getframe. I think they've given up on making sys._getframe a strictly non-public function though. Way too many people have discovered and made use of it in production code by this point :)
There was a problem hiding this comment.
The docs basically say "don't expect this to exist on anything but CPython". I shudder to think of all the other problems astropy would have running on anything but CPython, so I consider that a non-problem for us...
|
@mdboom Would you mind rebasing this so I can merge it? Thanks. |
9904502 to
ef901f3
Compare
|
Rebased. |
There was a problem hiding this comment.
Hm, that's a pain. IIRC I did this by hand last time these were updated.
Maybe (if you haven't already) I'll submit a patch to PLY to do this? (Certainly worth keeping this script around in the meantime though.)
There was a problem hiding this comment.
Yeah -- a patch to PLY would be helpful. I haven't got around to that yet.
|
I'll do the patch. I already sent one to objgraph the other day so I'm on a roll :p |
|
Granted, a patch to PLY won't involve the "license" line, but maybe we could morph that to a general tool that checks source files for that and adds it where it's missing. |
|
And on second thought, is this patch (or these touch-ups) really necessary in general? |
|
That's true. I guess I thought we were still supporting Python 3.2. If that's not the case then this doesn't matter. Why don't we keep the tables as-is for now, and I'll file a quick PR to remove the little dev tool. |
|
Hmm, I think we're only supporting Python 3.2 for 1.0.x (and poorly, at that, since travis-ci doesn't run 3.2 anymore, though I still test locally....sometimes...). I'm fine either way. |
|
Ok -- so we'll just keep as-is for now until Python 3.2 support is dropped, I guess. |
Reminded by #4001, we haven't upgraded PLY in a while. Notably 3.6 includes a Unicode-related patch to PLY that we've been keeping locally all this time. Moving to 3.6, though requiring one minor backward-incompatible change related to file generation, allows us to use a "stock" PLY where desired.
Note that PLY no longer officially supports Python 2.6. It works on our unit tests, so I think we're fine for now, but let that be another nail in the coffin of Python 2.6.
Related to this, PLY still generates code that isn't Python 2 and 3 cross-compatible, so it requires some modifications. I should submit a PR against PLY for that.