when listing distributions, if one has no ndk_api, consider it to be 0

[tshirtman]

this avoids crashes, but also ensure that it doesn't consider it a
match, is that something we want?

[tshirtman]

@inclement @AndreMiras opinions welcome!

[AndreMiras]

Thanks for looking into this!
I thought we had a default value setup somewhere. But seeing Distribution.ndk_api is None that's probably not what I was looking for 😄
What about just totally skipping it when the key is not available, just like we did for 'archs' in dist_info above?
I haven't played with it so I'm not sure if that would make sense. @inclement is your man since he made a large refactoring on this recently.

[tshirtman]

Yeah i was wondering about just setting to None, but i now think it's not going to make much of a difference, since this is only checked if user asked for a specific api, this way may trigger a few unnecessary distribution rebuild, and some unused space, but it's probably a lesser evil than trying to guess what api was used in an older distribution if we fail.

[DanShai]

Thanx @tshirtman your solution solved my problem !

[inclement]

The intended solution is to delete and rebuild the dist as ultimately using a dist with an unknown API target can't be recommended, but this wasn't a major point. Setting a default value is acceptable, it may as well be zero. It would also be good to print a warning in this case.

[tshirtman]

Ok, i'll add a warning, would something like:

"Distribution {dist} has been built with an unknown api target, ignoring it, you might want to delete it".

represent the intention well?

[inclement]

Sounds good to me!

[AndreMiras]

distdir seems unused here

[tshirtman]

I though it would make it easier for user to remove it… but i can remove it if it's not relevant.

[AndreMiras]

No sorry what I mean is the format string seems to add a param that's not used, right?

[ghost]

Yea kind of looks like (distdir) should be {distdir}

[tshirtman]

uh, i misread that >_<, thanks for your patience 😆

[ghost]

Please note an ndk_api of 0 possibly breaks this line:

https://github.com/kivy/python-for-android/blob/master/pythonforandroid/distribution.py#L98
(or at least did for me, the ndk_api wasn't set somewhere but that made it default to 0 instead of None, but the distribution was detected to have an api version (not 0) so this condition broke it being located properly - you should evaluate carefully whether this can happen in this case as well, and whether this line needs to be adjusted)

This is why I made this pull request: #1484 just to provide some background

So I highly recommend against merging this unless you either address this, or you're certain that this won't be an issue (I might be wrong, obviously, I'm not sure either if this code path can be hit in this case).

[DanShai]

When it was None I got tons of errors and couldnt build the apk, but @tshirtman solution worked like charm ! 0 is better and buit the apk on the fly. if you woried bout that line just replace:
if ndk_api is not None --to--> if ndk_api
thats it and leave it at 0

[ghost]

Right, that might be one way to address it, but then also the argument parser default value for --ndk-api (in toolchain.py) would need to be set to 0. My main intention was to point out that in the current form this pull request might clash with some things, not that None is the way to go or anything like that

[ghost]

See my comment, with None sometimes being used for ndk_api instead of 0. I think it should be either None or 0 everywhere, not a mix of both (this broke things for me at one place, which is why I changed the --ndk-api default in a pull request recently)

[ghost]

Another line that may need changing in relation to this pull request: https://github.com/kivy/python-for-android/blob/master/pythonforandroid/distribution.py#L215

Might be worth searching for ndk_api is None all over the code before going ahead with this change

[ghost]

Hm. Thinking about this some more, why can't we fix the code that breaks handling None? It is obviously a special case, using 0 might risk running through code without warnings/errors that should actually care about the ndk_api... so I'm not sure this is the right direction. But I wrote too much already, I'll let you guys discuss this and figure something out

[tshirtman]

It's good feedback, i had looked at the line you mention and assumed that it wouldn't be an issue, but didn't think that ndk_api could be 0, indeed, that wouldn't be good, maybe setting to None would be better, that way we are sure this condition will evaluate to True and the distribution is not matched.

Anything against that?

[ghost]

As elaborated above, I like None, but I can't claim to know all the potential consequences of this change 😬

[inclement]

As a general note, this is important only for the handling of existing dists that predate the ndk_api setting. It should not be possible to create new dists with p4a that don't have an NDK API. The change here should be seen in that light: we don't want dists with a missing NDK API to actually work well (if at all), just to not crash p4a.

[AndreMiras]

Thank you for clarifying @inclement then do we want to address it this way in this case?
I mean if at the end the apk is not going to work, why don't we fail fast with a meaningful error during the build? Rather than failing late at runtime?

[ghost]

@AndreMiras I might be wrong, but I think the change here means that rather than the presence of an old, non-ndk build crashing p4a always, it will no longer disrupt build of a proper distribution with ndk version when any old distribution is still present - with this warning advising the user to remove that old outdated distribution, since it won't be built or be recognized anyway.

(at least if --ndk-api is specified, a distribution with .ndk_api being None would be filtered out by get_distribution in distribution.py. maybe an additional change could also ensure it's never picked up even when --ndk-api is unspecified, that might be worth an addition?)
edit: corrected func reference

Nevertheless, I think this pull request is useful, because the warning is much more useful than a random backtrace, and I also find it useful to not block building a more recent project just because an older, incompatible distribution happens to be lying around. But of course one could still abort the entire build always but with a nice error. However, I prefer the solution here (given I understood things correctly)

[ghost]

@tshirtman what do you think about changing this line:
https://github.com/kivy/python-for-android/blob/master/pythonforandroid/distribution.py#L98
to this:
if (ndk_api is not None and dist.ndk_api != ndk_api) or dist.ndk_api is None:
?

(The idea being that a distribution with .ndk_api is not set is invalid, and this would ensure it's never picked up even when --ndk-api is unspecified by the user)

[ghost]

I haven't actually TESTED it, but just code-wise this all looks reasonable to me.

[AndreMiras]

looking good