Add ctypes workaround to bootstrap (fixes ctypes.util.find_library() crash) #1433
Conversation
| result_buf[targetoffset] = '\\'; | ||
| targetoffset++; | ||
| result_buf[targetoffset] = 'n'; | ||
| } else if (inline_code_arg[sourceoffset] == '\n') { |
There was a problem hiding this comment.
I didn't follow the discussion, but you probably mean '\r' here, right?
There was a problem hiding this comment.
Oh right. \r in paths are so rare I wasn't sure whether to put it in at all, but yea that's supposed to be \r
| * occurances of \n and \r with \\n and \\r for inline string literals. | ||
| * (Unix paths can contain line breaks, so to be super safe, we need this) */ | ||
| char *escape_quotes_of_inline_string(const char* inline_code_arg) { | ||
| char *result_buf = malloc(strlen(inline_code_arg) * 2 + 1); |
There was a problem hiding this comment.
I don't see where this is freed?
Edit: never mind I found it 😄
There was a problem hiding this comment.
Good point, I should really add a note to the comment above the function that it returns something that needs to be free()'d
| @@ -66,6 +66,38 @@ int file_exists(const char *filename) { | |||
| return 0; | |||
| } | |||
|
|
|||
| /* Helper function to escape all occurances of ' as \', and all | |||
| * occurances of \n and \r with \\n and \\r for inline string literals. | |||
There was a problem hiding this comment.
Hi, i understand you have a fast fix, but that the kind of stuff that nobody will look and change later. IMO, it should not be merged because you implies that all the apk requires ctypes, which is not the case. There is build where you can blacklist. A such patch make it a implicit requirement.
Plus, this code is only for ctypes, but execute for everybody at the start.
A last one, the patch you did is only for SDL2 bootstrap, while all the boostrap are impacted.
Best is to adapt your code to make it work under python3crystax and prepare it for python3 recipe that will be here in a matter of days. You may just be able to fix it using the previous patches done for Python 2!
|
Also, it has an impact on current Python 2 apps, as the patch done in the source will be applied after this one. Unsure about the implication of both running on another. |
|
For what it's worth, I updated it to catch a possible import error (no longer requiring Edit: and yes, since this has been broken for months, at this point I'm kind of looking for a "quick fix". I mainly stepped in because it didn't seem like this was going to be addressed soon, and it's quite a pain to work with |
|
Based on @tito 's feedback, it seems to make the most sense to close this. I am nevertheless already using this pull request for myself because I'm not interested in moving this hack back into my program, but that's my own problem for now Edit: I'll not be making a follow up pull request for this specific issue though, unless there is some consensus on the exact way it should be done. Aimlessly playing around with various variants of this hack doesn't feel like the best use of my limited expertise for the project |
|
Followup attempt is here: #1517 |
This adds the missing ctypes workaround for Python 3. Since it's in
start.cit will apply to any sort of Python 3 used, no matter if it's Crystax or any other python install that lacks a properly patchedctypes.util.find_library()function.While this should probably be patched in the Python 3 itself in the long run, I suggest this fix is merged some time soon because:
ctypes.util.find_library()function, it shouldn't render it inoperableFor my app, this worked perfectly fine and allowed me to get rid of ugly import order issues related to monkey patching this in the app code. It'd be very nice to have this finally fixed upstream.
See also ticket #1337