Conversation
inclement
left a comment
inclement
left a comment
There was a problem hiding this comment.
Thanks, no objection in principle but some questions about the details.
| tar_dirs.append(python_bundle_dir) | ||
| if get_bootstrap_name() == "webview": | ||
| tar_dirs.append('webview_includes') | ||
| if hasattr(args, "assets") and args.assets is not None: |
There was a problem hiding this comment.
I think argparse should make this simpler, isn't it an empty list by default or something so you can just do for asset in args.assets:?
There was a problem hiding this comment.
I thought I tried that and it didn't work. Maybe I misconfigured argparse.
There was a problem hiding this comment.
This is when we should coverage test this part as well. Not that I'm demanding it now, but I wish we had that already covered
|
|
||
| # Delete the old assets. | ||
| try_unlink(join(assets_dir, 'public.mp3')) | ||
| try_unlink(join(assets_dir, 'private.mp3')) |
There was a problem hiding this comment.
Looks like try_unlink isn't used any more, could you delete it?
pythonforandroid/toolchain.py
Outdated
| parser_apk.add_argument( | ||
| '--add-asset', dest='assets', | ||
| action="append", | ||
| help=('Put this in the assets folder')) |
There was a problem hiding this comment.
Unnecessary () around the string
pythonforandroid/toolchain.py
Outdated
| parser_apk.add_argument( | ||
| '--add-asset', dest='assets', | ||
| action="append", | ||
| help=('Put this in the assets folder')) |
There was a problem hiding this comment.
Could you document the : splitting thing in the help?
pythonforandroid/toolchain.py
Outdated
| asset_src, asset_dest = asset.split(":") | ||
| else: | ||
| asset_src = asset_dest = asset | ||
| args.unknown_args += ["--asset", os.path.abspath(asset_src)+":"+asset_dest] |
There was a problem hiding this comment.
Isn't this just unnecessary? The unknown_args include everything that wasn't parsed by the first p4a cmdparser, so if you just don't have a --add-asset in that argumentparser it will get passed through anyway. It seems you're pulling out the arguments, then adding them again (just with --asset instead of --add-asset)?
There was a problem hiding this comment.
AFAIK, the build.py is executed in a different working directory, so we need to convert the relative paths to absolute before all that. The unknown_args part is not the problem, os.path.abapath is.
There was a problem hiding this comment.
Ah, got it, that's fine then, we can leave that code pattern for fixing later. Thanks for the clarification.
| if os.path.exists(assets_dir): | ||
| shutil.rmtree(assets_dir) | ||
| ensure_dir(assets_dir) | ||
| open(os.path.join(assets_dir, ".gitkeep"), 'a').close() |
There was a problem hiding this comment.
Is this needed? Even if you're adding the output to git, it seems like this stuff should be handled at that later stage (although it's acceptable if there's some good reason for it).
There was a problem hiding this comment.
I asked about this on Discord. As it stands now, it's fine if the assets directory is completely deleted and recreated each time, but the behaviour used to be that the assets directory was kept so I tried to be consistent with that.
There was a problem hiding this comment.
Got it. I think it should be fine to ditch it and let the directory be deleted if you want, but if leaving this in could you add a comment noting this reasoning?
| if os.path.exists(assets_dir): | ||
| shutil.rmtree(assets_dir) |
There was a problem hiding this comment.
I'm allergic to if 😄
| if os.path.exists(assets_dir): | |
| shutil.rmtree(assets_dir) | |
| shutil.rmtree(assets_dir, ignore_errors=True) |
pythonforandroid/toolchain.py
Outdated
| # However, it is also needed before the distribution is finally | ||
| # assembled for locating the setup.py / other build systems, which | ||
| # is why we also add it here: | ||
| parser_apk.add_argument( |
There was a problem hiding this comment.
Could you cover test your change in tests/test_toolchain.py?
There're already some examples in this file and as I understand what you're doing in toolchain.py unit testing the behaviour should be fairly easy
Edit: forgot to mention I'd be glad to help if you're stuck
There was a problem hiding this comment.
Thanks for updating ❤️ this is looking good to me.
Let's wait for inclement approval otherwise I'll merge before this conflict further.
I wish we had (non blocking):
- unit tests for toolchain as suggested here #2132 (comment)
- squashed commits (I hope we think about it upon merging)
* command line option to add to the assets/ directory * allow custom destination paths * code review suggestions * fix bugs sleepliy introduced in commit "code review suggestions" * flake8 compliance
3 participants
Add an
--add-assetoption top4a apk.--add-asset assets/foowill copy the local directory./assets/footoassets/assets/fooin the apk. This can used multiple times, to add multiple files and directories.--add-asset /home/user/path/to/image.png:images/image.pngwill copy the file toassets/images/image.png.Probably not as useful with kivy, but very useful with SDL2 and SDL2-based frameworks, as SDL2 RWops use paths relative to the
assets/directory.