Skip to content

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

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

Merged
merged 2 commits into from Apr 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions pythonforandroid/bootstraps/common/build/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,8 @@ def make_package(args):
"args": args,
"service": service,
"service_names": service_names,
"android_api": android_api
"android_api": android_api,
"debug": "debug" in args.build_mode,
}
if get_bootstrap_name() == "sdl2":
render_args["url_scheme"] = url_scheme
Expand All @@ -478,7 +479,8 @@ def make_package(args):
aars=aars,
jars=jars,
android_api=android_api,
build_tools_version=build_tools_version
build_tools_version=build_tools_version,
debug_build="debug" in args.build_mode,
)

# ant build templates
Expand Down Expand Up @@ -543,7 +545,7 @@ def make_package(args):
raise e


def parse_args(args=None):
def parse_args_and_make_package(args=None):
global BLACKLIST_PATTERNS, WHITELIST_PATTERNS, PYTHON

# Get the default minsdk, equal to the NDK API that this dist is built against
Expand Down Expand Up @@ -661,6 +663,10 @@ def parse_args(args=None):
default=join(curdir, 'whitelist.txt'),
help=('Use a whitelist file to prevent blacklisting of '
'file in the final APK'))
ap.add_argument('--release', dest='build_mode', action='store_const',
const='release', default='debug',
help='Build your app as a non-debug release build. '
'(Disables gdb debugging among other things)')
ap.add_argument('--add-jar', dest='add_jar', action='append',
help=('Add a Java .jar to the libs, so you can access its '
'classes with pyjnius. You can specify this '
Expand Down Expand Up @@ -807,4 +813,4 @@ def _read_configuration():


if __name__ == "__main__":
parse_args()
parse_args_and_make_package()
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,22 @@ android {
versionName '{{ args.version }}'
}

{% if args.sign -%}
signingConfigs {
release {
storeFile file(System.getenv("P4A_RELEASE_KEYSTORE"))
keyAlias System.getenv("P4A_RELEASE_KEYALIAS")
storePassword System.getenv("P4A_RELEASE_KEYSTORE_PASSWD")
keyPassword System.getenv("P4A_RELEASE_KEYALIAS_PASSWD")
}
}
{% if debug_build -%}
packagingOptions {
doNotStrip '**/*.so'
}
{%- endif %}

{% if args.sign -%}
signingConfigs {
release {
storeFile file(System.getenv("P4A_RELEASE_KEYSTORE"))
keyAlias System.getenv("P4A_RELEASE_KEYALIAS")
storePassword System.getenv("P4A_RELEASE_KEYSTORE_PASSWD")
keyPassword System.getenv("P4A_RELEASE_KEYALIAS_PASSWD")
}
}

{%- endif %}

{% if args.packaging_options -%}
Expand Down
3 changes: 2 additions & 1 deletion pythonforandroid/bootstraps/sdl2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ def run_distribute(self):
with open('blacklist.txt', 'a') as fileh:
fileh.write('\nsqlite3/*\nlib-dynload/_sqlite3.so\n')

self.strip_libraries(arch)
if not self.ctx.build_as_debuggable:
self.strip_libraries(arch)
self.fry_eggs(site_packages_dir)
super().run_distribute()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
An example Java class can be found in README-android.txt
-->
<application android:label="@string/app_name"
{% if debug %}android:debuggable="true"{% endif %}
android:icon="@drawable/icon"
android:allowBackup="{{ args.allow_backup }}"
android:theme="{{args.android_apptheme}}{% if not args.window %}.Fullscreen{% endif %}"
Expand Down
3 changes: 2 additions & 1 deletion pythonforandroid/bootstraps/service_only/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ def run_distribute(self):
with open('blacklist.txt', 'a') as fileh:
fileh.write('\nsqlite3/*\nlib-dynload/_sqlite3.so\n')

self.strip_libraries(arch)
if not self.ctx.build_as_debuggable:
self.strip_libraries(arch)
self.fry_eggs(site_packages_dir)
super().run_distribute()

Expand Down
3 changes: 2 additions & 1 deletion pythonforandroid/bootstraps/webview/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ def run_distribute(self):
with open('blacklist.txt', 'a') as fileh:
fileh.write('\nsqlite3/*\nlib-dynload/_sqlite3.so\n')

self.strip_libraries(arch)
if not self.ctx.build_as_debuggable:
self.strip_libraries(arch)
self.fry_eggs(site_packages_dir)
super().run_distribute()

Expand Down
11 changes: 7 additions & 4 deletions pythonforandroid/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ class Context(object):
'''A build context. If anything will be built, an instance this class
will be instantiated and used to hold all the build state.'''

# Whether to build with debugging symbols
build_as_debuggable = False

env = environ.copy()
# the filepath of toolchain.py
root_dir = None
Expand Down Expand Up @@ -605,8 +608,6 @@ def build_recipes(build_order, python_modules, ctx, project_dir,
ignore_setup_py=ignore_project_setup_py
)

return


def project_has_setup_py(project_dir):
if project_dir is not None and \
Expand Down Expand Up @@ -851,8 +852,10 @@ def run_pymodules_install(ctx, modules, project_dir=None,
)

# Strip object files after potential Cython or native code builds:
standard_recipe.strip_object_files(ctx.archs[0], env,
build_dir=ctx.build_dir)
if not ctx.build_as_debuggable:
standard_recipe.strip_object_files(
ctx.archs[0], env, build_dir=ctx.build_dir
)


def biglink(ctx, arch):
Expand Down
4 changes: 3 additions & 1 deletion pythonforandroid/recipe.py
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,7 @@ def build_arch(self, arch, *extra_args):
shprint(
sh.ndk_build,
'V=1',
'NDK_DEBUG=' + ("1" if self.ctx.build_as_debuggable else "0"),
'APP_PLATFORM=android-' + str(self.ctx.ndk_api),
'APP_ABI=' + arch.arch,
*extra_args, _env=env
Expand Down Expand Up @@ -1080,7 +1081,8 @@ def build_cython_components(self, arch):
info('First build appeared to complete correctly, skipping manual'
'cythonising.')

self.strip_object_files(arch, env)
if not self.ctx.build_as_debuggable:
self.strip_object_files(arch, env)

def strip_object_files(self, arch, env, build_dir=None):
if build_dir is None:
Expand Down
7 changes: 6 additions & 1 deletion pythonforandroid/recipes/sdl2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ def build_arch(self, arch):
env = self.get_recipe_env(arch)

with current_directory(self.get_jni_dir()):
shprint(sh.ndk_build, "V=1", _env=env)
shprint(
sh.ndk_build,
"V=1",
"NDK_DEBUG=" + ("1" if self.ctx.build_as_debuggable else "0"),
_env=env
)


recipe = LibSDL2Recipe()
20 changes: 17 additions & 3 deletions pythonforandroid/toolchain.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ def build_dist_from_args(ctx, dist, args):
ctx.recipe_build_order))
info('Dist will also contain modules ({}) installed from pip'.format(
', '.join(ctx.python_modules)))
if hasattr(args, "build_mode") and args.build_mode == "debug":
info('Building WITH debugging symbols (no --release option used)')
else:
info('Building WITHOUT debugging symbols (--release option used)')

ctx.distribution = dist
ctx.prepare_bootstrap(bs)
Expand Down Expand Up @@ -499,7 +503,8 @@ def add_parser(subparsers, *args, **kwargs):
parser_apk.add_argument(
'--release', dest='build_mode', action='store_const',
const='release', default='debug',
help='Build the PARSER_APK. in Release mode')
help='Build your app as a non-debug release build. '
'(Disables gdb debugging among other things)')
parser_apk.add_argument(
'--use-setup-py', dest="use_setup_py",
action='store_true', default=False,
Expand Down Expand Up @@ -576,6 +581,8 @@ def add_parser(subparsers, *args, **kwargs):
if hasattr(args, "private") and args.private is not None:
# Pass this value on to the internal bootstrap build.py:
args.unknown_args += ["--private", args.private]
if hasattr(args, "build_mode") and args.build_mode == "release":
args.unknown_args += ["--release"]
if hasattr(args, "ignore_setup_py") and args.ignore_setup_py:
args.use_setup_py = False

Expand All @@ -592,6 +599,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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Author

@ghost ghost Aug 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

) == "debug"

have_setup_py_or_similar = False
if getattr(args, "private", None) is not None:
Expand Down Expand Up @@ -958,7 +968,9 @@ def apk(self, args):
with current_directory(dist.dist_dir):
self.hook("before_apk_build")
os.environ["ANDROID_API"] = str(self.ctx.android_api)
build_args = build.parse_args(args.unknown_args)
build_args = build.parse_args_and_make_package(
args.unknown_args
)
self.hook("after_apk_build")
self.hook("before_apk_assemble")

Expand Down Expand Up @@ -1008,7 +1020,9 @@ def apk(self, args):
gradle_task = "assembleRelease"
else:
raise BuildInterruptingException(
"Unknown build mode {} for apk()".format(args.build_mode))
"Unknown build mode {} for apk()".
format(args.build_mode)
)
output = shprint(gradlew, gradle_task, _tail=20,
_critical=True, _env=env)

Expand Down
29 changes: 29 additions & 0 deletions tests/test_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,32 @@ 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised we're not using an actual Context object, but I guess why not 😄

ctx.python_recipe.major_minor_version_string = "python3.6"
ctx.get_site_packages_dir.return_value = "test-doesntexist"
ctx.build_dir = "nonexistant_directory"
ctx.archs = ["arm64"]

modules = ["mymodule"]
project_dir = None
with mock.patch('pythonforandroid.build.info'), \
mock.patch('sh.Command'),\
mock.patch('pythonforandroid.build.open'),\
mock.patch('pythonforandroid.build.shprint'),\
mock.patch('pythonforandroid.build.current_directory'),\
mock.patch('pythonforandroid.build.CythonRecipe') as m_CythonRecipe, \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_files

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mock.patch('pythonforandroid.build.project_has_setup_py') as m_project_has_setup_py, \
mock.patch('pythonforandroid.build.run_setuppy_install'):
m_project_has_setup_py.return_value = False

# Make sure it is NOT called when debug build:
ctx.build_as_debuggable = True
assert run_pymodules_install(ctx, modules, project_dir) is None
assert m_CythonRecipe().strip_object_files.called is False

# Make sure strip object files IS called when release build:
ctx.build_as_debuggable = False
assert run_pymodules_install(ctx, modules, project_dir) is None
assert m_CythonRecipe().strip_object_files.called is True