[WIP] Enhance distribution check when reusing distributions and... #1926

opacam · 2019-07-20T10:25:57Z

...Skip some unneeded rebuilds

The main reason to do this, it's because we usually have to deal with issues related with old cached builds as well as the reuse of a distributions it seems to not work as expected, so this PR is intended to deal with this.

Things made in here:

Save into dist_info.json file more information:
- android api
- archs
- p4a version
- recipes version
Perform more checks when we find a dist with the same name:
- check if android api is the same as the one we request
- check if distribution's arch is the same that we are requesting
- check if p4a version we are using is the same as the one we used for the built distribution (and if not, force a cleanup of the build directory)
- check recipes version and remove old build in the case that we detect an outdated recipe version
Fix Recipes.clean_build, so we can remove old builds
Implement BootstrapNDKRecipe.should_build , so we can remove old bootstrap builds (Here I make use of ndk-build clean which it seems to remove all bootstrap builds and libraries regarding that which recipe we are targeting...:thinking:...I suppose it's fine for now (it simplifies a little the thing)
~~Implement GuestPythonRecipe.should_build so we can skip python rebuild~~ (removed commit since it's already done at Fix build for case-insensitive FS and add CI test for OSX #1951)
change bootstrap build directory (to ensure that when we change arch and we already have build with another arch, we don't get conflicts)
detect when a PythonRecipe needs a rebuild, so we remove our python-installs directory (yeah, it's a little drastic this point but it would do the job and simplifies the code)
remove recipe's generated libraries from libs_collection when we detected a changed library recipe

Things to consider...I may forgot something in here:

The build/dist directories will grow up considerably
we never remove recipe's generates libraries from libs_collection when we detected a changed recipe (we only remove the build directory...so this may cause some troubles in case that Recipe.should_build checks for the library into that folder)...mmm
buildozer need to be updated to work with this (since the dist name it will be modified with a suffix)

Closes: #1920 and #1917

I leave this WIP because:

we may want to discuss the whole idea...we could simply force a full clean in each build (sure that it would be simpler)
There are some parts of the code that may be improved or even moved from one place to another...I have to review it...but if anyone sees something, I'll be glad to discuss/fix it
I tested different situations and this thing seems to work, but I may forgot something...so better other eyes take look on this
we may want to split this thing into multiple PRs...sometimes things make sense in one PR, but when PRs go big it complicates things for all of us...should we have a workflow to deal with this? Maybe the stacked pull requests method could be a solution?
- As a side note: I think this method could only be used for those ones with write permissions to repo, since needs to create a new upstream branch plus it requires to merge things in reverse order)

inclement · 2019-07-20T10:44:38Z

Thanks @opacam, I have no immediate objection but I won't have time to look at this properly until at least next week.

ghost · 2019-07-21T00:32:51Z

I actually support the idea of forcing clean builds, and adding mechanisms to speed up build in general instead. E.g. separate distribution-independent download caching, promoting the use of ccache more which works fine, and other such stuff.

The build just seems too complex to be worth caching if you throw in external packages that may have new releases, change in deps, and other things of the (outside) world changing.

So my vote would be to drop all the ways to recognize a distribution as same or different, and just make a fresh one for each build unconditionally (and throw it away right after each build unless some --no-cleanup-distribution is used to keep it around for debugging and poking. This is also similar to how pip works with its temporary build dir)

Edit/Additional point: people also often get tripped over when they change --requirements and old external libs are still in the distribution. But now if you run with --use-setup-py they aren't even specified and then this would again need special handling code to notice some deps are suddenly gone. It just all seems to go down an infinite rabbit hole of complexity

pythonforandroid/distribution.py

AndreMiras · 2019-07-29T18:16:48Z

pythonforandroid/distribution.py

                continue
-            for recipe in recipes:
-                if recipe not in dist.recipes:
+            checked_dist_recipes = set()


same here for string literals {} which by the way is slightly faster in Python (because set would require alias resolution), but who cares 😄

Nevermind ref #1926 (comment)
Thanks @opacam

So we can later check if the dist was created with an different `android api`

Because we already set it in `Context`

So in case that we detect a mismatched value, we will not use that dist. Note: also removed unneeded duplicated check

So we can later check if the dist was created with an old version of p4a

And in case that we detect a mismatch version we will force to delete all the previous build folders to start from scratch Note: In case that we want to export a created dist we will omit the removal, unless we need to rebuild the dist.

This way, we can later check if any recipe has been updated, so we can rebuild it if necessary

And in case that we detect a mismatch version we will force to delete the old build so it can be rebuild

So we avoid to overwrite them when arch changes (which is probably what we want)

Now that we reuse the dists as expected, we should be clear about when we reuse a dist or when we build a new one...otherwise we will get confusing messages in our build logs

To avoid conflicts when we reuse a dist with a different arch. The same name convention used for standard recipe builds has been used, but contemplating all the ctx.archs (because ndk builds are supposed to support multi arch builds). So an bootstrap build folder would look like as: - armeabi-v7a_arm64-v8a__ndk_target_21 - arm64-v8a__ndk_target_21 ...

when we detect an updated python recipe, so we make sure that the new one is built and installed. Note: this will remove all the python's site packages...maybe is not the best solution but it will do the job for now...

In case that we detect that the recipe it's a library recipe

So...if we make a new distribution, we will be able to reuse the already built libraries

inclement · 2019-09-15T21:38:40Z

Just noting here since I mentioned it on #1716 - I think I'd like to propose some simplifications to this PR before we merge it. In the meantime I'm working on a simpler change to support multiple arch builds for the same dist name, just to make sure this annoyance is resolved in the next release (which I'll make as soon as this change is added).

opacam · 2019-11-30T15:12:57Z

I close this for several reasons:

there are merge conflicts which are hard to solve
when reviewing the code I find some parts that I don't like
hard to review this PR
how I deal with android_api is not right

But, I will create a new set of PRs to deal with the problems we have when reusing distributions, which...I hope...it will be easier/faster to review 😉

¡¡Thanks all for your feedback in here!! 😄

opacam mentioned this pull request Jul 21, 2019

Fix debug build not resulting in gdb-debuggable build #1867

Merged

opacam added the need-discussion The issue or pull request needs a discussion to take a team decission label Jul 21, 2019

AndreMiras reviewed Jul 29, 2019

View reviewed changes

pythonforandroid/distribution.py Show resolved Hide resolved

AndreMiras reviewed Jul 29, 2019

View reviewed changes

opacam added 11 commits September 1, 2019 22:17

[dist] Save android api into dist_info.json

703dcc0

So we can later check if the dist was created with an different `android api`

[dist] Remove the kwarg ndk_api from get_distribution

46a05ca

Because we already set it in `Context`

[dist] Check android_api and archs when reusing dist

1e7d0d4

So in case that we detect a mismatched value, we will not use that dist. Note: also removed unneeded duplicated check

[dist] Save p4a version into dist_info.json

8276ec5

So we can later check if the dist was created with an old version of p4a

[dist] Check p4a version when reusing dist

bf69bb1

And in case that we detect a mismatch version we will force to delete all the previous build folders to start from scratch Note: In case that we want to export a created dist we will omit the removal, unless we need to rebuild the dist.

[dist] Save recipes version into dist_info.json

a05cc6b

This way, we can later check if any recipe has been updated, so we can rebuild it if necessary

[dist] Check recipes version when reusing dist

975d7bb

And in case that we detect a mismatch version we will force to delete the old build so it can be rebuild

[dist] Update test_distribution to latest Distribution changes

759475c

[dist] Fix Recipe.clean_build for Recipe

7a0bf03

[dist] Fix Recipe.clean_build for BootstrapNDKRecipe

2b5960e

[dist] Implement BootstrapNDKRecipe.should_build

b7a2520

opacam force-pushed the feature-dist-checks branch from 1947491 to 5a2eba4 Compare September 1, 2019 20:24

opacam added 7 commits September 4, 2019 00:06

[dist] Add an arch suffix to our dists

096e3e5

So we avoid to overwrite them when arch changes (which is probably what we want)

[dist] Add message about reusing dist

53e39dd

Now that we reuse the dists as expected, we should be clear about when we reuse a dist or when we build a new one...otherwise we will get confusing messages in our build logs

[dist] Remove python-install when reusing dist and...

479e71e

when we detect an updated python recipe, so we make sure that the new one is built and installed. Note: this will remove all the python's site packages...maybe is not the best solution but it will do the job for now...

[dist] Remove ctx's library on clean_build

6a17464

In case that we detect that the recipe it's a library recipe

[dist] Fix ctx not being set for Recipe.get_recipe

9ee9f29

[dist] Force execution of recipe.install_libraries

e563dfe

So...if we make a new distribution, we will be able to reuse the already built libraries

opacam force-pushed the feature-dist-checks branch from 5a2eba4 to e563dfe Compare September 3, 2019 22:21

opacam mentioned this pull request Sep 15, 2019

Consider arch when looking for compatible dist. #1716

Closed

opacam closed this Nov 30, 2019

opacam mentioned this pull request Nov 30, 2019

Enhance distribution reuse: android_api #2031

Open

opacam deleted the feature-dist-checks branch December 2, 2019 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Enhance distribution check when reusing distributions and... #1926

[WIP] Enhance distribution check when reusing distributions and... #1926

opacam commented Jul 20, 2019 •

edited

Loading

inclement commented Jul 20, 2019

ghost commented Jul 21, 2019 •

edited by ghost

Loading

AndreMiras Jul 29, 2019

AndreMiras Jul 29, 2019

inclement commented Sep 15, 2019

opacam commented Nov 30, 2019

[WIP] Enhance distribution check when reusing distributions and... #1926

[WIP] Enhance distribution check when reusing distributions and... #1926

Conversation

opacam commented Jul 20, 2019 • edited Loading

inclement commented Jul 20, 2019

ghost commented Jul 21, 2019 • edited by ghost Loading

AndreMiras Jul 29, 2019

Choose a reason for hiding this comment

AndreMiras Jul 29, 2019

Choose a reason for hiding this comment

inclement commented Sep 15, 2019

opacam commented Nov 30, 2019

opacam commented Jul 20, 2019 •

edited

Loading

ghost commented Jul 21, 2019 •

edited by ghost

Loading