temporal: Update and vendor ply dependency per repo's instructions as no longer published #5942
Conversation
…ved in newer version
github-actions
bot
added
windows
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for n, s in enumerate(syms): | ||
| if s[0] in "'\"": | ||
| try: | ||
| c = eval(s) |
Check warning
Code scanning / Bandit
Use of possibly insecure function - consider using safer ast.literal_eval. Warning
There was a problem hiding this comment.
From the following error message, "Literal token %s in rule %r may only be a single character" it seems ast.literal_eval may be enough, but I didn't get anything beyond that. We can make sure we test well enough our use cases (and we do have a good coverage of temporal things already) and just try to replace it.
There was a problem hiding this comment.
Do you want to do changes to the upstream code right away in the initial PR, or try to have them later on so we can separate out changes from the upstream changes? We have to think for the next time we sync the code
There was a problem hiding this comment.
If we don't add a README (or similar) with list of patches added, we should add the original version as is. Then git history will tell what modifications we have made.
There was a problem hiding this comment.
Git is the way to go because it will be automated as opposed to keeping a list. The security check produces a warning, but will let us merge without anything special. Let's merge the original, unpatched version.
|
PLY comes with ctypesgen (https://github.com/OSGeo/grass/tree/main/python/libgrass_interface_generator/ctypesgen/parser). Not sure if we should use that one instead… |
At the time, I didn't want to couple the ply version from ctypesgen into our runtime code. Its already hard enough to get ctypesgen updated, so I left their version alone, it works with their code. Ply, in the python grass.temporal code, is distributed at runtime. I think ctypesgen is meant to be a compile-time tool, and not really distributed. |
| for n, s in enumerate(syms): | ||
| if s[0] in "'\"": | ||
| try: | ||
| c = eval(s) |
Check warning
Code scanning / Bandit
Use of possibly insecure function - consider using safer ast.literal_eval.
|
PLY in ctypesgen could have been used, only copied at build. But, the embedded PLY in ctypesgen does have minor patch(es), so we should avoid that.
As I understand it, it will not be updated with pip and similar anyway. |
Whatever you think is the best. I don't get what role ctypesgen plays in PLY. It is using existing C headers?
Anyway, including PLY sounds as the right solution. |
It doesn't. It only itself embeds an older copy, and usually we don't have ctypesgen available at runtime once installed (at least we don't want to, it's a build time tool, and ply is an implementation detail). The implementation detail part is the reasoning of the author of not publishing newer releases. As most projects should end up tuning their use of it. |
Ctypesgen uses PLY, in a central role for parsing C headers, and has it embedded, and in extension we already have it in the GRASS repo. But ctypesgen's embedded version contains minor "hacks", and is the latest released version 3.11 (from 2018). This PR adds the current snapshot of PLY (from master), last updated in 2022, which is not ideal but I suppose ok. |
|
Oh, that's the other way around than I thought, that makes more sense. Let's have another PLY for our (direct) use, then. |
Closes #5917
I updated my old branch that updated the ply dependency with a vendored copy, as per the author's instructions.
Tests still pass in CI, and it is also adjusted to be built with CMake. The dependency is removed from the various lists.