Adapt reportlab recipe for python3crystax [WIP] [DONT MERGE!]

[ghost]

DON'T MERGE, OUTDATED. MOST NECESSARY CHANGES ARE ALREADY IN MASTER!

I adopted the reportlab recipe for python3crystax.

Note: I had to change the p4a core to set LDSHARED which for some reason wasn't set. This worked fine with no side effects on everything else I tested (Pillow, pysdl2 recipes for Android) but it could have an effect on some other recipes or platforms which I'm not aware of

Note 2: this shouldn't have any impact on the python 2 use, but I haven't actually tested this

WIP: needs to be adapted to work with #1361

[AndreMiras]

Hi @Jonast thank you for the contribution again.
I'm curious why is this toolchain_include_path needed, could you elaborated a bit more?

Edit:
Travis build not happy on Python3:

• https://travis-ci.org/kivy/python-for-android/jobs/425362933
• https://travis-ci.org/kivy/python-for-android/jobs/425362934

error: [Errno 2] No such file or directory: '/opt/android/crystax-ndk-10.3.2/toolchains/arm-linux-androideabi-5/prebuilt/linux-x86_64/lib/gcc/arm-linux-androideabi'

[AndreMiras]

Why don't you put that in the recipe itself to be sure it doesn't mess up with other ones?

[ghost]

Because it seemed to me like a fundamental issue with the build environment. The problem appeared in python setup.py build_ext, to be more specific, in distutils/unixccompiler.py in function def link when it accesses self.linker_so - that one simply points to the wrong compiler/linker without LDSHARED being set. Of course I could hack around the problem in the recipe, but since the linker is simply set wrong, I felt like this should be solved in the core. I'd be happy to make a separate pull request for it if that's better

[AndreMiras]

OK, I didn't investigate it that much, but my fear is that it could conflict with eventually other recipes options e.g. -static. I usually don't dare to touch the core that much unless I see myself having to add every time the same option to a wide range of recipes.

[ghost]

Well it's LDSHARED, why would that ever be used for a static link? (sorry if that question is naive, I'm only medium familiar with intricate linker options) It's literally called through link_shared_object in distutils/ccompiler.py, I know that since I debugged around in distutils to track this down. I can see why this change would need some more testing though - but I'm all for fixing it properly, even if that will delay inclusion a little

[AndreMiras]

Oh yes sorry I overlooked, I thought it was env['CC'] += ' -shared'. Then yes it may make total sense, thanks.
But I still wonder if it doesn't it's PR on its own, so we also clean all the other -shared added around per recipes. We have a ton of them.
In fact it's been a while that I'm thinking about unit testing it at functional level. Not actually building the recipes, but checking the return of get_recipe_env() across recipes and cleaning it up. I'm not sure how feasible it is.

[ghost]

Maybe for doing that, an integration test might be a good idea. How about setting up a test script that will do p4a ... --requirements=python3crystax,<packagename> for each package independently that has a recipe? Of course that would take a really long time to run, but one could still let it run overnight to get a basic overview over which recipes are currently broken or not

[AndreMiras]

Yes I also believe an integration test is safer, even though it doesn't cover everything. But as you stated it would run for ages. We are doing some continuous integration testing for only few critical recipes and that already takes ages. Not to mention flaky tests #1306
If you take a look at the current CI on Travis, we're running quite some builds in parallel and it still take ages. We're still very early in p4a CI so I'm sure there's room for improvement and help is more than welcome.

[ghost]

Ok so should I do a separate LDSHARED pull request? Or just add env["LDSHARED"] = env["CC"] + " -shared " to the recipe? Or keep things like they are? I'm open for anything, but in any case, a proper value of LDSHARED does seem to be a vital prerequisite for reportlab's setup.py to run through

[AndreMiras]

I would be tempted to say I'm fine either ways. But since I honestly don't know where it should really fit (recipe.py could also be a candidate right?) I would say let's address it later in a dedicated issue, I've just created here #1360
Until then you can pull that get_recipe_env() quick fix I've mentioned in the ticket.
It's just that I think it requires some dedicated attention, it's a long standing technical debt that we should definitely tackle. What do you think?

[ghost]

I think arch.py is the perfect place because that's where env["CC"] and all other things are set up as well. And I don't see how anything else outside of recipes would require a broken env["LDSHARED"] value, so it seems like the proper place to add this. recipe.py would be a recipes-specific fix (as in for all recipes, but not anything else) which doesn't seem right to me

[AndreMiras]

Could you elaborated why is this needed?

[ghost]

See comment below for lengthier explanation. Essentially, stdarg.h is missing somehow from the regular includes provided by CrystaX and hidden away in this toolchain-specific gcc include folder. I don't know why, or why this issue doesn't appear for other recipes that use -nostdinc and include stdio.h - if you have an idea what to investigate, I'll happily look into it. For now, this seemed like the cleanest solution to avoid this issue and also allow other recipes to use this path if necessary

[ghost]

@AndreMiras ok so basically, the recipe uses -nostdinc (like the Pillow one) because otherwise, glibc linux headers will be pulled in. However, as a result, stdarg.h (included by stdio.h) seems to be missing since for some reason, the added CrystaX cross-compile standard headers don't have this - that file is hidden away in the toolchain specific gcc include folder. This folder is now referred to & used by the recipe through this new toolchain_include_path variable. I simply added toolchain_path as well in the process (that one isn't used so far).

Why exactly this happens, and doesn't happen with the Pillow recipe, I have no idea. You can try removing the self.ctx.toolchain_include_path CFLAGS addition in this reportlab recipe and try building it for python3crystax, and you'll see the missing stdarg.h error if you want to look at it yourself.

Edit: to be more specific, remove this CFLAGS addition from the recipe to test:

" -I" + os.path.join(self.ctx.toolchain_include_path)

In the end, this seemed like the best solution to add this without major hacks in only the recipe, while potentially offering this path for use in other recipes as well, should ever the same issue pop up in them. But honestly, I still have no idea why it's necessary in the first place, or why that file would be missing

[ghost]

Regarding the travis build error: that path exists for me in my copy of crystax. Is it possible that the extraction script used in the travis build environment deletes it? It's that toolchain path which includes the missing stdarg.h file deep down in an include folder.

[AndreMiras]

Thanks for explaining!

Is it possible that the extraction script used in the travis build environment deletes it? It's that toolchain path which includes the missing stdarg.h file deep down in an include folder.

Yesn't, it's not deleted, it's just not extracted, because it seemed not used for p4a.
See here https://github.com/kivy/python-for-android/blob/9bef536/Dockerfile#L66

In fact it's probably because you're compiling on for different target archs, could it be?
I don't know yet this part of the code and I don't know what's the status of p4a regarding the support of other archs. But if are supposed/going to support different archs, there's definitely something that needs to be done, also integration with Travis. But I think then it deserves it's how ticket/PR/investigation.

I would then suggest you to give your recipe a try on the "default" armeabi-v7a arch, see how it goes. And if it works with all the core changes, let's make a first simple PR with the toolchain path fixes.
Does it makes sense?

[ghost]

In fact it's probably because you're compiling on for different target archs, could it be?

I'm just compiling for armeabi-v7a as usual, from my x86_64 host. Never did anything else, and I'm not compiling for two targets at once or anything. Maybe it's only necessary for all libs using setup.py in some specific way to specify C extensions, and therefore doesn't affect all libraries, like Pillow and others? I'm really not sure.

I actually ran this on a real armv7a phone to test, so I'm quite sure I successfully compiled for that architecture since PDF export ran through just fine 😄

[AndreMiras]

Ouch OK then, thanks for the update, it helps.
In this case I'd like to invest more time to play with it. Or if another developer wants to pick it up, it's also good.

[ghost]

Well if you have any things to try, let me know. Basically my line of thinking was just: 1. I got an error saying stdarg.h is gone that I need to fix, 2. it indeed doesn't seem to be there in any of the include path folders, 3. it's a bog-standard file that definitely should be there so that sounds like some central include folder is missing, 4. the folder I found & added to the include path appears to be exactly for this toolchain + compiler and happens to contain the file.

That's pretty much all I know, and I have no clue why it is that way or why the other recipes don't run into this. So I'm open for ideas to find out more about this.

[AndreMiras]

Then I would be tempted to work in small safe increments.
My strategy would be to sandbox your required changes as much as possible, hence fixing path and env within "your" reportlab recipe only. I know it feels wrong and unsatisfying as it may just increase the technical debt, but I think it's the safest and easiest for now.
Also the more we see these kind of "hacks" the more it's easy to foresee the needs to update the core and to convince the core devs it's needed 😄
Aside from that, I think you can create either an investigation ticket or directly a PR where we can discuss the need for that toolchains include path update directly in the core.
Does this sound reasonable to you?
By the way, I'm not often there, but there's a kivy-dev on IRC freenode. You can probably talk to inclement there he know that part better and may picture it differently and have different opinion about it.

[ghost]

I literally just added an additional attribute toolchain_include_path which nothing else accesses (hence not really likely to have an influence on anything) - and it's a very SDK-specific path, so I don't think the recipe is the right place to put it. Of course I'm just an unimportant contributor, but if you ask me, it's a much better idea to put this part into the core

As for the LDSHARED issue, I'm preparing a separate pull request right now: #1361

[AndreMiras]

To begin with I'm also just a contributor, but I wouldn't say we're unimportant, otherwise the open source would not be "a thing".
I agree adding it in the reportlab sounds very silly, but I better safe than sorry since I didn't investigate enough. I'm very careful with these changes even though it "just adds" paths for at least the following two reasons:

1. yes it could actually break things, e.g. jugging from the assert you placed here assert(os.path.exists(self.toolchain_include_path)), a good example is the build currently failing
2. it might be a better way to tackle it, I've the feeling this is somehow supported or almost, but we're doing it wrong, so I would love to also investigate that

Again I don't have the same insight as some other contributors or core devs, so perhaps you could give it a try to discuss on IRC.
I'm very glad you made such interesting contributions and I'm definitely willing to look deeper into that one, but I cannot just merge right now without further investigation.

[ghost]

Oh, I forgot. Don't merge anyway, since LDSHARED is now a separate request. I'll mark it WIP, sec - Edit: okay, done.

Right, I forgot about that assert. But if I remove it, the build would still only be failing with this recipe in it - there is no difference to whether I add this in the recipe itself or not regarding that, other than it's ugly to have this just in the recipe (and will require future recipes to hard-code this path as well which seems like a bad idea). Just my two cents

I'm always open for things to try though. I agree that it seems odd this is necessary in the first place, but I'm really out of ideas about what could be the reason

[ghost]

For what it's worth, I just updated the pull request such that LDSHARED and the include path to python3crystax's headers (the two things covered in #1361 ) are assumed to be present.

This means 1. this pull request now depends on #1361 being merged, 2. only the toolchain_include_path addition remains as modification outside of the recipe file itself.

I propose the following steps:

1. We get Fix LDSHARED and missing CFLAGS include path for python3crystax sources #1361 merged first
2. You and/or others decide on whether toolchain_include_path is ok in the core (I still think that's the better place) or whether I should pull that path hard-coded into the recipe. I will adapt this recipe accordingly
3. This reportlab pull request should then be ready for testing & merging

[ghost]

Ok I tested the new variant based on current master with #1361 merged, and it works fine for me! I exported a larger PDF including images and all with no problems.

The travis build appears to fail since toolchains/arm-linux-androideabi-5/prebuilt/linux-x86_64 is missing in the build environment (which is usually present in a non-reduced copy of crystax) - so once that folder is added back, I would expect it to work.

So I would propose that 1. someone tells me whether putting toolchain_include_path for reuse for other recipes into the core is a stupid idea (so I can not do that if necessary), 2. the travis build gets that folder readded back so we can see if it runs through.

After that, someone who uses python 2 could give it a try, and other than that it seems done at least as far as I'm concerned!

[AndreMiras]

I actually could compile reportlab (c088826211ca) after fixing only minor things in freetype and harfbuzz recipes.
Perhaps I would run into some run time issue so I'd like to give it a try also.

[ghost]

So you basically want to revise it further? Not sure what exactly you're speaking of now, but by all means, knock yourself out! I'm not in a huge hurry to get it merged. The only reason I keep pushing it a little is because I tend to forget about side efforts I'm participating in, and things eventually become stale.

I would also be happy to test the result with Python 3 if you point me to what fork/branch archive I should install from to test it! Or if there's anything else I could help try or change about the recipe, let me know

[AndreMiras]

Yes sorry, I have to say I feel embarrassed 😕
So yes I'm currently giving a try building a small app with it to check for run time issues.
I'll create a repo with it and let you know.

[ghost]

Oh I think I may have misread, you were basically saying you want to test it, right? Okay, looking forward to that 👍 somehow I read into it that you want to change things in it after freetype & harfbuzz for some other tech debt cleanup - too much going on, I'm getting confused 😆

[AndreMiras]

In fact it seems to work out of the box when I try with buildozer (I didn't need freetype & harfbuzz fixes). I've created a throw away repo and demo video:

1. https://github.com/AndreMiras/p4a-reportlab
2. https://photos.app.goo.gl/WoGut2jXQQm1xk779

By the way I'm on crystax-ndk-10.3.2-linux-x86.tar.xz not crystax-ndk-10.3.2-linux-x86_64.tar.xz dunno if that could make a difference.

[ghost]

Nice! Yeah, I'm actually using this pull request in my app right now and it's just this pull request on p4a master, no other recipes or other things changed, and that works fine. But I just tested it for Python 3, not Python 2 (since I don't have any code lying around for that and I barely use it, I have mostly 3.5+ only code). Is that also what you tested, or did you use a more extensive pull request combination?

[AndreMiras]

I'm actually using master bare with no changes so not even your PR and it's working perfect even run time.
So at least in my tests your pull request is not needed now.
Well that's since python3crystax was added to depends list (very recently) thanks to yours and that #1182
So I'd like you to give it a try building from scratch on master. And if you get an error please share it again.

[ghost]

Ah! Very interesting. Will do, hang on

[ghost]

Weird, now I'm a bit confused it works for you. This is the build error I'm getting:

Traceback (most recent call last):
  File "/usr/local/bin/p4a", line 11, in <module>
    load_entry_point('python-for-android==0.6.0', 'console_scripts', 'p4a')()
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/toolchain.py", line 973, in main
    ToolchainCL()
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/toolchain.py", line 512, in __init__
    getattr(self, args.subparser_name.replace('-', '_'))(args)
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/toolchain.py", line 147, in wrapper_func
    build_dist_from_args(ctx, dist, args)
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/toolchain.py", line 191, in build_dist_from_args
    build_recipes(build_order, python_modules, ctx)
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/build.py", line 565, in build_recipes
    recipe.prebuild_arch(arch)
  File "/usr/local/lib/python3.6/dist-packages/pythonforandroid/recipes/reportlab/__init__.py", line 37, in prebuild_arch
    text = f.read().replace('_FT_LIB_', ft_lib_dir).replace('_FT_INC_', ft_inc_dir)
TypeError: a bytes-like object is required, not 'str'

This really is what I expect, since in Python 3, opening files in binary mode will always give a bytes-like object, which cannot be combined with unicode strings in any of the find(), replace(), ... whatever standard functions - exactly what the current recipe code ignores.

So why does that not crash for you? Does that part not run, are you building some different version of it somehow?

[AndreMiras]

Oh wait I see, I'm running python2 on host. So buildozer runs from Python2. However I compile for Python3.
So yes I'll give that a try later.
In the meantime give it a try yourself. Basically uninstall buildozer:

pip uninstall buildozer

Install it for python2:

python2 -m pip install --user buildozer

And try again 🤞

[ghost]

Okay, worked with Python 2 as build host. But maybe it'd be worth to do at least the following change to the recipe in master:

                if os.path.isfile("setup.py"):
                    if sys.version_info[0] < 3:
                        with open('setup.py', 'rb') as f:
                            text = f.read().replace('_FT_LIB_', ft_lib_dir).replace('_FT_INC_', ft_inc_dir)
                    else:
                        with open('setup.py', 'rb') as f:
                            text = f.read().replace(
                                    b'_FT_LIB_',
                                    ft_lib_dir.encode("utf-8", "replace")
                                ).replace(
                                    b'_FT_INC_',
                                    ft_inc_dir.encode("utf-8", "replace")
                                )
                    with open('setup.py', 'wb') as f:
                        f.write(text)

... since that is obviously needed for a Python 3 build host while not affecting Python 2, and it'd allow us to investigate potential other issues with the Python 3 build host!

Other than that, this merge request is probably unnecessary now 👍 so that's nice, working reportlab 🎉

Edit: I'm mostly interested in getting the Python 3 host to work because Python 2 is ancient old fart stuff that is worth not depending upon even for build, IMHO. But obviously, this isn't super time-critical

[AndreMiras]

Good news! Yes I agree this should be tackled.
Feel free to either create a dedicated pull request and close that one. Or update this one otherwise I'll definitely create one myself.
But if you update, please make it clear in both commit message and body what's it's doing e.g. fix for python 3 host. And also rebase/squash to avoid back and forth commits.
Thanks a lot for your contributions!

[ghost]

I'll make a separate one, just in case the env change part turns out to be still interesting to look at for something. (Even though I assume at this point it is completely unnecessary, but who knows)

[AndreMiras]

Yes makes total sense. I'm closing that one nonetheless and wait for the other one.
I'm currently on holidays with very crappy connection so not sure I'll be able to review on test very soon 😢
Can't wait to see it

[AndreMiras]

Hi @Jonast I was a bit busy working on #1380 but I could eventually reproduce the TypeError on host Python 3. The fix looks fairly easy and is described here:
2d2b097#r30666346
I'll clean up the unit test and make a PR. Could you give it a try when it's ready?

Edit:
Here it is #1383 @Jonast can you give it a try?