Fix librt recipe requires that NDK folder is writable #1623
Conversation
ghost
changed the title
|
Why handling it like an usual recipe, e.g. it has it's own build dir (not temporary) and we leave the "build" artifact and just link to it? |
|
@AndreMiras ah lol yeah that would make a lot more sense. Is |
ghost
changed the title
ghost
changed the title
ghost
changed the title
|
changes done & tested with my app |
|
I agree with the general value dislike of adding this type of thing, but it seems pretty reasonable and I've thought about similar options before. Ultimately, we probably do want a generic way for recipes to say 'other recipes may want to include/link with me'. |
pythonforandroid/archs.py
Outdated
| # Link the extra global link paths first before anything else | ||
| # (such that overriding system libraries with them is possible) | ||
| env['LDFLAGS'] = ' ' + " ".join([ | ||
| "-L'" + l.replace("'", "'\"'\"'") + "'" |
There was a problem hiding this comment.
I'm not completely clear, what is this fixing?
There was a problem hiding this comment.
@inclement this is just adding these new global link options as proposed, and adding them first so they take precedence. (actually now that you say it I'm not sure the -L order matters so maybe the comment is nonsense and it could have been added at another point in that function, but then again I still don't see a reason to move it somewhere else) The replace is there because it should be, it does proper quoting in all cases for paths including space, single quotes, ...
There was a problem hiding this comment.
I think the -L order does matter, just wasn't clear what the effect of the escaping is. I'm happy to believe it does the right thing, but maybe it could go in a different quote_path function to be clear?
There was a problem hiding this comment.
@inclement this escaping is the ultimate(TM) shell escaping which should be used everywhere - if it was, then we'd have no issue with spaces in folders and all these other things.
It's pretty simple as well, if you think about it: single quotes basically disable everything that could do anything ($, space chars, ...) - except for terminating single quotes. So the single quotes themselves are escaped by '"'"', spaced out this is: ' " ' " '. Explanation: it terminates the current single quote block, then adds a single quote in double quotes (to avoid starting a new block and keep it as a literal single quote), then starts a new single quote block directly afterwards to continue regularly.
I got this escaping method from shlex.quote actually, I believe?! I forgot. Maybe we could actually use that function, I tend to forget it exists. Anyway, my point was I didn't make this up myself but smarter people did, so the escaping method should be solid 😄
There was a problem hiding this comment.
Sure, I understand what it's doing and I totally believe it's correct, it's just hard to judge it without knowing the rules of quoting offhand. It sounds like using shlex.quote would be the best option.
There was a problem hiding this comment.
@inclement you're right, it should just be shlex.quote 👍
(The NDK folder can be installed system-wide and may not be writable, so without this fix the librt build crashes in such an environment)
ghost
changed the title
ghost
changed the title
|
build ran through fine for me with the |
|
Build failed because shlex.quote isn't present yet in Python 2, maybe that's why it wasn't used in the first place? That was the only issue though, so merging :) |
|
Wait isn't it kinda bad if it fails under python 2? Should I make another pull request to revert it to the explicit quote |
|
For now it only means the boost recipe fails under python2, because otherwise there's nothing in extra_global_link_paths. Also, I'm getting tired of making things harder to support python2 :p If you want to change it back to the explicit replace, that would be okay though, just maybe comment that it's only there because shlex.quote isn't available in py2. |
|
@inclement it also affects reportlab and greenlets I believe, so the fallout isn't that tiny. I'll make a pull request to change it back with a comment |
ghost
deleted the
This fixes that the librt recipe requires the NDK folder to be writable. The NDK folder can be installed system-wide and may not be writable, so without this fix the librt build crashes in such an environment. This fixes #1621
Fixes the build for me! Feel free to test & merge! (or to tell me what to change)