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

Added a matplotlib recipe #1822

Merged
merged 16 commits into from
Jun 3, 2019
Merged

Conversation

inclement
Copy link
Member

@inclement inclement commented May 21, 2019

Finally got this working. This isn't in a mergeable state due to the changes it makes to other recipes and some parts that could be improved, but it's surprisingly close.

The necessary steps seem to be:

  • Build freetype as a shared library
    • Probably matplotlib can include the static library somehow, but I couldn't work it out
    • I disabled harfbuzz to make this easy, otherwise the .so file has a missing hb_ft_... symbol. Possibly this can be fixed by the three-stage build (freetype-without-harfbuzz then harfbuzz-with-freetype then freetype-with-harfbuzz) that you can read about online.
  • Build png as a shared library
    • Again, was easier than making matplotlib include a static library
  • Set the matplotlib config properly
    • Basically, don't build all the unnecessary gui stuff
  • Patch matplotlib for some final things
    • Patch the numpy version to match p4a's - matplotlib downloads a numpy wheel to compile against, and it needs to match the version that the app will actually load. Could be improved by making the numpy version dynamic, or by just letting matplotlib link against a local numpy that p4a has built in the local env.
    • Prevent TkAgg from being built, matplotlib seems to ignore the setup.cfg option for this. It has C++ compile issues that could probably be fixed, but aren't relevant since Tk won't work anyway.

I haven't tested it widely, but the testapp seems fine with basic plotting:

mpl2

@inclement inclement added the WIP label May 21, 2019
@AndreMiras
Copy link
Member

Awesome work!
Well if it's not breaking existing recipes and if CI build is happy, I won't be against an early merge. Or at least I won't wait to have it perfect.

@inclement
Copy link
Member Author

For now it probably does break other things using freetype and libpng, since it makes the recipes build shared object files instead of static libraries. Also freetype no longer uses harfbuzz, which presumably leads to worse results. Both things can be addressed.

@inclement
Copy link
Member Author

I was working to fix the last couple of issues but my phone's broken, so it won't be done for a few days.

Along the way, I was going to delete the PIL recipe (depends on png which I've changed) since pillow seems to work fine for us. Any objections to that?

@opacam
Copy link
Member

opacam commented May 26, 2019

I've no problem on that, but maybe, instead of removing it, we could redirect the Pil recipe to Pillow (with a message warning the user about that) so we could avoid users complaining about a non working Pil recipe...

As a side note: as far as I know, the Pil module it has been unmaintained for years...Pillow it's the replacement and it's maintained...so it makes sense to me to redirect the build from Pil to Pillow

About this:

I disabled harfbuzz to make this easy, otherwise the .so file has a missing hb_ft_... symbol. Possibly this can be fixed by the three-stage build (freetype-without-harfbuzz then harfbuzz-with-freetype then freetype-with-harfbuzz) that you can read about online

Some time ago I fixed this cyclic dependency issue with freetype and harfbuzz for personal use and never created a pr...seeing this pr I remembered that, and I updated that job to produce shared libraries so it will be compatible with this recipe, now it's ready to create a pr...so.... do you want that I create this pr? (I don't want to interfere with your work...maybe you already have this cyclic dependency issue solved and you are planing to push it in here or in another pr...)

@inclement
Copy link
Member Author

@opacam I'd be delighted if you do a PR for that, it should be no problem to merge to master and rebase this one on it. Did you do the apparently-correct build process of first building a bare freetype, then building harfbuzz depending on that freetype, then building freetype depending on that harfbuzz?

@opacam
Copy link
Member

opacam commented May 26, 2019

Yes, that is the process...
The problem is that adds a little complexity to the build...

@inclement inclement force-pushed the matplotlib_recipe branch from af7ee7d to cd3d18f Compare May 28, 2019 21:11
@inclement
Copy link
Member Author

This seems to work now, I tested pillow as well with the changes to libpng and it was able to load, modify and save a png image fine.

I'm not completely happy about copying libpng.so to libpng16.so and then loading it explicitly in PythonUtil.java, but I couldn't get matplotlib to link with it otherwise and it seems basically fine. I guess I could have left it as libpng.so in the apk (and the autoloading should take care of finding the symbols), but it isn't clear that this is actually a better choice - if matplotlib wants to link to libpng16, maybe other things do as well.

Overall, I think it's okay to leave a further decision to if/when we have more recipes wanting to link with libpng.

@inclement
Copy link
Member Author

I've replaced the pil recipe with a wrapper that prints a warning and then builds pillow. This seems to work fine, and should avoid backwards compatibility issues.

Also modify Pillow recipe because depends on png
@opacam
Copy link
Member

opacam commented May 30, 2019

I'm not completely happy about copying libpng.so to libpng16.so and then loading it explicitly in PythonUtil.java...

Yes, I don't like either this, we already had some discussion about the load of the libraries in that file (and teorically we don't need to do that... as you said) so seeing png's recipe I remembered that I myself put those comments in there. Yesterday night I tried to move libpng to the mainline repo and it seems to work fine. I may add that the final generated library is precisely libpng16.so the name that you need to make it work the matplotlib recipe. Also the soname of the library it's the same libpng16.so.

The fact that the soname it's the same that the library filename make the things work without touching the javafile, at least on my tests with Pillow, but should be the same with matplotlib...so...could you try with this png recipe removing all the changes you made to png recipe and without the PythonUtil.java changes?

Note: since we already had surprises with freetype and harfbuzz cyclic dependency issue, I think that would be great that we could test this with another system than mine or travis

The png recipe: ⬇️

@inclement
Copy link
Member Author

I've merged and tested @opacam's changes, it works fine. PIL also works, tested loading a png, modifying, and saving again.

I think this can be merged now.

@opacam
Copy link
Member

opacam commented Jun 3, 2019

Great @inclement!!
we almost have it, do you mind to fix those tox errors please?

https://travis-ci.org/kivy/python-for-android/jobs/540940495#L238-L244



MatplotlibApp().run()
runTouchApp(Image(source='test.png', allow_stretch=True))
Copy link
Member

Choose a reason for hiding this comment

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

I think that we could remove this line, I'm right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that's an old debug line, thanks.

@inclement inclement force-pushed the matplotlib_recipe branch from 2cdc3ce to f7f0451 Compare June 3, 2019 21:09
Copy link
Member

@opacam opacam left a comment

Choose a reason for hiding this comment

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

Thanks @inclement, Good job!!

@inclement inclement merged commit 3658089 into kivy:master Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants