Fix debug build not resulting in gdb-debuggable build #1867
Conversation
ghost
changed the title
ghost
dismissed
|
@AndreMiras the pull request actually just leverages the |
ghost
changed the title
|
For what it's worth this is incredibly useful even if just because now |
ghost
changed the title
ghost
changed the title
|
As far as I'm concerned this is ready for some more testing & merge, I manually extracted the |
|
@Jonast, good work 😄!! I've tested this with I also reviewed the code and it looks good to me, so you have my OK, but I will leave the final word to @AndreMiras or @inclement Note: I see that you don't save the build mode into As a side note: I usually don't publish my apks and with this I will end up with bigger apks...unless I build it with |
|
Thanks for the feedback ❤️ You should always sign any public release with your own developer key so only you can publish updates. So I don't think a test release with no symbols but with debug signing makes too much sense. However, I think it might make sense to have a debug release with symbols that isn't signed with some random debug key - but that wasn't possible before this pull request either, so I'd say it is a separate feature |
|
Thanks again for the effort and discussion guys! I'll review/test/merge tonight. |
|
Not a good start, this breaks in buildozer it seems. Complete debug log: If we had better test coverage we would have caught that earlier I guess. I hope I can make some time to have basic |
|
@AndreMiras I added one first very basic test making sure that calling the argument parsers still works (which would have caught this mistake) 😄 Edit: oops, seems like you have a separate pull request for that already! I'll drop the additional test from this one then |
Increases `test_create()` coverage demonstrating crash referenced in: <kivy#1867 (comment)> Adds `test_recipes()` checking if it prints out without crashing.
Increases `test_create()` coverage demonstrating crash referenced in: <kivy#1867 (comment)> Adds `test_recipes()` checking if it prints out without crashing.
|
Thanks for the update. Could you review #1933 so we can rebase your PR on it once it's merged? |
There was a problem hiding this comment.
Really nice work overall 💪
I haven't re-tested it yet after your recent fix. Looks good in general, but wondering if we shouldn't pass debug option through the context rather than Recipe.build_as_debuggable. Probably @inclement knows more about a good design here.
Also like most of the new features making it to the tree, I really think this feature should definitely be unit tested. So basically demoing somehow that passing the release param we have the strip_object_files() being called while not called in debug mode.
pythonforandroid/recipe.py
Outdated
| '''Returns the Recipe with the given name, if it exists.''' | ||
| name = name.lower() | ||
| if not hasattr(cls, "recipes"): | ||
| cls.recipes = {} | ||
| if name in cls.recipes: | ||
| if ctx is not None: | ||
| cls.recipes[name].ctx = ctx | ||
| cls.recipes[name].build_as_debuggable = build_as_debuggable |
There was a problem hiding this comment.
This is really really confusing me. Why do we even have to do this since it's been done here below via:
recipe.build_as_debuggable = build_as_debuggable
recipe.ctx = ctx
cls.recipes[name.lower()] = recipePlus the whole thing seems unnatural to me, we're passing Recipe attributes from here while it's usually set a recipe level, feels like we're missing design patterns/vision somehow, no?
Couldn't the context be used to pass such things around?
There was a problem hiding this comment.
@AndreMiras for what it's worth the additional cls.recipes[name].ctx = ctx here is vital because the unit tests will call this function and obtain the same recipe via multiple contexts (for the different tests) and break otherwise in weird ways
Regarding build_as_debuggable, you are completely right. I will look into moving it 👍 👏 makes more sense on the context
There was a problem hiding this comment.
Thanks for looking into this
pythonforandroid/toolchain.py
Outdated
| ) | ||
|
|
||
| ctx.bootstrap.run_distribute() | ||
| ctx.bootstrap.run_distribute(debug_build=(args.build_mode == "debug")) |
There was a problem hiding this comment.
Minor: if it's the second time we use such debug_build as a boolean, maybe we could create a temp variable.
debug_build = args.build_mode == "debug"
tests/test_distribution.py
Outdated
| self.setUp_distribution_with_bootstrap( | ||
| Bootstrap().get_bootstrap("sdl2", self.ctx) | ||
| ) | ||
| self.ctx.bootstrap.distribution.folder_exists() | ||
| mock_exists.assert_called_once_with( | ||
| mock_exists.assert_called_with( |
There was a problem hiding this comment.
So is it called more than once now, how is that?
Is it worth documenting it with call_args_list then?
There was a problem hiding this comment.
It is called more than once, I have actually been thinking I should probably check why 😂 I changed it because it didn't seem obvious to me why it'd be important that it is only called once, it just didn't seem like a condition important to guarantee
There was a problem hiding this comment.
Yes I agree the condition may indeed not be important to guarantee. But if the behavior changed and it's not so obvious why, sometimes it's worth to dig slightly more. We may see we introduced something we didn't want to.
I can help digging later when you're done introducing the context update
ghost
changed the title
|
Thanks for all the feedback! I'll rebase and put |
ghost
changed the title
There was a problem hiding this comment.
I haven't tested it runtime yet, but it looks good in general.
Something that disappoints me is it has absolutely no unit tests. I was expecting some tests like showing strip_object_files() is being called based on the args.build_mode provided or at least based on Context.build_as_debuggable status
pythonforandroid/recipe.py
Outdated
| @@ -583,6 +583,8 @@ def get_recipe(cls, name, ctx): | |||
| if not hasattr(cls, "recipes"): | |||
| cls.recipes = {} | |||
| if name in cls.recipes: | |||
| if ctx is not None: | |||
| cls.recipes[name].ctx = ctx | |||
There was a problem hiding this comment.
That really gets me curious 🤔
Why would we need to override the context of the cached recipe? It should already be there via this https://github.com/kivy/python-for-android/blob/v2019.07.08/pythonforandroid/recipe.py#L610 right?
Edit: I just recall you replied to that comment already #1867 (comment) but I feel something is fishy still, don't you think?
There was a problem hiding this comment.
Well the fishiness is that the code is written under the assumption just one context will ever be relevant which obviously isn't true at least for the tests, so the whole static caching may not be an ideal design. But for regular program execution of p4a it probably won't matter, but sure it is pretty weird in its inherent assumptions - not that this is really caused by this pull request
There was a problem hiding this comment.
OK then if it's the test that are misbehaving, should the tests be updated?
Anyway that looked properly weird to me so I checked out the branch and tried removing that lines. Tests are passing just fine
There was a problem hiding this comment.
@AndreMiras it depends on the test execution order I think, and if you just look at the tests it is obvious it is inherently, necessarily going to blow up. Look in tests/test_recipe.py, multiple tests construct a separate Context() for themselves and these contexts are used for obtaining a recipe, so obviously if any two tests ever obtain the same recipe you already got the disaster spelled out (because now it's the recipe but attached with the wrong context of a previous test, and that can introduce all sorts of subtle wrong behavior)
Fixing the tests from my first conclusions doesn't make too much sense, because why even have the ctx parameter for getting a recipe if it's ignored? I think the bandaid most reasonable fix is what I added here, and the long-term fix might be to rethink this cached recipe approach in different ways (e.g. cache it with a dictionary mapping to context with per-context-and-recipe-pair singleton rather than global per-recipe singleton)
There was a problem hiding this comment.
OK I think I understand what you mean. But then in this case I still feel we're increasing the technical debt (if any) by adding this line that just feels unnatural to me 😕
Unit tests should be definition be isolated and not leave the context modified from one to another unless we specifically want to. And we should not put a workaround in the code to fix this misbehaviour. I don't think static attributes is anything new in Python or in other languages, same things like with singleton and stuff. When we have one we need to carefully write tests to not leave it in a state we don't want to. I know I'm completely guilty of writing some of such tests, thinking "it's fine I'll address when it impacts us"
There was a problem hiding this comment.
Just to emphasize this again, I think it is wrong to fix this in the tests. get_recipe has a context/ctx parameter, it is completely counter-intuitive and somewhat nonsensical to me that it is ever ignored. So if there is any fix I think it has to be inside get_recipe
I'm open to dropping this from the pull request (I put it on because I saw spurious test failures) but I would expect us to see weird errors down the line unless we do something about it. I agree this bandaid is not the nicest way to fix it, but breaking up the singleton design entirely also could have tons of implications and break a lot of things when done thoughtlessly, so I don't think a good solution will come quickly.
and not leave the context modified
I'm against "fixing" anything in the tests themselves since again, I think this is just hiding the design problem of get_recipe() here and a worse technical debt than this workaround. I think the function header doesn't suggest such a context would stick around, so that it does is IMHO the actual problem, not the tests
There was a problem hiding this comment.
Very sad to ear 😢
In the order of preference I personally have (the first being my preferred solution):
- of course a proper fix by design if you think it's a design flow (but as you said that would involve a lot of changes)
- introduce "work-around" in the tests (I've implemented an example in Adds new
backup_attributescontext manager for tests #1948) - introduce "work-around" in the code (what you're currently implementing)
Anyway let's assume we keep your implementation of 3). Then could you please write a dedicated and comprehensive test (and docstring) showing this is needed and avoiding later regressions. I reckon the test should be fairly easy to write I think.
Then two thing come to my mind.
- with your workaround, do we still need this line then https://github.com/kivy/python-for-android/blob/v2019.07.08/pythonforandroid/recipe.py#L610 ?
- why in your workaround
Noneis treated differently? Let's cover that case as well
There was a problem hiding this comment.
I think a work-around in a test is just masking it, so that seems risky. However what you proposed as workaround actually seems like a useful long-term test outcome stability measure so it doesn't seem like the worst idea - just not good as a "solution" for dealing with this actual issue
I think we need to fix it, I'm just not sure how yet. I'll sleep about it and see if I can think of what might be the best way to address this 👍
There was a problem hiding this comment.
(oh and I'll also back this "fix" out of this pull request here unless you disagree, since this is growing way too big/unrelated and needs more thought. I suggest we should handle it separately)
There was a problem hiding this comment.
Yes I would prefer to not have any of theses workaround or fixes in the PR. But if we do, then I would like them properly unit tested
| @@ -587,6 +594,9 @@ def add_parser(subparsers, *args, **kwargs): | |||
|
|
|||
| self.ctx = Context() | |||
| self.ctx.use_setup_py = getattr(args, "use_setup_py", True) | |||
| self.ctx.build_as_debuggable = getattr( | |||
| args, "build_mode", "debug" | |||
There was a problem hiding this comment.
Minor: seems like we're repeating ourselves in a similar fashion few times, enough to make an helper function maybe.
Maybe something like:
def build_as_debuggable(args):
return getattr(args, "build_mode", "debug") == "release"There was a problem hiding this comment.
I just tried this, and I have the feeling the actual problem is we want something like #1812 instead so we can do simple (args.build_mode == "debug") everywhere and it'll look ok. With your suggested build_as_debuggable(...) the problem is that sometimes "release" and "debug" are checked separately and it looks like someone may want to add a mode in these places so dropping this in everywhere would kinda look weird & restrictive I think, but in only half the places it also looks weird. I think the real problem really is hasattr/getattr making this look ugly
There was a problem hiding this comment.
Yes hasattr/getattr definitely makes it look hugly, but testing for debug or release for me is just like testing for True or False. It's a two state, so you can always negate it. Yes it can also be three state, but I don't see it as a problem.
Anyway the PR looks fine overall and it's unit tested, so I'm also happy to keep it this way
oh no 😱 and yeah, you're right I'm on it! 👍 I should manage to produce at least one decent test for this on the |
This context manager will makes it possible to rollback an object state after leaving the context manager. This is a follow up of <kivy#1867 (comment)> Also address docstring format as suggested by @opacam in <kivy#1933 (comment)>
This context manager will makes it possible to rollback an object state after leaving the context manager. This is a follow up of <kivy#1867 (comment)> Also address docstring format as suggested by @opacam in <kivy#1933 (comment)>
This context manager will makes it possible to rollback an object state after leaving the context manager. This is a follow up of <kivy#1867 (comment)> Also address docstring format as suggested by @opacam in <kivy#1933 (comment)>
ghost
changed the title
|
@AndreMiras I'm struggling a bit to get some of the mocking to work, you wouldn't happen to know why I'll probably find out myself soon enough, but if you have a minute to have a look & a good idea that could be super useful ❤️ 😍 |
|
Yes the mock path is wrong. Most of the time you need to mock the path where it's imported, not where it's defined. |
tests/test_build.py
Outdated
| ctx.get_site_packages_dir.return_value = "test-doesntexist" | ||
| ctx.build_dir = "nonexistant_directory" | ||
| ctx.archs = ["arm64"] | ||
| ctx.build_as_debuggable = False |
There was a problem hiding this comment.
Minor: This one is not needed right, since you're doing ctx.build_as_debuggable = True later on just before calling run_pymodules_install
| mock.patch('pythonforandroid.build.open'),\ | ||
| mock.patch('pythonforandroid.build.shprint'),\ | ||
| mock.patch('pythonforandroid.build.current_directory'),\ | ||
| mock.patch('pythonforandroid.build.CythonRecipe') as m_CythonRecipe, \ |
There was a problem hiding this comment.
Minor: I usually try to patch the actual method to have the least impact, hence something like:
mock.patch('pythonforandroid.build.CythonRecipe.strip_object_files') as m_strip_object_filesThere was a problem hiding this comment.
it felt safer that way, to reduce any side effects I'm not interested in and I don't really need any functionality of the recipe anyway in this test, so it was trivial to mock a bit more far-reaching
| @@ -23,3 +23,33 @@ def test_run_pymodules_install_optional_project_dir(self): | |||
| assert run_pymodules_install(ctx, modules, project_dir) is None | |||
| assert m_info.call_args_list[-1] == mock.call( | |||
| 'No Python modules and no setup.py to process, skipping') | |||
|
|
|||
| def test_strip_if_debuggable(self): | |||
| ctx = mock.Mock() | |||
There was a problem hiding this comment.
I'm surprised we're not using an actual Context object, but I guess why not 😄
tests/test_build.py
Outdated
| # Make sure it is NOT called when debug build: | ||
| ctx.build_as_debuggable = True | ||
| assert run_pymodules_install(ctx, modules, project_dir) is None | ||
| self.assertFalse(m_CythonRecipe().strip_object_files.called) |
There was a problem hiding this comment.
Minor: why don't we make it consistent via the assert keyword?
assert m_CythonRecipe().strip_object_files.called is FalseNote how I purposely didn't write:
assert not m_CythonRecipe().strip_object_files.calledBecause I also want to enforce type checking, specially for mocks and unit testing
- fixes that a debug (non-`--release`) `.apk` build won't actually enable gdb debugging in the manifest - renames `build.py`'s `parse_args` to `parse_args_and_make_package` because that is what it appears to be actually doing 😄 - makes SDL2 and any standard `NDKRecipe` build with `NDK_DEBUG=1` when `--release` is not specified to add debugging symbols - fixes debugging symbols being stripped even when not using `--releaase` which makes using gdb a hassle
ghost
changed the title
|
Hi @jtc0de, I got side tracked and never merged. But this is looking good I think. |
|
Hi @etc0de I have a few questions: does this fix enable using Have you tested debugging built apps on an Android device? If so, what is the process like? Another: Say a recipe (which is not inheriting |
Yes, you open the resulting APK with the debugging symbols directly in Android Studio (without using a regular project file) and then just press the regular debug launch button and the IDE does everything like launching it, connecting gdb, etc. You may need to pick the proper API level in the APK "project" settings though since somehow broken Android Studio failed to set this from the APK for me for some reason. (And in that case, it'll complain and not launch since it doesn't know what NDK debugger version to use.) All of this also already works without debugging symbols and without this pull request, but obviously then all the traces you get in gdb are useless & cryptic which this pull request addresses. It just fixes the gdb output, basically.
The recipe needs to be changed to add in debugging symbols when the build context's |
|
Just fixed the only merge conflict, assuming the tests pass then let's go ahead and merge. |
|
@inclement what sort of conflict did you get? I just attempted a rebase as a test and it went without a conflict, maybe merging develop into it isn't that optimal Edit: oops, didn't rebase on the latest, where there is indeed a small conflict in the gradle file. Nevertheless, rebased it looks way cleaner without all the weird merge commit history. Do you want me to replace this one with the rebased one? (rebased one currently lives under branch |
|
Looking good to me, thanks for fixing the conflicts. |
This pull request does the following things:
--release).apkbuild won't actually enable gdb debugging in the manifestbuild.py'sparse_argstoparse_args_and_make_packagebecause that is what it appears to be actually doing smileNDKRecipebuild withNDK_DEBUG=1when--releaseis not specified to add debugging symbols--releasewhich makes using gdb a hassle