Fix ctypes.util.find_library() crash with live patch on startup

[ghost]

Fix ctypes.util.find_library() crash with live patch on startup. This is a new variant based on the new common bootstrap area of the previous attempt in #1433

EDIT: just realized semantics are wrong, Edit 2: -- > fixed

[ghost]

Tested it with my actual real world app, and it works! Since this is vital for using Python 3 + ctypes without monkey-patching around, it would be very useful to have this merged for everyone

[ghost]

I confirm the semantics fix to be working with my real-world app, including with the PySDL2 handling removed. As far as I'm concerned, this is ready for merging

[inclement]

Maybe could log something here, like "Could not import ctypes, build your app with libffi if you want ctypes to be present". I think that could help people.

[ghost]

I put that in now!

I'm just curious though, is libffi shipped per default and would need to be opted out of with some option, or does it actually need to be put into --requirements for ctypes to be present at all? Because an opt-out intuitively would seem to make more sense to me, in which case people would already be aware and the warning wouldn't be that important, would it?

[AndreMiras]

Would it make sense and be easy enough to have this large string loaded from a file instead? That way it would make it easier to edit in case it's needed?

[ghost]

The clean way of how to do that would I think be to install it into the site-packages, but that might be a little complex to look at. (Like, harder to figure out for people who try to understand how the boot process works) The other alternative I can think of is adding the file to the asset folder, and then directly open()ing it in that inline python and using exec() or something.

I'm a bit undecided on how much 'better' these two solutions are. They would certainly be easier to edit, but I'm not sure if it wouldn't also be more confusing about what is going on at boot.

Edit: also the parameters would need to be passed from C in any case, so at least the escaping code this wouldn't get rid of

[ghost]

Basically I can't think of the most obvious mechanism and it would introduce additional complexity, so I'm a bit torn

[AndreMiras]

OK thanks 👍

[AndreMiras]

I made minor comments, but it's looking overall good to me. I just need time to actually test.
I'm not too sure the presence of ctypes module has something to do with libffi. I think ctypes is simply shipped by default, unless explicitly blacklisted via p4a blacklist.txt files.
What do you guys think about it?

[AndreMiras]

This is a rather super large try/except block 😮
What would you think about some more like:

char code_with_placeholder[] = (""
    "orig_func = None\n"
    "try:\n"
    "    import ctypes.util\n"
    "    orig_func = ctypes.util.find_library\n"
    "except ImportError:\n"
    "    print('Could not import ctypes, build your app with '\n"
    "          'libffi if you want ctypes to be present')\n"
    "def android_find_library_hack(*args):\n"
    "    import os\n"
    "    name = args[0]\n"
    "    \n"
    "    # Truncate ending for easier comparison:\n"
    "    if name.find('.so.') >= 0:\n"
    "        name = name.partition('.so.')[0]\n"
    "    if name.endswith('.so'):\n"
    "        name = name[:-len('.so')]\n"
    "    if not name.endswith('.'):\n"
    "        name += '.'\n"
    "    \n"
    "    # Helper function to check lib name:\n"
    "    def check_name(lib_name, search_name):\n"
    "        if filename.endswith('.so') and (\n"
    "                filename.startswith('lib' + search_name) or\n"
    "                filename.startswith(search_name)):\n"
    "            return True\n"
    "        return False\n"
    "    \n"
    "    # Check the user app lib dir and system dir:\n"
    "    app_root = os.path.normpath(os.path.abspath(os.path.join(\n"
    "        '%s', '..', '..', 'lib')))\n"
    "    app_root_arm = os.path.join(app_root,\n"
    "                                'arm')  # fixme: other archs?\n"
    "    sys_dir = '/system/lib'\n"
    "    for lib_search in [app_root, app_root_arm, sys_dir]:\n"
    "        if not os.path.exists(lib_search):\n"
    "            continue\n"
    "        for filename in os.listdir(lib_search):\n"
    "            if check_name(filename, name):\n"
    "                return os.path.join(lib_search, filename)\n"
    "    try:\n"
    "        return orig_func(*args)\n"
    "    except OSError:\n"  // catch bogus error about missing "sh"
    "        return None\n"
    "if orig_func is not None:\n" // only patch if ctypes module is present
    "    ctypes.util.find_library = android_find_library_hack\n");

[ghost]

good point, I also just noticed I'm polluting the global namespace with orig_func and even worse, if user code deletes/changes that global variable it'll break. I changed that now but I still need to test that change

[inclement]

I think ctypes is simply shipped by default, unless explicitly blacklisted via p4a blacklist.txt files. What do you guys think about it?

The python configure/setup.py simply don't build ctypes unless they can find libffi. I think the other python builds bypassed this by implicitly building libffi in the python2 case, and by explicitly building it during the python bundling in the crystax case. With this in mind, the libffi requirement actually looks like the correct p4a way to handle things, although I didn't realise this until looking into it to work out why ctypes wasn't already working.

[AndreMiras]

Good to know, thanks @inclement

[ghost]

@inclement ok but then it might make sense to throw libffi into the default base recipe set anyway, wouldn't it? (similar to pyjnius) in that case the usual app would always ship with it included, although of course having a sane error handling message still doesn't hurt

[inclement]

@Jonast Yep, I agree. Sorry, I know you suggested that before, I forgot to reply to that one.

[ghost]

I did? I'm very forgetful 😂

[ghost]

Tested the new change in real world app, works!

[ghost]

@inclement ok wait, isn't the new pull request by @opacam fixing openssl for python 3?

If yes, isn't this pull request here kinda stupid now? Given the new python3 recipe always builds from scratch so a patch would be a cleaner solution, and crystax is kinda unnecessary, outdated and overly complicated anyway?

Seems to me like we should not merge this and I need to do a proper patch as it is done against python 2 instead

[AndreMiras]

Agree to see if #1537 gets merged with its patch. And also agree for not putting CrystaX specific efforts 👍

[ghost]

I'm closing this just in case, don't think it makes sense to have this merged now

Edit: I'll happily help out with patching the python 3 recipe, don't worry