[WIP] Fix crashes when using other commands than 'apk'

[ghost]

I broke things with the setup.py pull request 😂 commands other than p4a apk appear to be broken, from my test this fix makes it work again, sorry 😬

[nitanmarcel]

@Jonast This PR works but now there's a new error" Namespace object has no attribute private

[ghost]

@nitanmarcel can you give me either the backtrace or the source code line in which that error occurs? it's likely also something broken by the setup.py pull request

[nitanmarcel]

@Jonast in the line 202 at build_recipes(...

[ghost]

did you use a non-sdl bootstrap by any chance? service or webview or something?

[nitanmarcel]

did you use a non-sdl bootstrap by any chance? service or webview or something?

Nope. Honestly I don't have any ideas what I'm doing 😆 I'm using buildozer in a env with pyenv

[nitanmarcel]

@Jonast The command buildoizer is trying to execute and fails:

# Command failed: /usr/bin/python3.7 -m pythonforandroid.toolchain create --dist_name=rexteserdroid --bootstrap=sdl2 --requirements=python3,kivy,pillow,git+https://github.com/HeaTTheatR/KivyMD --arch armeabi-v7a --copy-libs --color=always --storage-dir="/home/alexandrumarcel/python_apps/rexdroid/.buildozer/android/platform/build" --ndk-api=21

[nitanmarcel]

This little one liner fixed it for me 🍰

    build_recipes(build_order, python_modules, ctx, args.private if hasattr(args, "private") else None,
                  ignore_project_setup_py=args.ignore_setup_py if hasattr(args, "ignore_setup_py") else False,
                 )

[ghost]

@nitanmarcel yeah I just tried something similar 👍 still testing whether it blows up the non-buildozer build, could you give the updated pull request a retry in buildozer?

[nitanmarcel]

@Jonast yes, it didn't raised any exceptions so far so the latest fix worked

[nitanmarcel]

Wasn't better to add the following arguments to the non APK ones? Or that would completely break p4a? @Jonast

[nitanmarcel]

Everything working on installation using other commands than apk

[inclement]

Thanks for the quick fix @Jonast, just one minor issue I'd prefer to do a different way.

[inclement]

I would prefer using () instead of \ for line breaks.

[ghost]

oops yea that's reasonable, changed it!

[inclement]

I think it would be neater to move the --use-setup-py etc. arguments to generic_parser so that they're always in the args object, instead of using getattr in different places. It isn't a great api to have to stick non-generic stuff in generic_parser like this, but I think it's a failure of the way the abstraction is set up rather than something specific to this particular argument. There are plenty of other things in generic_parser for the same reason.

[ghost]

I disagree, I think it shouldn't be in the generic_parser if it's not for the other commands. I'd rather have the special case/getattr handling that is admittedly ugly, but still allows me to understand to which commands the arguments belong, BOTH as a coder and a user. (generated parser help texts would list all options even for other commands, right?) Therefore, moving it into generic_parser seems like a cheap hack to me

However, as usual if you really think that's how it should be I will change it accordingly, so your call 👍

[inclement]

Merging to get this fixed as it's a major issue, but I really dislike having hasattr/getattr spread around like this, I think it is a real code smell (arising from the argumentparser structure and how the arguments are used, not your mistake).

[ghost]

@inclement I agree some other solution might be nicer. but I think just putting many things into generic_parser is worse, and I'm not too sure what is better. maybe a wrapper object that will return None on non-existing attributes that don't start with _ or something? I'm really not sure Edit: no that's horrible, we'll typo args and get None all the time, that's a horrible idea forget about that

[ghost]

Ok, another idea: what if all our parsers derive from a custom class instead of ArgumentParser which will somehow globally collect all the args, and which overwrites parse_args to return some sort of wrapper object that will return None for any attribute not present only if used as arg by another known parser?

[inclement]

Well, that would be a neat but complex solution!

I was thinking that the simplest option, which I'd still consider an improvement, would be to have an explicit step in between parser.parse_args and everything else that sets default values for any attributes we expect to access but might not have been created.

I guess there are various other options using a more sophisticated ArgumentParser subclass, including what you've suggested.

[ghost]

@inclement I am willing to give that ArgumentParser subclass a shot if you think that might be worth the complexity. I would definitely personally prefer it over hardcoding args in some make-sure-they're-around function (that's essentially what you're proposing, right?)

[inclement]

(that's essentially what you're proposing, right?)

Right, but it's not what I'm proposing, it's what we already have since I never fixed this since originally implementing it :p

@inclement I am willing to give that ArgumentParser subclass a shot if you think that might be worth the complexity

I don't think it's worth actively focusing on unless you particularly want to, but I'd welcome a good rethink of p4a's argument parsing structure if it made the whole thing less smelly. Your proposal could be nice, but I don't have a strong vision of what it would end up looking like.

[ghost]

@inclement I was bored (and curious) and gave it a try: #1812