Skip to content

Replaced many exit(1)s with exception raising #1468

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
Dec 14, 2018
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
12 changes: 6 additions & 6 deletions pythonforandroid/archs.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
import sys
from distutils.spawn import find_executable

from pythonforandroid.logger import warning
from pythonforandroid.recipe import Recipe
from pythonforandroid.util import BuildInterruptingException


class Arch(object):
Expand Down Expand Up @@ -86,11 +86,11 @@ def get_env(self, with_flags_in_cc=True):
command_prefix=command_prefix), path=environ['PATH'])
if cc is None:
print('Searching path are: {!r}'.format(environ['PATH']))
warning('Couldn\'t find executable for CC. This indicates a '
'problem locating the {} executable in the Android '
'NDK, not that you don\'t have a normal compiler '
'installed. Exiting.')
exit(1)
raise BuildInterruptingException(
'Couldn\'t find executable for CC. This indicates a '
'problem locating the {} executable in the Android '
'NDK, not that you don\'t have a normal compiler '
'installed. Exiting.')

if with_flags_in_cc:
env['CC'] = '{ccache}{command_prefix}-gcc {cflags}'.format(
Expand Down
1 change: 0 additions & 1 deletion pythonforandroid/bdistapk.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ def finalize_options(self):
sys.argv.append('--arch={}'.format(arch))

def run(self):

self.prepare_build_dir()

from pythonforandroid.toolchain import main
Expand Down
55 changes: 24 additions & 31 deletions pythonforandroid/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@
import sh
import subprocess

from pythonforandroid.util import (ensure_dir, current_directory)
from pythonforandroid.logger import (info, warning, error, info_notify,
Err_Fore, info_main, shprint)
from pythonforandroid.util import (ensure_dir, current_directory, BuildInterruptingException)
from pythonforandroid.logger import (info, warning, info_notify, info_main, shprint)
from pythonforandroid.archs import ArchARM, ArchARMv7_a, ArchAarch_64, Archx86, Archx86_64
from pythonforandroid.recipe import Recipe

Expand Down Expand Up @@ -224,8 +223,7 @@ def prepare_build_environment(self,
'maintain your own SDK download.')
sdk_dir = possible_dirs[0]
if sdk_dir is None:
warning('Android SDK dir was not specified, exiting.')
exit(1)
raise BuildInterruptingException('Android SDK dir was not specified, exiting.')
self.sdk_dir = realpath(sdk_dir)

# Check what Android API we're using
Expand All @@ -244,11 +242,11 @@ def prepare_build_environment(self,
self.android_api = android_api

if self.android_api >= 21 and self.archs[0].arch == 'armeabi':
error('Asked to build for armeabi architecture with API '
'{}, but API 21 or greater does not support armeabi'.format(
self.android_api))
error('You probably want to build with --arch=armeabi-v7a instead')
exit(1)
raise BuildInterruptingException(
'Asked to build for armeabi architecture with API '
'{}, but API 21 or greater does not support armeabi'.format(
self.android_api),
instructions='You probably want to build with --arch=armeabi-v7a instead')

if exists(join(sdk_dir, 'tools', 'bin', 'avdmanager')):
avdmanager = sh.Command(join(sdk_dir, 'tools', 'bin', 'avdmanager'))
Expand All @@ -257,9 +255,9 @@ def prepare_build_environment(self,
android = sh.Command(join(sdk_dir, 'tools', 'android'))
targets = android('list').stdout.decode('utf-8').split('\n')
else:
error('Could not find `android` or `sdkmanager` binaries in '
'Android SDK. Exiting.')
exit(1)
raise BuildInterruptingException(
'Could not find `android` or `sdkmanager` binaries in Android SDK',
instructions='Make sure the path to the Android SDK is correct')
apis = [s for s in targets if re.match(r'^ *API level: ', s)]
apis = [re.findall(r'[0-9]+', s) for s in apis]
apis = [int(s[0]) for s in apis if s]
Expand All @@ -269,10 +267,9 @@ def prepare_build_environment(self,
info(('Requested API target {} is available, '
'continuing.').format(android_api))
else:
warning(('Requested API target {} is not available, install '
'it with the SDK android tool.').format(android_api))
warning('Exiting.')
exit(1)
raise BuildInterruptingException(
('Requested API target {} is not available, install '
'it with the SDK android tool.').format(android_api))

# Find the Android NDK
# Could also use ANDROID_NDK, but doesn't look like many tools use this
Expand Down Expand Up @@ -305,8 +302,7 @@ def prepare_build_environment(self,
'maintain your own NDK download.')
ndk_dir = possible_dirs[0]
if ndk_dir is None:
warning('Android NDK dir was not specified, exiting.')
exit(1)
raise BuildInterruptingException('Android NDK dir was not specified')
self.ndk_dir = realpath(ndk_dir)

# Find the NDK version, and check it against what the NDK dir
Expand Down Expand Up @@ -367,11 +363,11 @@ def prepare_build_environment(self,
self.ndk_api = ndk_api

if self.ndk_api > self.android_api:
error('Target NDK API is {}, higher than the target Android API {}.'.format(
self.ndk_api, self.android_api))
error('The NDK API is a minimum supported API number and must be lower '
'than the target Android API')
exit(1)
raise BuildInterruptingException(
'Target NDK API is {}, higher than the target Android API {}.'.format(
self.ndk_api, self.android_api),
instructions=('The NDK API is a minimum supported API number and must be lower '
'than the target Android API'))

info('Using {} NDK {}'.format(self.ndk.capitalize(), self.ndk_ver))

Expand Down Expand Up @@ -399,8 +395,7 @@ def prepare_build_environment(self,
self.cython = cython
break
else:
error('No cython binary found. Exiting.')
exit(1)
raise BuildInterruptingException('No cython binary found.')
if not self.cython:
ok = False
warning("Missing requirement: cython is not installed")
Expand Down Expand Up @@ -474,9 +469,8 @@ def prepare_build_environment(self,
executable))

if not ok:
error('{}python-for-android cannot continue; aborting{}'.format(
Err_Fore.RED, Err_Fore.RESET))
sys.exit(1)
raise BuildInterruptingException(
'python-for-android cannot continue due to the missing executables above')

def __init__(self):
super(Context, self).__init__()
Expand Down Expand Up @@ -524,8 +518,7 @@ def set_archs(self, arch_names):
new_archs.add(match)
self.archs = list(new_archs)
if not self.archs:
warning('Asked to compile for no Archs, so failing.')
exit(1)
raise BuildInterruptingException('Asked to compile for no Archs, so failing.')
info('Will compile for the following archs: {}'.format(
', '.join([arch.arch for arch in self.archs])))

Expand Down
32 changes: 15 additions & 17 deletions pythonforandroid/distribution.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@
import glob
import json

from pythonforandroid.logger import (info, info_notify, warning,
Err_Style, Err_Fore, error)
from pythonforandroid.util import current_directory
from pythonforandroid.logger import (info, info_notify, warning, Err_Style, Err_Fore)
from pythonforandroid.util import current_directory, BuildInterruptingException
from shutil import rmtree


Expand Down Expand Up @@ -132,17 +131,16 @@ def get_distribution(cls, ctx, name=None, recipes=[],
# then the existing dist is incompatible with the requested
# configuration and the build cannot continue
if name_match_dist is not None and not allow_replace_dist:
error('Asked for dist with name {name} with recipes ({req_recipes}) and '
'NDK API {req_ndk_api}, but a dist '
'with this name already exists and has either incompatible recipes '
'({dist_recipes}) or NDK API {dist_ndk_api}'.format(
name=name,
req_ndk_api=ndk_api,
dist_ndk_api=name_match_dist.ndk_api,
req_recipes=', '.join(recipes),
dist_recipes=', '.join(name_match_dist.recipes)))
error('No compatible dist found, so exiting.')
exit(1)
raise BuildInterruptingException(
'Asked for dist with name {name} with recipes ({req_recipes}) and '
'NDK API {req_ndk_api}, but a dist '
'with this name already exists and has either incompatible recipes '
'({dist_recipes}) or NDK API {dist_ndk_api}'.format(
name=name,
req_ndk_api=ndk_api,
dist_ndk_api=name_match_dist.ndk_api,
req_recipes=', '.join(recipes),
dist_recipes=', '.join(name_match_dist.recipes)))

# If we got this far, we need to build a new dist
dist = Distribution(ctx)
Expand Down Expand Up @@ -172,9 +170,9 @@ def delete(self):
def get_distributions(cls, ctx, extra_dist_dirs=[]):
'''Returns all the distributions found locally.'''
if extra_dist_dirs:
warning('extra_dist_dirs argument to get_distributions '
'is not yet implemented')
exit(1)
raise BuildInterruptingException(
'extra_dist_dirs argument to get_distributions '
'is not yet implemented')
dist_dir = ctx.dist_dir
folders = glob.glob(join(dist_dir, '*'))
for dir in extra_dist_dirs:
Expand Down
13 changes: 6 additions & 7 deletions pythonforandroid/graph.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
from copy import deepcopy
from itertools import product
from sys import exit

from pythonforandroid.logger import (info, warning, error)
from pythonforandroid.logger import (info, warning)
from pythonforandroid.recipe import Recipe
from pythonforandroid.bootstrap import Bootstrap
from pythonforandroid.util import BuildInterruptingException


class RecipeOrder(dict):
Expand Down Expand Up @@ -133,11 +133,10 @@ def get_recipe_order_and_bootstrap(ctx, names, bs=None):
key=lambda order: -('python2' in order) - ('sdl2' in order))

if not orders:
error('Didn\'t find any valid dependency graphs.')
error('This means that some of your requirements pull in '
'conflicting dependencies.')
error('Exiting.')
exit(1)
raise BuildInterruptingException(
'Didn\'t find any valid dependency graphs. This means that some of your '
'requirements pull in conflicting dependencies.')

# It would be better to check against possible orders other
# than the first one, but in practice clashes will be rare,
# and can be resolved by specifying more parameters
Expand Down
15 changes: 8 additions & 7 deletions pythonforandroid/recipe.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
from urlparse import urlparse
except ImportError:
from urllib.parse import urlparse
from pythonforandroid.logger import (logger, info, warning, error, debug, shprint, info_main)
from pythonforandroid.util import (urlretrieve, current_directory, ensure_dir)
from pythonforandroid.logger import (logger, info, warning, debug, shprint, info_main)
from pythonforandroid.util import (urlretrieve, current_directory, ensure_dir,
BuildInterruptingException)

# this import is necessary to keep imp.load_source from complaining :)
if PY2:
Expand Down Expand Up @@ -582,8 +583,8 @@ class IncludedFilesBehaviour(object):

def prepare_build_dir(self, arch):
if self.src_filename is None:
print('IncludedFilesBehaviour failed: no src_filename specified')
exit(1)
raise BuildInterruptingException(
'IncludedFilesBehaviour failed: no src_filename specified')
shprint(sh.rm, '-rf', self.get_build_dir(arch))
shprint(sh.cp, '-a', join(self.get_recipe_dir(), self.src_filename),
self.get_build_dir(arch))
Expand Down Expand Up @@ -1051,9 +1052,9 @@ def __init__(self, *args, **kwargs):
def prebuild_arch(self, arch):
super(TargetPythonRecipe, self).prebuild_arch(arch)
if self.from_crystax and self.ctx.ndk != 'crystax':
error('The {} recipe can only be built when '
'using the CrystaX NDK. Exiting.'.format(self.name))
exit(1)
raise BuildInterruptingException(
'The {} recipe can only be built when '
'using the CrystaX NDK. Exiting.'.format(self.name))
self.ctx.python_recipe = self

def include_root(self, arch):
Expand Down
10 changes: 5 additions & 5 deletions pythonforandroid/recipes/python3/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from pythonforandroid.recipe import TargetPythonRecipe, Recipe
from pythonforandroid.toolchain import shprint, current_directory
from pythonforandroid.logger import logger, info, error
from pythonforandroid.util import ensure_dir, walk_valid_filens
from pythonforandroid.logger import logger, info
from pythonforandroid.util import ensure_dir, walk_valid_filens, BuildInterruptingException
from os.path import exists, join, dirname
from os import environ
import glob
Expand Down Expand Up @@ -67,9 +67,9 @@ def set_libs_flags(self, env, arch):

def build_arch(self, arch):
if self.ctx.ndk_api < self.MIN_NDK_API:
error('Target ndk-api is {}, but the python3 recipe supports only {}+'.format(
self.ctx.ndk_api, self.MIN_NDK_API))
exit(1)
raise BuildInterruptingException(
'Target ndk-api is {}, but the python3 recipe supports only {}+'.format(
self.ctx.ndk_api, self.MIN_NDK_API))

recipe_build_dir = self.get_build_dir(arch.arch)

Expand Down
33 changes: 18 additions & 15 deletions pythonforandroid/toolchain.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from __future__ import print_function
from pythonforandroid import __version__
from pythonforandroid.build import DEFAULT_NDK_API, DEFAULT_ANDROID_API
from pythonforandroid.util import BuildInterruptingException, handle_build_exception


def check_python_dependencies():
Expand Down Expand Up @@ -58,7 +59,7 @@ def check_python_dependencies():
ok = False

if not ok:
print('python-for-android is exiting due to the errors above.')
print('python-for-android is exiting due to the errors logged above')
Copy link

Choose a reason for hiding this comment

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

Should there be an error raised here? Or is this change intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally did, but actually this code is run before most of the build process (because it has to run before the imports) so I think it needs a different treatment.

Copy link

@ghost ghost Dec 3, 2018

Choose a reason for hiding this comment

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

well but as it is, this seems to be a big change in behavior since the code flow just seems to resume. Is that intended? Maybe the exit should be left in if a runtime error doesn't make sense here? Or will this problem of not ok be evaluated again & properly dealt with later?

If you say it makes sense I'll take your word for it, I haven't looked at the followup code closely. it just seems like a potential trouble source breaking things

Copy link
Member Author

Choose a reason for hiding this comment

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

I also added the exit(1) back in with the previous comment, I missed that I'd accidentally removed it before so thanks for pointing it out!

Copy link

Choose a reason for hiding this comment

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

Ah right, the removing line is gone now. Didn't see that 😆

exit(1)


Expand All @@ -85,7 +86,7 @@ def check_python_dependencies():
from pythonforandroid.recipe import Recipe
from pythonforandroid.logger import (logger, info, warning, setup_color,
Out_Style, Out_Fore,
info_notify, info_main, shprint, error)
info_notify, info_main, shprint)
from pythonforandroid.util import current_directory
from pythonforandroid.bootstrap import Bootstrap
from pythonforandroid.distribution import Distribution, pretty_log_dists
Expand Down Expand Up @@ -638,7 +639,7 @@ def clean(self, args):

for component in components:
if component not in component_clean_methods:
raise ValueError((
raise BuildInterruptingException((
'Asked to clean "{}" but this argument is not '
'recognised'.format(component)))
component_clean_methods[component](args)
Expand Down Expand Up @@ -735,10 +736,10 @@ def export_dist(self, args):
ctx = self.ctx
dist = dist_from_args(ctx, args)
if dist.needs_build:
info('You asked to export a dist, but there is no dist '
'with suitable recipes available. For now, you must '
' create one first with the create argument.')
exit(1)
raise BuildInterruptingException(
'You asked to export a dist, but there is no dist '
'with suitable recipes available. For now, you must '
' create one first with the create argument.')
if args.symlink:
shprint(sh.ln, '-s', dist.dist_dir, args.output_dir)
else:
Expand Down Expand Up @@ -839,9 +840,8 @@ def apk(self, args):
elif args.build_mode == "release":
gradle_task = "assembleRelease"
else:
error("Unknown build mode {} for apk()".format(
args.build_mode))
exit(1)
raise BuildInterruptingException(
"Unknown build mode {} for apk()".format(args.build_mode))
output = shprint(gradlew, gradle_task, _tail=20,
_critical=True, _env=env)

Expand All @@ -858,9 +858,9 @@ def apk(self, args):
try:
ant = sh.Command('ant')
except sh.CommandNotFound:
error('Could not find ant binary, please install it '
'and make sure it is in your $PATH.')
exit(1)
raise BuildInterruptingException(
'Could not find ant binary, please install it '
'and make sure it is in your $PATH.')
output = shprint(ant, args.build_mode, _tail=20,
_critical=True, _env=env)
apk_dir = join(dist.dist_dir, "bin")
Expand Down Expand Up @@ -894,7 +894,7 @@ def apk(self, args):
apk_file = apks[-1]
break
else:
raise ValueError('Couldn\'t find the built APK')
raise BuildInterruptingException('Couldn\'t find the built APK')

info_main('# Found APK file: {}'.format(apk_file))
if apk_add_version:
Expand Down Expand Up @@ -1028,7 +1028,10 @@ def build_status(self, _args):


def main():
ToolchainCL()
try:
ToolchainCL()
except BuildInterruptingException as exc:
handle_build_exception(exc)


if __name__ == "__main__":
Expand Down
Loading