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

Implement --blacklist option and include more modules/recipes by default #1683

Merged
merged 1 commit into from Feb 6, 2019
Merged

Implement --blacklist option and include more modules/recipes by default #1683

merged 1 commit into from Feb 6, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 6, 2019

This is another subpart split out of my misguidedly oversized #1625 (setup.py) pull request.

Changes:

  • Adds --blacklist option that prevents recipes/packages from being added even though 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 a little

@ghost
Copy link
Author

ghost commented Feb 6, 2019

@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 --blacklist to reduce it. I added a doc section for it though, so it shouldn't be too hard to figure out for anyone who wants to do that

tests/test_graph.py Outdated Show resolved Hide resolved
AndreMiras
AndreMiras previously approved these changes Feb 6, 2019
Copy link
Member

@AndreMiras AndreMiras left a 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

@ghost ghost changed the title [WIP] Implement --blacklist option and include more modules/recipes by default Implement --blacklist option and include more modules/recipes by default Feb 6, 2019
* 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
@ghost
Copy link
Author

ghost commented Feb 6, 2019

@AndreMiras commit details added, also I just tested this with a real world app to verify that --blacklist=openssl indeed makes it unavailable and it does. As far as I'm concerned this is ready for merge 👍

@AndreMiras AndreMiras merged commit 8122bde into kivy:master Feb 6, 2019
@inclement
Copy link
Member

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?

@AndreMiras
Copy link
Member

Ouch good point @inclement
Too bad, that one right? https://github.com/kivy/python-for-android/blob/6660b9e/pythonforandroid/bootstraps/common/build/build.py#L634
Not nice 😢
Also I didn't know you discussed that previously, I should have aligned with you to double check.
Now what I guess first thing is to verify if indeed the other --blacklist is being masked by this one. In this case it would require using another option name that doesn't clash with that one.
Or do we want to completely revert that change?

@inclement
Copy link
Member

Now what I guess first thing is to verify if indeed the other --blacklist is being masked by this one

I didn't directly check, but I don't see how it could not be.

Let's at least rename it to something like --blacklist-requirements. I might personally prefer to avoid the word blacklist, partly because I don't like how broad the documented procedure is when actually I can't see what it would ever be used for except this one specific thing. Maybe I'm missing some other use cases though.

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?

@AndreMiras
Copy link
Member

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... 😕
We can also drop the blacklist part of this PR and if dev want to reduce size of their apk even further they use the usual --blacklist param to blacklist via path. So that would simplify the code/doc while keeping the sqlite3 feature included by default

@inclement
Copy link
Member

True story, the argument I see is just to stick to most of the default python distributions we know

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.

We can also drop the blacklist part of this PR and if dev want to reduce size of their apk even further they use the usual --blacklist param to blacklist via path

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.

@AndreMiras
Copy link
Member

I like the MinimalPython3 idea and I don't at the same time.
I like because it seems a more elegant solution to that problem. But I don't because even though it feels pretty simple it also add some "complexity" over the prio-pr1683-state. I mean I can already foresee that recipe would either be a lot of copy past, hence we would have two recipe to maintain python3 (e.g. configure flag update, version bump). Or we would go with fancy inheritance/composition to honor DRY and then it still adds complexity.

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 openssl and sqlite3 support would just go as before, by simply providing it. It's unexpected Python behaviour but not that cumbersome after all

@inclement
Copy link
Member

But I don't because even though it feels pretty simple it also add some "complexity" over the prio-pr1683-state.

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.

I mean I can already foresee that recipe would either be a lot of copy past, hence we would have two recipe to maintain python3 (e.g. configure flag update, version bump). Or we would go with fancy inheritance/composition to honor DRY and then it still adds complexity.

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 --minimal-python argument of some sort, but I think the recipe would be fine.

@AndreMiras
Copy link
Member

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.

Yes agree that's why I was suggesting if at the end it annoys more than it helps we can revert it, period.

@ghost
Copy link
Author

ghost commented Feb 7, 2019

@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 setup.py although maybe that will never be possible since pip appears to have no easy way to completely blacklist things... so maybe ignore that. Edit: also it could be useful to people who want to disable a dependent recipe in some other case where they're sure it works without it - but does that happen often? Probably not, so again, maybe ignore that 😂

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

@ghost
Copy link
Author

ghost commented Feb 7, 2019

(--> 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)

@ghost
Copy link
Author

ghost commented Feb 7, 2019

I made #1689 in case you decide to keep the functionality but with a proper parameter name

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