Fix libffi recipe, and build + runtime linker errors when compiling on WSL#1744
Merged
AndreMiras merged 8 commits intokivy:masterfrom Mar 20, 2019
Merged
Conversation
Force the use of lld (LLVM's linker), and include a python37 bugfix. https://clang.llvm.org/docs/Toolchain.html https://lld.llvm.org/ https://bz-attachments.freebsd.org/attachment.cgi?id=200526
Libtool didnt like the space after --sysroot, replaced with =
Aralox
changed the title
|
Hi @Aralox , I tried your branch, but I see this error: Fedora 29
I am not sure how to help :( |
Contributor
Author
|
Hi Robert, thank you for testing my branch! Try installing (deduced from |
|
Thank you @Aralox I was reading BTW, I can confirm your change is correct, if fixes Be sure you add For Fedora 29 sudo dnf install lldGood work! |
Member
|
Thanks to you both, it actually useful to see that error messaging popping up. |
Contributor
Author
|
Can confirm clang is used to compile libffi in the docker container, so the dependency on lld is required there. Thanks Andre! |
AndreMiras
reviewed
Mar 18, 2019
Member
|
Awesom work @Aralox! It seems to work and you also simplified the code, thanks! I've tried compiling an app without |
|
@AndreMiras may be ubuntu docs won't need an update, but I think Fedora will, I can add that dependency for Fedora if you guys want |
Member
|
Sure @robertpro you can pull request the Fedora docs once #1744 gets merged. I'm also exited about it being merged, just want @Aralox to take a final look at my last comment to make it optional and I'll merge. It's definitely on my top prio this week 😃 |
Contributor
Author
|
Hey Andre, sorry for the confusion, I don't think I explained very well in my initial post, but to fix the libffi compile, I discovered that lld was not actually required at all. Using it initially fixed compile issues, but I realised that the root cause was deeper which was why I went ahead and fixes the recipe. Edit: sorry again! I misread your comment, I thought the exitcode referred to the libffi compile. What you have suggested with the check for lld existence makes perfect sense, it allows backwards compatibility, and gives developers a clear step to take if the libffi compile fails. Yeah it looks great. Quite busy this week but will try to squeeze in the change one of these nights |
Member
|
Great I've read your edit and yes you exactly got my point, it should smooth the transition to lld 👍 |
opacam
reviewed
Mar 20, 2019
opacam
reviewed
Mar 20, 2019
opacam
reviewed
Mar 20, 2019
Changed hardcoded cpu count to cpu_count(). Added comment to python patch. Made remove-fix-cortex-a8.patch conditional on lld. Moved LDFLAGS change to get_recipe_env() so its not libffi specific within python.py.
Contributor
Author
|
The Travis CI build seems to use python 2.7 for |
Member
|
Thanks for the update @Aralox! And that Python2 is super annoying, but glad the CI could catch that. import sh
assert sh.which('foobar') is None
assert sh.which('ls') is not NoneOtherwise with |
for python2 compatibility.
Contributor
Author
|
Nice man, you are really quick! I've changed it as you suggested, I think it looks better too. I'll see how it goes tomorrow, I definitely need to sleep now! 😴 |
AndreMiras
reviewed
Mar 20, 2019
Member
|
It looks pretty good thanks. I'll test it again without Edit: |
Contributor
Author
|
I tested with and without lld on armv7 and x86 emulators, all looks fine. Happy to drop it from the docker file tonight, or it looks like github has a feature where you can suggest an edit right here and I can click accept, maybe we can give that a go? I'm on my phone |
Member
|
I've dropped it. I'll squash and merge if the build goes green. |
AndreMiras
approved these changes
Mar 20, 2019
Member
AndreMiras
left a comment
AndreMiras
left a comment
There was a problem hiding this comment.
Build is green, ready for the merge
Contributor
Author
|
I'm just messing around in the same general area of code, and saw that |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Force the use of lld (LLVM's linker), and include a python37 bugfix.
https://clang.llvm.org/docs/Toolchain.html
https://lld.llvm.org/
https://bz-attachments.freebsd.org/attachment.cgi?id=200526
I use p4a in windows bash (windows subsystem for linux). Upgrading from the stable version 0.7.0 to the develop version 0.7.1 causes linker errors when building libffi:
This is partially due the fact that we now use clang to compile libffi instead of gcc. I fixed the problem by
forcing the clang invocation to usefixing the libffi recipe, by adding an equals sign = between thelld, which is llvm's linker--sysrootflag and value in LDFLAGS (see commit messages). If this PR is merged,lldwill now be a requirement to use p4a. I think this is fine since we are using clang anyway (and it is awesome).EDIT:
lldis now not required, libffi builds successfully with regularldnow, but I have left in my deltas which change the linker tolld, as I feel like it should be the way forward. Let me know what you think.The
-L.flag is just me incorporating the fix of a bug in python37, as linked above.I've updated the dockerfile for python 3 to include an
lldinstall, but please let me know if I should update any documentation etc to indicate thatlldis required. Thanks!