Skip to content

Consider arch when looking for compatible dist.#1716

Closed
Aralox wants to merge 2 commits into
kivy:developfrom
Aralox:consider-arch-when-looking-for-dists
Closed

Consider arch when looking for compatible dist.#1716
Aralox wants to merge 2 commits into
kivy:developfrom
Aralox:consider-arch-when-looking-for-dists

Conversation

@Aralox

@Aralox Aralox commented Feb 19, 2019

Copy link
Copy Markdown
Contributor

Cache self.dist so it isn't calculated multiple times (e.g. wrapper_func in toolchain).

Current problem scenario:

  • I build for arm
  • I build the same project for x86. p4a uses existing distribution and packages arm libraries into the apk instead of recompiling project for x86

I also modified _dist() as it was getting called multiple times, causing the dist search (and log output) twice.

Cache self.dist so it isn't calculated multiple times (e.g. wrapper_func in toolchain).
Comment thread pythonforandroid/toolchain.py Outdated
Comment thread pythonforandroid/distribution.py Outdated
@AndreMiras

Copy link
Copy Markdown
Member

Thank you for your contribution 🍺
I didn't get time to really wrap my head around it and play with it yet. Just left a couple of comments

@Aralox

Aralox commented Mar 18, 2019

Copy link
Copy Markdown
Contributor Author

This pr isn't urgent. Workaround is to run p4a clean_dists between compiling for different architectures. I think the other pr currently in progress for upgrading to ndk 19 might already take this into account.

@inclement inclement changed the base branch from master to develop June 6, 2019 20:40
@AndreMiras

Copy link
Copy Markdown
Member

Hi @Aralox is it still relevant after changes from #1722 ?
If I understood correctly, we can verify this by compiling for two different archs without running a clean between and checking if the APKs run OK on both. If the issue is still here, the second arch we build for would have compiled dists from the previous one hence should crash, correct?

@Aralox

Aralox commented Sep 15, 2019

Copy link
Copy Markdown
Contributor Author

Hi Andre yes I think that's exactly it. I haven't really kept up with the progress of this project so I'm not sure if the other PR addresses this (it probably does), but yeah that would be the verification. Feel free to close this PR if unnecessary. Cheers :)

@inclement

Copy link
Copy Markdown
Member

@AndreMiras As far as I can see, #1722 doesn't affect this and it's still an issue, am I missing something?

@opacam

opacam commented Sep 15, 2019

Copy link
Copy Markdown
Member

Yeah, agree with @inclement, I don't see any conflict with the NDK-r19 migration thing...but I just now realize that this will probably conflict with #1926 (as the author of this last one, no problem at all, I will fix conflicts if convenient 😉).

@inclement

inclement commented Sep 15, 2019

Copy link
Copy Markdown
Member

I'm dealing with this in a new PR. I'm considering it urgent for the next release.

@opacam

opacam commented Sep 15, 2019

Copy link
Copy Markdown
Member

@inclement, good to know 👍, feel free to use the commits I made at #1926, related with this arch problem, I see two:

Of course, feel free to implement it whatever way you want, you may have a better idea 😄

@inclement

inclement commented Sep 15, 2019

Copy link
Copy Markdown
Member

@opacam I'm feeling inclined to make a simpler PR that addresses just this for now, in a slightly different way, with #1926 left to merge for the next release. I don't want to merge #1926 quite yet because I'm concerned it may be a little overcomplicated. I'll add some thoughts about that later.

@opacam

opacam commented Sep 15, 2019

Copy link
Copy Markdown
Member

@inclement, no problem at all, I agree that #1926 complicates things... if we can slim it in some way better for all of us 😉

so go for this new PR 👍!

@inclement

inclement commented Mar 30, 2020

Copy link
Copy Markdown
Member

Closing as I think this was resolved elsewhere, but thanks for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants