Dont always regenerate parsetab #188
Conversation
|
Can one of the admins verify this patch? |
setup.py
Outdated
| @@ -24,7 +24,7 @@ | |||
| 'chardet>=1.0.1', | |||
| 'cryptography>=0.5', | |||
| 'idna>=2.5', | |||
| 'ply>=3.10', | |||
| 'ply==3.10', | |||
There was a problem hiding this comment.
Because if the version of PLY were to change then the format of the generated parsetab files could change requiring them to be regenerated.
There was a problem hiding this comment.
Pinning version in setup.py is not a good idea. Instead I suggest adding logic that checks if the installed ply version is the one that the tab files were generated with. And if it is not, then regenerate them automatically.
There was a problem hiding this comment.
Actually I just glanced through the source of PLY and it looks like it checks the version of PLY the previous table was generated with and if it doesn't match the current version then it regenerates the parsetab file. So I don't think we need to pin the version after all.
There was a problem hiding this comment.
See https://github.com/dabeaz/ply/blob/master/ply/yacc.py#L1988 for the code I'm referring to.
There was a problem hiding this comment.
@mhahnenberg, you are correct that PLY will regenerate the parsetab files if it notices that they're out of date, but the way in which it generates those parsetab files is not safe to do if you have multiple python processes running the same code at the same time. See #206 for more information.
mhahnenberg
force-pushed
the
dont-always-regenerate-parsetab
branch
from
bd47fa6 to
6335859
Compare
|
Could you please squash the commits to keep the history clean. |
|
Sure thing. |
Yacc tries to load the previously generated parsetab.py, but because we're building a bunch of different parsers with different start tokens we always fail to load the cached parsetab.py because the signatures don't match. This diff changes things so that we generate a unique parsetab.py for each of the different parsers. We also commit these parsetabs so they don't need to be regenerated at startup when flanker is imported.
|
@b0d0nne11 seems reasonable, what do you think? Besides this probably should help with #168. |
mhahnenberg
force-pushed
the
dont-always-regenerate-parsetab
branch
from
6335859 to
16c2ba9
Compare
|
Works for me. |
|
Thank you! |
The previous implementation had issues with multiple python processes simultaneously invoking `.yacc(...)` with the same arugments. One of them could be midway through generating a .py file that the other process would attempt to import and then crash with a `SyntaxError`. This diff avoids that problem by instead writing to a tempfile and then atomically renaming it. This means processes starting up at the same time will both do the same work, but that's a lot better than having one of them crash! `flanker` had this issue reported to them in mailgun/flanker#168 and they attempted to work around this by committing the parsetab files that ply generates (mailgun/flanker#188), but this doesn't help if the user is running a different version of ply than the version that flanker generated their parsetab files with (because ply will go ahead and regenerate those files).
No description provided.