-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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, | ||
|
@@ -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 | ||
|
||
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. def build_as_debuggable(args):
return getattr(args, "build_mode", "debug") == "release" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
) == "debug" | ||
|
||
have_setup_py_or_similar = False | ||
if getattr(args, "private", None) is not None: | ||
|
@@ -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") | ||
|
||
|
@@ -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) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm surprised we're not using an actual |
||
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, \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Uh oh!
There was an error while loading. Please reload this page.