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

Consider arch when looking for compatible dist. #1716

Closed

Conversation

Aralox
Copy link
Contributor

@Aralox Aralox commented Feb 19, 2019

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).
@AndreMiras
Copy link
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
Copy link
Contributor Author

Aralox commented Mar 18, 2019

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
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
Copy link
Contributor Author

Aralox commented Sep 15, 2019

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
Member

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

@opacam
Copy link
Member

opacam commented Sep 15, 2019

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
Copy link
Member

inclement commented Sep 15, 2019

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

@opacam
Copy link
Member

opacam commented Sep 15, 2019

@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
Copy link
Member

inclement commented Sep 15, 2019

@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
Copy link
Member

opacam commented Sep 15, 2019

@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
Copy link
Member

inclement commented Mar 30, 2020

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