-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Implement --blacklist option and include more modules/recipes by default #1683
Conversation
@inclement this changes by the way what we talked about, that it will now include all core modules per default and the people who want a more trimmed down build need to actively specify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good to me.
Do you have an idea of how much is the minimal APK size after adding sqlite3
, openssl
and libffi
compared to without?
Edit:
Also commit message pro-tip: put all the useful details you have in your PR description in the commit message body. People reviewing the log or blaming later will have the useful info without having to look for the associated PR.
For instance take a look at the Linux kernel commit log for an example of comprehensive commit message
* Adds `--blacklist` option that prevents recipes/packages from being added even when they are in the `depends` of recipes or otherwise added * Makes the following modules included per default: `android` (via `bootstrap.py` dependency of all bootstraps), `openssl`/`libffi`/`sqlite3` (via `python3` recipe dependency) * Documents `--blacklist` option and that `android` is now included by default * Cleans up the packaging kivy/sdl2 apps part in the Quickstart section
@AndreMiras commit details added, also I just tested this with a real world app to verify that |
As discussed before, I still don't really see the value of doing this, but I'll defer to the clear approval of everyone else. However, doesn't this break the existing --blacklist argument? |
Ouch good point @inclement |
I didn't directly check, but I don't see how it could not be. Let's at least rename it to something like Is there a strong reason to implement an argument for this rather than the other suggestion of having a MinimalPython3 recipe that omits the opt_depends from the hard requirements? |
True story, the argument I see is just to stick to most of the default python distributions we know. But yes then I agree it then requires to add the "blacklist" case hence more code, more noise... 😕 |
This is fine, but a generic blacklist isn't the only way to deal with it. The suggestion mentioned above (also discussed on discord I think?) was to have a MinimalPython3 recipe that's identical to the Python3 one except that it places all the not-really-dependencies in opt_depends rather than depends. The result would be that anyone taking the extra step to optimise the apk replaces python3 with minimalpython3 (or other name as appropriate), then adds any optional dependencies explicitly.
I don't think this is good enough, it's too indirect a method for something that a significant fraction of users will ultimately want to do. |
I like the MinimalPython3 idea and I don't at the same time. I do agree that if we care more about minimal apk size than we care about "the ease" to use p4a, then maybe p4a is better without #1683. Then dev who need |
So does the whole new blacklisting system just introduced - that's basically my objection here, I think it's far more complex than a minimal python recipe but for not much benefit since it doesn't have any other use that I can see.
One of the recipes would be a trivial subclass of the other, the only difference is the depends/opt_depends. I guess another alternative for the same effect would be a |
Yes agree that's why I was suggesting if at the end it annoys more than it helps we can revert it, period. |
@AndreMiras @inclement oops @ option, feel free to revert I can reintroduce it with a different name as a new pull request. Sorry about that, feel kind of silly for not noticing that 😢 @inclement I personally think it is nicer to disable what you don't want rather than either have everything, or minimal/nothing with everything added in, it just seems a little strange to me. Also, I had the hope of being able to one day use this argument to block dependencies pulled in by a I'm playing a bit devil's advocate here. It seemed like the more universally useful approach to me but just by a small margin that maybe isn't worth the complexity |
(--> if you both prefer a minimal recipe then maybe that's the way to go. That's just the reasons why I preferred a substractive rather than an additive approach) |
I made #1689 in case you decide to keep the functionality but with a proper parameter name |
This is another subpart split out of my misguidedly oversized #1625 (
setup.py
) pull request.Changes:
--blacklist
option that prevents recipes/packages from being added even though they are in thedepends
of recipes or otherwise addedandroid
(viabootstrap.py
dependency of all bootstraps),openssl
/libffi
/sqlite3
(viapython3
recipe dependency)--blacklist
option and thatandroid
is now included by default