[WIP] Run project's setup.py if present, unless --ignore-setup-py was set

[ghost]

Run project's setup.py if present, unless --ignore-setup-py was set.

What exactly does this do:

Changes to p4a's behavior:

1. If a setup.py is present in the --private code directory and --use-setup-py is set,
   all dependencies that map to recipes will be implicitly added to --requirements
2. After all modules in --requirements have been built, the setup.py is run (again only if
   --use-setup-py was specified)

This has the following consequences:

1. You can now use a setup.py and --use-setup-py, and either omit --requirements
   (If you use Python 3) or otherwise just specify `--requirements=python2
2. For projects without a setup.py file, nothing changes
3. For projects that for some reason have a setup.py but don't want it to run for p4a, there will be a big fat warning printed that in the future, --ignore-setup-py must be specified (right now the default is still not to run the setup.py unless an explicit --use-setup-py was given)

Why this pull request is more than just a neat feature addition:

This functionality solves fundamental issues in python-for-android::

1. Currently, projects that need to be installed to work are a huge pain to get working. (They need to be copied to another place and then that folder needs to be added as an additional --requirements entry) This will no longer be needed

2. --requirements is kind of annoying and violating DRY for projects that already use setup.py for desktop

3. Currently, Cython projects that do cross-package .pxd imports are impossible to install without notable modifications to the packages themselves (e.g. manually copying the .pxd files around from one package into the other and set them up to be importable, which may not be much easier than fully merging the packages into one). This is because p4a assumes the install order of all the non-recipe dependencies doesn't matter, which is wrong for this case.

   With this pull request, this won't be fully fixed: however, if the affected Cython libraries are regular enough or p4a-aware enough that they work without a special CythonRecipe (since basic Cython now works in p4a master without any recipe), then the project's setup.py can install them and proper install order will be preserved - so for these libraries, it will be fixed.

[ghost]

Edit: wrote here I was done, but now I added things so I'm not done yet

[AndreMiras]

This method starts to be pretty big, do you think that would make sense to split it a bit more by concern or that's too painful?

[ghost]

I vaguely recall that John Carmack wrote a nice article why splitting up big functions without actually making use of the inter-dependencies can actually be harmful. (because you get hidden dependencies between the split up functions increasing the risk of later messing things up when changing stuff, and the code flow is less obvious for no benefit)

I agree with this sentiment, and don't think there is any use in splitting it up just because it's long. However, if you think one part in there might be also needed to be called from somewhere else then that sounds like a good idea to me

[AndreMiras]

Well sometimes when the split by concern makes obvious sense. And sometimes it also simplifies unit testing it.
But it was just a suggestion, I didn't dig into it myself and I will not for now

[ghost]

makes sense. for what it's worth, this function is the core install-the-whole-bunch place so I don't think it is very useful to be unit tested. (it's mainly relevant for the integration test where the non-split up code doesn't make a difference) so good point @ unit tests, but IMHO this should be ok

[AndreMiras]

Impressive work as usual 👏
Few remarks before I actually test it.

1. I would have intuitively turned the feature off by default and make it possible to enable with a flag. That way we're sure we don't introduce breaking changes
2. This may be working now, but of course it introduces more complexity, which means more things to think of when introducing new changes, more checks to do, more lines to maintain. Plus this is not unit tested nor integration tested. This is probably not easy to automate it anyway. But that's a friendly reminder to keep in mind every time we introduce code to the core. This code will have to be maintained
3. Did you see the python2 CI is now failing, do you know why? https://travis-ci.org/kivy/python-for-android/builds/486715046

[ghost]

@AndreMiras I was bored and it has everything now:

• All recipe dependencies are picked up from setup.py including indirect ones
• The setup.py is run later and installs all non-recipe things
• --requirements is pretty much never needed unless you want to fall back to Python 2
• There is --recipe-blacklist and the python3 recipe defaults to including everything needed for all standard modules (sqlite3, openssl, libffi, others to be added if I forgot some) and to trim it down from the non-default include-all, one can use the blacklist (this is somewhat necessary because 1. it's a way easier to get started with default, 2. in a setup.py there is no way to specify such core deps because that's not needed on desktop, so it makes more sense to default to something that works with setup.py properly)

I haven't checked pep8 and need to sleep on this and have another look, but I think I'm pretty close to this being done 👍

This will fix #1576 #1490 (at least if you use setup.py) and #1488

[ghost]

I had an issue with graph.py (and other recipe code) being case sensitive, while pip is not. So I had to fix that as well to get everything working which blew up the pull request a little, sorry about that 😨

While doing that I also noticed the bootstrap base recipe dependencies aren't actually added in, that all deriving bootstraps override the base bootstrap's dependencies instead of adding to them, and that the bootstraps aren't all actually depending on shipping python which seemed a bit nonsensical to me. So I these things as well 😬

[AndreMiras]

Thanks @Jonast for that PR!
You know how I like huge PR, don't you? 😛
Specially since this one may have controversial changes. Would you mind to split it by concerns?

[AndreMiras]

Yes that's the idea, for instance the case insensitive change for instance already make sense on its own right?
Also the change for #1576 could be standalone on another PR correct?
I'm really bad a reviewing/merging large PR. But I know it's not that of a problem for other core devs

[ghost]

Right, not a bad suggestion 👍 I can back out all the graph changes as one: that would include #1576 since it is directly tied to the blacklist, lowercase graph mapping, Recipe-found-by-lowercase changes, and the default bootstrap dependencies not being set/added in properly.

I would rather not go more granular though and make multiple graph pull requests because it's not very fun to debug, so I don't want to debug it multiple times for intermediate state nobody really needs

[AndreMiras]

Thank you so much @Jonast ! 🍺

[AndreMiras]

some thoughts: this is why we use context managers. It's surprising Python doesn't provide one built-in for the tempfile use case. I may build that package one day...

[ghost]

if you know a context manager for this I'd happily add it here!

[inclement]

pythonforandroid.util.temp_directory provides this functionality as a context manager, but in a very shallow way.

[AndreMiras]

Thanks for writing some docstrings 😄
But this looks so complex 😨 I hope it's properly covered by tests😬 You know I'll probably check the corner cases and look to simply things 😅

[ghost]

The _get_system_python_executable one? Yeah I added multiple test cases that actually create a virtualenv, see tests/test_pythonpackage.py

[AndreMiras]

Thanks! I'll play with it more in depth if I ever get time

[inclement]

Github ate my original comment, so here's some main points :p

Overall, great stuff, I have no issue with the major structure of things and @AndreMiras has picked up a lot of individual issues. I'm not that worried about any other minor issues that might remain.

My main concern is understanding how this fits into how we should build with p4a. I didn't actually try it yet (will soon), but I'm not clear on how the setup.py inclusion works - a normal project structure would have a setup.py alongside a folder containing the rest of the project, but if you specify the root folder with --private then won't the setup.py install the folder and p4a will copy the folder into your app? Sorry if I missed something here, I'll look again another day but didn't want to hold up posting comments.

With that in mind, would you be able to add a trivial testapp with the folder structure you have in mind and a setup.py that just does some trivial package install?

I'm also wondering what is the ideal user build experience. Running the user's setup.py is a perfect solution to points 1 and 3 that you raise (installing user's module is convenient, and it's a nice way for user cython to work). I still think that point 2 (--requirements is annoying) is relatively trivial, and if this DRY is a problem then you've already reached python packaging nirvana :p. However, it's not like it's offensive that it works.

More broadly (and I haven't thought about this part before), I'm not sure how this should interact with the existing setup.py method - in the spirit of using standard python tools, it seems natural to want to run python setup.py apk to invoke p4a, and this is what I normally do for my projects. This would be easily compatible with this new feature if you use two different setup.py files, but I guess it should also work fine if you use the same setup.py file for both purposes? That would be nice and neat.

That would also make the parsing of install_requires more obvious from my perspective, I think the current python setup.py apk support just ignores them because I never solved the dependencies problem you've addressed here.

To be clear, I'm not anticipating this blocking your PR, I'd just like to have a clear vision about it.

[inclement]

pythonforandroid.util.temp_directory provides this functionality as a context manager, but in a very shallow way.

[inclement]

This is the same thing as the --blacklist-requirements that did get merged, right? I guess it needs removing now?

[inclement]

Will it?

Edit: Yes is probably an acceptable answer, see main comment

[ghost]

Haha, I guess I did suggest that it will change in the future 😂 of course if most of you think that's a bad idea I'll just take it out. But my suggestion would be, just for the sake of having even easier defaults, that eventually this is changed such that --use-setup-py is the default

[inclement]

Yep, virtualenv should probably be here. It's only missing because I wasn't thinking of it as a pypi dependency.

[ghost]

Edit: forgot to say, thanks for looking at it! 😍

but if you specify the root folder with --private then won't the setup.py install the folder and p4a will copy the folder into your app?

Hmmm, good point. Yes, it ends up both in the root, and in the site-packages, and in the end it still runs main.py as before. Maybe I should change it to only copy the main.py in, and none of the other files anymore? Do you have any suggestions? For my app that would work since my main.py just imports the package from site packages and runs it but I don't have a large amount of data files hence I never thought about this duplication much. I'm not 100% sure what the usual user expectation would be

With that in mind, would you be able to add a trivial testapp with the folder structure you have in mind and a setup.py that just does some trivial package install?

Just add a setup.py to your --private folder root (where your main.py is). Then launch p4a apk --use-setup-py ... and it will automatically scan the deps from it, and call it during the python package install phase such that your app gets installed into the site-packages inside the python bundle.

but I guess it should also work fine if you use the same setup.py file for both purposes? That would be nice and neat.

Yes that should work fine, because the handling I am adding really doesn't care that your setup.py also has the apk method (or whether it was used to indirectly launch it). All it does is simply invoke setup.py install or the pep517 mechanisms to list the dependencies (edit: and simply adds them to --requirements directly after the parser collects the user- or however-specified ones), it shouldn't care any more about that additional apk action than normally running setup.py install does

). I still think that point 2 (--requirements is annoying) is relatively trivial, and if this DRY is a problem then you've already reached python packaging nirvana :p

I disagree @ trivial! For small projects maybe, but just think about version pins or really long indirect deps lists! And now imagine you'll keep them in sync, always. Since that always works out so well. (It probably won't...!)

But I consider the way more compelling argument that it's much easier to start with p4a with "use your setup.py as before", instead of "read about --requirements, realize you need all deps INCLUDING indirect ones in there, now go find out what your indirect deps even are, then realize you need to specify some differently so the according recipe is found". This process is much more complicated for newbies than us veterans may realize, and you don't need to do any of it with this pull request Edit: to explain, it will always resolve the pip reference to a package name. So it will understand it maps to a recipe even if you specify it via git URL or some other unusual way

[ghost]

Marked as WIP because I think @inclement 's double-copy remark should be given some thought. The best approach I can think of is to copy only main.py when a setup.py is used, and make it very clear that the setup.py install needs to fully install all needed data files as well.

But I'd like to hear your ideas for this. Maybe it should be an option? Although I am inclined to push people to write proper setup.py files if they want to use this which would IMHO for sure mean to copy everything needed into site-packages without exceptions

[ghost]

I was impatient as always and implemented it like that 😂 😂 😂 might still be broken lol, but in theory it works 😬

Edit: also works fine in my test ☺️ now only main.py/main.pyo is copied from --private when a setup.py is used!

[inclement]

Just add a setup.py to your --private folder root (where your main.py is). Then launch p4a apk --use-setup-py ... and it will automatically scan the deps from it, and call it during the python package install phase such that your app gets installed into the site-packages inside the python bundle.

I get this, but I'd like to have a testapp as much for documentation purposes as anything else. It's valuable to be able to say not just 'look at the docs' but 'look at this example' when a user asks how to include their cython code.

I disagree @ trivial! For small projects maybe, but just think about version pins or really long indirect deps lists! And now imagine you'll keep them in sync, always.

I appreciate the point, but I still think that it's a relatively niche problem to actually have, and that in any case it's hardly unresolvable - you don't need to keep multiple lists in sync, just read them from the same place.

But I consider the way more compelling argument that it's much easier to start with p4a with "use your setup.py as before

I strongly agree with this point, but this really isn't what I'd mean by "use your setup.py as before" for most purposes. From my point of view python setup.py apk does is much more in that vein, since it integrates p4a as a natural syntax alongside other distribution options, and makes an attempt to use the setup.py config (app name etc.) to set all the p4a options.

I don't mean that this is in conflict with your change, clearly the ability to install the user's python module in the target environment and to compile cython is quite reasonable, I'd just like both to work in a cohesive way. I'm still thinking through what this needs to imply.

instead of "read about --requirements, realize you need all deps INCLUDING indirect ones in there, now go find out what your indirect deps even are, then realize you need to specify some differently so the according recipe is found"

This is multiple issues:

• read about --requirements
• realise you need all deps
• realise you need to specify some differently so the according recipe is found

Of these, the second two are mostly not being resolved by anything to do with the user's setup.py, but by your neat code to resolve the dependencies for them. The first one is the one I consider relatively trivial, I think that anyone using a packaging tool should be prepared to expect an argument explicitly specifying what to package.

To be clear though, I'm not saying you should remove this feature, there's nothing fundamentally wrong with it and I don't believe that it causes any problems. I'm just more personally interested in maybe integrating this functionality into the normal requirements resolution, or especially to the python setup.py apk syntax for which using the install_requires is clearly expected (and not currently done!).

But I'd like to hear your ideas for this. Maybe it should be an option? Although I am inclined to push people to write proper setup.py files if they want to use this

I don't think I have a clear vision about this, I'm still thinking about it. I'll try to post some thoughts tomorrow, or on discord if riot ever comes back up!

[ghost]

But I consider the way more compelling argument that it's much easier to start with p4a with "use your setup.py as before

I strongly agree with this point, but this really isn't what I'd mean by "use your setup.py as before" for most purposes.

I think that view is very flawed / misses the main point of using a setup.py. The options and avoided duplication and easy calling are only one side of the coin, which setup.py apk covers, fair enough. But the much more important one is actually running the install routine. I think you are under-estimating how much easier that is for porting larger projects, e.g. you just can't run my app without installing it into the site-packages and working around that would be time-consuming and complex (since the install routine exists for a reason)

As for your other suggestions regarding name resolution and apk integration:

I'm a bit torn whether the name-to-recipe resolution should be automatically used without a setup.py as well, simply because it's quite slow. Otherwise I would agree that it's a useful thing to do. As for making setup.py apk also look at install_requires(), that should be possible but it sounds more like a distinct problem (I wouldn't expect the necessary code changes to be in any of the parts I changed)

[inclement]

But the much more important one is actually running the install routine. I think you are under-estimating how much easier that is for porting larger projects, e.g. you just can't run my app without installing it and working around that would be quite complicated (since the install routine exists for a reason)

Maybe I wasn't clear, this is the part I think is a great feature and am 100% behind.

[ghost]

Ah, very well. I agree all the other points are not that major. And I think it makes sense that setup.py apk would learn to scan for install_requires() as well, since then these two features could be combined. So that sounds like a reasonable idea. I'm just not sure if it makes sense to also put it into this pull request or rather a followup one

[inclement]

No need to put it in this PR, I know it isn't part of the core change and there's no need to have it immediately!

[ghost]

Alrighty then let me put together a quick test app. What about maybe converting one of the ones that are already run anyway for travis? Then we wouldn't need an additional travis test run but this would still be tested

[inclement]

Yep, using one of the existing testapps sounds great, I'm really just thinking of something where someone can be told 'copy this basic file/folder structure to start'. Thanks.

[ghost]

Hmmm the only test app currently run right now for python 3 is testapp_python3_sqlite_openssl_googlendk, but that one would be a little less minimal, right? I could convert that one anyway and add another additional minimal example into the docs (with actual setup.py contents) or something, or what would you suggest?

[inclement]

I guess what I'd really like would be an example that builds a trivial cython file, e.g. a cython function that calls libc.math.sqrt and a main.py that just runs that function. The main.py wouldn't need to have a gui or anything, just nice to have a demonstration of how you would package an app with a cython file. I realise this isn't hard to work out from the point of view of knowing how these things work, but I think it's quite valuable for users who often are not that familiar with how cython or python packaging really fits together.

[ghost]

@inclement ok I am just encountering a problem: to run the setup.py (with this pull request) it needs to be 1. named setup.py, 2. located in the root of --private such that the package can still be installed (that also means as a consequence main.py would usually be an outside file, not inside a submodule folder). Both is not the case for every single test app in testapps right now

I also find this pretty counter-intuitive in general, apart from this being a problem for testing this pull request. Should I refactor them all to be properly contained in a folder with a regular setup.py? Or what is the reason they're separated out like that? I've never seen it done like that before

[inclement]

I think the simplest way to fit with the current folder structure would be to make a new folder testapp_with_setup (better name welcome!) that contains the setup.py and main.py and any other files/folders you want, then a script build_testapp_with_setup.sh at the root level containing the p4a command to build that folder.

The current structure is organised by app code (in the folders) and setup.py files intended for python setup_some_details.py that build specific folders with specific p4a options. The reason for separating things this way is that multiple setup.py files often share the same app code. I've never perceived any issues with this structure, but I'm not clear what change you're proposing exactly.

[ghost]

but I'm not clear what change you're proposing exactly.

I proposed making them all separate subfolders with their respective setup.py file named "setup.py"!

Oh they share the app code? Hm, what about using symlinks then? The thing is it's IMHO kind of an unusual organization (not necessarily bad, just not very common) and now if I make some with subfolder with setup.py inside it's going to be an even weirder mix, right? Some folders being submodules to the setup-py-crowd in the folder above, some bringing their own setup.py inside, that doesn't sound too great to me

[inclement]

I've thought about that before, but I ended up thinking that the end result would be just the same but with unnecessary nesting: instead of multiple files setup_{X,Y,Z}.py you'd have multiple folders build_app_{X,Y,Z} each containing a single file setup.py. I didn't think that was really any better.

How about making another issue for this folder structure if you think it's worth looking at? I'm happy to discuss it, but probably not worth blocking this big PR on it.

[ghost]

Sure I agree it wasn't any better up to current master, so I see where you're coming from. But with this PR this structure will be required for at least one test app, so I'd rather have them all like that personally even if that means more nesting

But okay I can leave it varying/inconsistent for now, it just bothered me a little since it's ugly to me 😬 (the inconsistency)

[inclement]

Sorry for yet another delay with this, I've been busier than expected.

Right now I'm inclined to just merge it, there's not much value in my business holding it up, since it's a good addition. Is there anything you wanted to change before that? It doesn't look like it adds a testapp, but I can do that if you want.

[ghost]

@inclement sorry I was also just sidetracked with CSS in my toolkit and forgot to add a test app 😂 it's complete and I use this daily, so apart from the missing test app (which is why I left WIP in the title) I don't see anything else that's missing

[inclement]

@Jonast The thing I wanted to do but didn't get around to was to try to think through fully how the python-for-android api should work - I kind of feel that both the existing setup.py stuff, and the way your additions work, are imperfect interfaces. I was thinking of trying to write some ideal documentation for how I'd like python-for-android to work for people, as a way to try to see what we could change.

I think this might lead to changing the recommended api, maybe even a breaking change, but I guess it's better to just roll with it and have this code in than to think too hard about changes I don't have time for right now anyway!

So, thanks for the PR :)

[nitanmarcel]

@Jonast Could you take a look at my issue #1808 ? I think is linked with your pull requests. Thanks

[ghost]

@nitanmarcel indeed that was me 😬 fix is here: #1809 someone should get around to review and merge it in the next hours, then things should hopefully work again with buildozer