Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--requirements=android gives crash #1504

Closed
RobertFlatt opened this issue Dec 7, 2018 · 7 comments
Closed

--requirements=android gives crash #1504

RobertFlatt opened this issue Dec 7, 2018 · 7 comments
Assignees
Labels

Comments

@RobertFlatt
Copy link
Contributor

RobertFlatt commented Dec 7, 2018

Versions

  • Python: 3.6
  • OS: Ubuntu 18.04.1
  • Kivy: whatever p4a downloads
  • Cython: 0.29.1
  • p4a: master as of 12/6/2018

Description

Three command line option issues:

  1. "--requirement=android" causes p4a crash

If .p4a contains these two lines:


--bootstrap=sdl2
--requirements=python3,kivy,android

p4a fails because --bootstrap value is encoded as a str not utf-8

The problem is with encoding of bootstrap string in:
/usr/local/lib/python3.6/dist-packages/pythonforandroid/recipes/android/__init__.py


bootstrap = bootstrap_name = self.ctx.bootstrap.name.decode('utf-8')

Assumes the --boostrap option is encoded as 'utf-8', I think all options are 'str'; except the requirements option which is re-coded to 'utf -8'.

A workaround is removing the decode(), this fixes the problem for me


bootstrap = bootstrap_name = self.ctx.bootstrap.name

  1. Presplash

--presplash depends on the 'android' package, though there is no easy way for a user to know this.

But 'android' is not loaded by default.

Enhancement request: Add 'android' to 'requirements' if it is not specified, and --presplash specified

  1. Ndk_api

toolchain.py line 268 :


            '--ndk-api', type=int, default=None,

is missing an argument, change to


            '--ndk-api', '--ndk_api', type=int, default=None,

Command:

p4a apk

.p4a file:

--dist_name=yy
--private .
--package=com.xxx.yy
--name Yy
--bootstrap=sdl2
--requirements=python3,kivy,android
--arch=armeabi-v7a
--permission INTERNET
--orientation sensor
--icon chart.png
--presplash chart.jpg
--sdk_dir /someplace/androidtools/sdk
--ndk_dir /someplace/androidtools/android-ndk-r17c
--ndk_version 17c
--android_api 28
--ndk-api 21
--version 0.3.14

Logs


Traceback (most recent call last):
  File "/usr/local/bin/p4a", line 11, in <module>
    load_entry_point('python-for-android==0.6.0', 'console_scripts', 'p4a')()
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/toolchain.py", line 1032, in main
    ToolchainCL()
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/toolchain.py", line 554, in __init__
    getattr(self, args.subparser_name.replace('-', '_'))(args)
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/toolchain.py", line 152, in wrapper_func
    build_dist_from_args(ctx, dist, args)
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/toolchain.py", line 192, in build_dist_from_args
    build_recipes(build_order, python_modules, ctx)
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/build.py", line 635, in build_recipes
    recipe.prebuild_arch(arch)
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/recipes/android/__init__.py", line 36, in prebuild_arch
    bootstrap = bootstrap_name = self.ctx.bootstrap.name.decode('utf-8')
AttributeError: 'str' object has no attribute 'decode'

@KeyWeeUsr KeyWeeUsr self-assigned this Dec 8, 2018
@ghost
Copy link

ghost commented Dec 9, 2018

@KeyWeeUsr shouldn't self.ctx.bootstrap.name ideally always be a unicode string? (In an ideal world, I mean) Seems to me like the .decode(...) in the recipe shouldn't be necessary in the first place... or does p4a commonly use raw bytestrings for options when running with python 2? I know some legacy python 2 apps do that, but unicode everywhere still seems like a better idea, at least for things like user-specified command line options

(sorry for the wall of text and I'm aware this is not necessarily helping with fixing the issue, I'm just curious)

@RobertFlatt
Copy link
Contributor Author

RobertFlatt commented Dec 9, 2018

@Jonast @KeyWeeUsr The proposed fix has the goal of not breaking anything else. Argument values are hardcoded and are used all over the place, changing the encoding is very likely going to break something somewhere - creating a new issue is not really a fix!

Context: As I read it (correctly??), all arguments are parsed as str, and only --requirements values are then cast to utf8. This inconsistency is probably the cause of the issue; and this cast has no utility because does not increase the usable hardcoded character set.

These hardcoded strings must be easily available on many keyboard types, must be a lowest common denominator. Allowing utf8 in the hardcoded values creates a potential problem. The world it turns out is not ideal ;)

I do think consistency would be a good thing, but changing --requirements to str would also introduces a risk too.

@ghost
Copy link

ghost commented Dec 9, 2018

This project is full of hacks and these hacks tend to pile up and become a nightmare to maintain. Not that I'm opposed to this quick fix, but I personally prefer to understand what is going on, and what (possibly bigger change) will also avoid similar problems in the future.

However, this is also most likely a non-issue in Python 3 because that one has a much better string design, so maybe we should just move faster to deprecate Python 2 entirely.

@RobertFlatt
Copy link
Contributor Author

@Jonast I totally agree about the nightmare. To address that I'd suggest 'str' as the lowest risk path to consistency. Starting from scratch I would perhaps make a different suggestion. Of course whoever actually picks ups the issue gets to choose.

@AndreMiras
Copy link
Member

Thanks for discussing it, I only see that topic now sorry.
Anyway it's fixed in #1512
I agree with you guys the fix is maye not ideal as I commented in the PR, but it's still better than the current situation (broken build).
I'm closing for now, if you feel it still requires some further investigation/refactoring feel free to reopen or open a dedicated issue to investigate.

@RobertFlatt
Copy link
Contributor Author

@AndreMiras Thank you.

@KeyWeeUsr
Copy link
Contributor

Ref #1516.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants