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

Fix LDSHARED and missing CFLAGS include path for python3crystax sources #1361

Merged
merged 1 commit into from Sep 11, 2018
Merged

Fix LDSHARED and missing CFLAGS include path for python3crystax sources #1361

merged 1 commit into from Sep 11, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 6, 2018

As proposed in #1360 this change fixes env["LDSHARED"] not being set which can confuse distutils / python setup.py build_ext to use the incorrect host compiler, and it adds the -I folder to the python3 crystax sources when using python 3 crystax. This should allow a lot of recipes to be cleaned up later which currently have the same repeated hacks to work around this. (Uncleaned recipes should just duplicate include paths which should do no harm, so this shouldn't require any recipes to be changed to be safely merged)

Testing state:

  • tested with python3crystax without kivy (but with Pillow recipe)
  • not tested with python 2
  • not tested with other recipes

@AndreMiras
Copy link
Member

Thanks, you're a machine! 😂
I'd like to test this change myself, hence killing this flag from the 17 recipes before and after your change and see if the compilation first fails and then passes.
Of course if we had a 100% integration coverage that would be way easier, but as discussed we don't have that yet for various reasons 😕

@ghost
Copy link
Author

ghost commented Sep 6, 2018

@AndreMiras I really need python-for-android for my app, so it is worth it for me to sink some time into getting things fixed. 👍

I just tested this with my actual real world app which is python3crystax and uses the recipes Pillow and pysdl2, and some other pure python dependencies. For this app, this pull request builds & runs fine.

@ghost ghost changed the title Fix LDSHARED and missing CFLAGS include path for python3crystax sources - WIP ! Fix LDSHARED and missing CFLAGS include path for python3crystax sources Sep 6, 2018
@AndreMiras
Copy link
Member

AndreMiras commented Sep 7, 2018

OK good stuff. However it seems to break the Python2 builds 😕 yes I know it's a nightmare.
3 builds are failing with the same error https://travis-ci.org/kivy/python-for-android/builds/425450653

/usr/bin/ccache arm-linux-androideabi-gcc -DANDROID -mandroid -fomit-frame-pointer -D__ANDROID_API__=19 -isystem /opt/android/android-ndk-r16b/sysroot/usr/include/arm-linux-androideabi -isysroot /opt/android/android-ndk-r16b/sysroot -I/root/.local/share/python-for-android/build/python-installs/bdisttest_python2/include/python2.7 --sysroot /opt/android/android-ndk-r16b/platforms/android-19/arch-arm -lm -L/root/.local/share/python-for-android/build/libs_collections/bdisttest_python2/armeabi -Xlinker -export-dynamic -o python \
		Modules/python.o \
		-L. -lpython2.7 -ldl  -L/root/.local/share/python-for-android/build/other_builds/python2/armeabi/python2/python-install/lib -lz    -lm  
Creating directory /root/.local/share/python-for-android/build/other_builds/python2/armeabi/python2/python-install/bin
/bin/sh: 2: --sysroot: not found
Makefile:429: recipe for target 'sharedmods' failed
make: *** [sharedmods] Error 127
make: *** Waiting for unfinished jobs....
/usr/bin/install -c python /root/.local/share/python-for-android/build/other_builds/python2/armeabi/python2/python-install/bin/python2.7
if test -f libpython2.7.so; then \
	if test -n "" ; then \
		/usr/bin/install -c -m 555  /root/.local/share/python-for-android/build/other_builds/python2/armeabi/python2/python-install/bin; \
	else \
		/usr/bin/install -c -m 555 libpython2.7.so /root/.local/share/python-for-android/build/other_builds/python2/armeabi/python2/python-install/lib/libpython2.7.so; \
		if test libpython2.7.so != libpython2.7.so; then \
			(cd /root/.local/share/python-for-android/build/other_builds/python2/armeabi/python2/python-install/lib; ln -sf libpython2.7.so libpython2.7.so) \
		fi \
	fi; \
else	true; \
fi


  STDERR:

You can view the complete error clicking on the "Raw logs" button, e.g. https://api.travis-ci.org/v3/job/425450654/log.txt

Yes sometimes simple change break unsuspected things. I also don't see yet the relation between your changes and the way the build broke, but that's quite usual in p4a 😛
The way I usually debug this is by comparing the ENV before and after my changes, trying to understand what would break.

Edit: oh that might be that trailing quote after -Bsymbolic-functions there https://github.com/kivy/python-for-android/pull/1361/files#diff-569e13021e33ced8b54385f55b49cbe6R116
So now I'm curious why this didn't fail in Python3Crystax build for instance. Maybe this get overridden...

@ghost
Copy link
Author

ghost commented Sep 7, 2018

Yeah, interesting, it definitely didn't fail for me for some reason. Anyway, I fixed the trailing comma now. Let's see if that improves things

@ghost
Copy link
Author

ghost commented Sep 11, 2018

Is there something you have in mind I could test? Or how do we go about making a test branch with the recipes adapted accordingly so we can find out if this works for them?

@AndreMiras
Copy link
Member

No, it's looking good, I do think it works.
I actually addressed the python include part in that PR https://github.com/kivy/python-for-android/pull/1250/files#diff-569e13021e33ced8b54385f55b49cbe6

I had more concern regarding if it should be done that way.
For example why don't we use the actual linker env['LD'] rather than the compiler env['CC']. I know we're doing it in the recipes and I know gcc can link, but well, was just wondering.
Also the env["CFLAGS"] python include part, I'm wondering if it should not sit in PythoRecipe.get_recipe_env() instead or if there's some rework we should do about it.

I'll discuss briefly with other devs on IRC and if it's OK we'll merge.

@AndreMiras AndreMiras merged commit b9ae628 into kivy:master Sep 11, 2018
AndreMiras added a commit to AndreMiras/EtherollApp that referenced this pull request Sep 16, 2018
Since kivy/python-for-android#1361 was merged
upstream, some `LDSHARED` and `CFLAGS` could be cleaned.
@ghost ghost deleted the fix-ldshared-and-cflags branch November 23, 2018 12:35
AndreMiras added a commit to AndreMiras/python-for-android that referenced this pull request Jan 29, 2019
Since kivy#1361 was merged, we can drop LDSHARED override.
AndreMiras added a commit to AndreMiras/python-for-android that referenced this pull request Jan 30, 2019
Since kivy#1361 was merged, we can drop LDSHARED override.
Also pined `secp256k1-py` version and couple of other fixes.
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.

2 participants