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

Make Cython work without recipe #1483

Merged
merged 1 commit into from Jan 13, 2019
Merged

Make Cython work without recipe #1483

merged 1 commit into from Jan 13, 2019

Conversation

ghost
Copy link

@ghost ghost commented Nov 23, 2018

What this does: This merge request makes Cython work for the non-recipe regular build. This means CythonRecipe is now unnecessary if the project doesn't need Android-specific extra changes. That's good because a recipe with no actual content is redundant, and because CythonRecipe is problematic:

Why CythonRecipe is problematic: I noticed CythonRecipe results in the packages own setup.py not run for Cythonization, and instead, p4a engages some manual Cython compilation which is broken per design for a couple of reasons:

  1. it assumes all .pyx files anywhere in the project dir must compile. This breaks projects that have any .pyx files that are not meant to be built for the main build and break when Cythonized out of the blue
  2. it uses the wrong site-packages because it uses host python. This breaks .pxd cross-includes from any dependencies, since this requires proper site-packages during Cythonize (this affected me, see .pxd files of dependency targeted recipe'd library not found #1473 )
  3. it assumes the setup.py won't produce new .pyx files on its own. This breaks projects where that happens

(Please note this pull request doesn't fix CythonRecipe in any way, it just makes it avoidable for Android-aware Cython projects)

Needs quite some testing, obviously!

@ghost
Copy link
Author

ghost commented Nov 23, 2018

Side note: I'm unhappy about how this pull request makes things based on CythonRecipe for the env vars of the regular build, since I think the CythonRecipe should be a special case of special needs Cython projects, not the other way around. However, I didn't take care of this yet because I wanted to first see if this works for other projects at all, and then gradually move stuff out of CythonRecipe and into the core later, stripping down CythonRecipe to its special uses in that process.

(Also, it would move quite a lot of stuff around, so just for cleanliness I think it'd be nicer to have it in a separate pull request later)

@tito
Copy link
Member

tito commented Nov 23, 2018

About your 1. just to let you know Kivy source code have pyx that doesn't work and required on Android (like window_x11.pyx, etc). So maybe you should have an option to give a list of files to ignore?

@ghost
Copy link
Author

ghost commented Nov 23, 2018

About your 1. just to let you know Kivy source code have pyx that doesn't work and required on Android (like window_x11.pyx, etc). So maybe you should have an option to give a list of files to ignore?

That's like a hack of another hack. The real problem is that the setup.py already does this for any sane project, and p4a's CythonRecipe basically assumes the setup.py never works for any project for Cythonizing which IMHO is just a bad assumption. (It would be ok as a manual per-recipe enabled workaround, but as a default behavior for Cython projects I don't think it makes a lot of sense)

Also, I don't have any non-compiling .pyx files, I was just explaining why CythonRecipe is problematic.

(Nevertheless, thanks for your input! Interesting to know @ kivy, I wonder how it works around this - but still, I don't find it directly relevant for this pull request)

@ghost
Copy link
Author

ghost commented Nov 23, 2018

It appears to be working mostly fine, but I can't fully test since I'm currently stuck on #1487

edit: resolved, since #1487 is now fixed

@ghost
Copy link
Author

ghost commented Nov 24, 2018

Edit: resolved (was an error due to wrong LDFLAGS env var)

@ghost
Copy link
Author

ghost commented Nov 26, 2018

I spent quite some time making the process much more similar to the CythonRecipe, even down to stripping the object files at the end - but sadly, I still get above error. Edit: now solved! yay

@inclement or @AndreMiras you wouldn't happen to have a clue why with the above gcc command line, I appear to be getting broken .so files that give that cannot locate symbol "PyExc_DeprecationWarning" error?

edit: done, see below

@ghost
Copy link
Author

ghost commented Nov 26, 2018

Ok thanks to a hint from @AndreMiras and some more debugging I CAN NOW CONFIRM IT WORKING for python3crystax, full build process up to deploying & actually running the app.

@ghost
Copy link
Author

ghost commented Nov 26, 2018

(this is still WIP though, I want to clean up some stuff - but I'd love to hear test results!! Even if your app doesn't use Cython, just to make sure other things aren't broken by this)

edit: done, see below

@ghost ghost changed the title [WIP] Make Cython work without recipe Make Cython work without recipe Nov 28, 2018
@ghost
Copy link
Author

ghost commented Nov 28, 2018

Okay, I cleaned up the parts I still wanted to clean up and it works for me.

To test this and give me useful feedback, it is enough to just build your project with this pull request. While your project/libraries would need adjustments to use the new functionality, the much more important question is whether this pull request breaks existing stuff. It changes the regular non-recipe python module install env, so it has the potential to cause some interference, so some wider testing wouldn't be a bad idea.

Anyone who's interested in testing, please do and report back here! It works fine for me is all I can say

@ghost
Copy link
Author

ghost commented Nov 30, 2018

Just a tiny note: latest force pushes were just rebasing, not changes. It works for me, feel free to test to see if it works for you!

@ghost
Copy link
Author

ghost commented Dec 20, 2018

@AndreMiras by the way, any clue on when we're going to merge this? It's pretty much been sitting here for three weeks, I use it for my active project and it's done as far as I'm concerned... so IMHO it would make sense to review & merge this if you guys want this feature. I personally think moving away from recipes where they aren't strictly necessary is always an improvement, but that's just my view on things

Edit: force pushes are just me rebasing, I'm not changing it or anything

@inclement inclement merged commit febdb3d into kivy:master Jan 13, 2019
@inclement
Copy link
Member

Thanks, sorry for the delay on this one.

info('Stripping object files')
if self.ctx.python_recipe.name == 'python2legacy':
info('Stripping object files')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: isn't this redundant with the one two lines above?

@ghost
Copy link
Author

ghost commented Jan 13, 2019 via email

@AndreMiras
Copy link
Member

Well that's not a big deal anyway, very good work as usual and sorry for letting the PR rot for so long. Great that inclement took over and merged

@ghost ghost deleted the cythonfix branch January 31, 2019 02:41
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.

3 participants