Skip to content

Feature: Requirements config option and handling #833

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.

Sign up for GitHub

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 3 commits into from
Feb 11, 2019
Merged

Feature: Requirements config option and handling #833

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8924fd
Introduce config flag 'no_requirements', which disables the built in …
screamerbg Feb 1, 2019
a675604
Remove version change
screamerbg Feb 8, 2019
fb420f0
Change flag names based on PR comments
screamerbg Feb 11, 2019
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
27 changes: 16 additions & 11 deletions mbed/mbed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1618,7 +1618,12 @@ def requirements_contains(self, library_name):
with open(os.path.join(req_path, req_file), 'r') as f:
return library_name in f.read()

def check_requirements(self, show_warning=False):
def check_requirements(self, require_install=False):
skip_requirements = self.get_cfg("NO_REQUIREMENTS", False)
if skip_requirements:
action("Skipping installed requirements check due to configuration flag.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this a verbose only print?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a non standard behavior and it's very easy to forget it. An extra line is not an overkill and it passes the responsibility to the user to handle requirements. Without the notice, there's no indication that one of the fundamental mbed CLI functions is not being performed because of user config.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having it under a verbose print makes sense, we already tell the user to run with -v if the subprocess exits with a non zero code: https://github.com/ARMmbed/mbed-cli/blob/master/mbed/mbed.py#L3388

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I already explained that not reminding that we're skipping all requirements check (which is NOT RECOMMENDED) should be clearly visible. Read my comment.

return True

req_path = self.get_requirements() or self.path
if not req_path:
return False
Expand All @@ -1636,7 +1641,7 @@ def check_requirements(self, show_warning=False):
if not pkg in installed_packages:
missing.append(pkg)

if missing and install_requirements:
if missing and install_requirements and require_install:
try:
action("Auto-installing missing Python modules (%s)..." % ', '.join(missing))
pquery([python_cmd, '-m', 'pip', 'install', '-q', '-r', os.path.join(req_path, req_file)])
Expand All @@ -1648,18 +1653,18 @@ def check_requirements(self, show_warning=False):

if missing:
msg = (
"Unable to auto-install required Python modules.\n"
"The mbed OS tools in this program require the following Python modules: %s\n"
"Missing Python modules were not auto-installed.\n"
"The Mbed OS tools in this program require the following Python modules: %s\n"
"You can install all missing modules by running \"pip install -r %s\" in \"%s\"" % (', '.join(missing), req_file, req_path))
if os.name == 'posix' and platform.system() == 'Darwin':
msg += "\nOn Mac you might have to install packages as your user by adding the \"--user\" flag"
elif os.name == 'posix':
msg += "\nOn Posix systems (Linux, etc) you might have to switch to superuser account or use \"sudo\""

if show_warning:
warning(msg)
else:
if require_install:
error(msg, 1)
else:
warning(msg)

return True

Expand Down Expand Up @@ -2632,7 +2637,7 @@ def compile_(toolchain=None, target=None, profile=False, compile_library=False,
args = remainder
# Find the root of the program
program = Program(getcwd(), True)
program.check_requirements(True)
program.check_requirements()
# Remember the original path. this is needed for compiling only the libraries and tests for the current folder.
orig_path = getcwd()
orig_target = target
Expand Down Expand Up @@ -2794,7 +2799,7 @@ def test_(toolchain=None, target=None, compile_list=False, run_list=False,
args = remainder
# Find the root of the program
program = Program(getcwd(), True)
program.check_requirements(True)
program.check_requirements()
# Check if current Mbed OS support icetea
icetea_supported = program.requirements_contains('icetea')

Expand Down Expand Up @@ -3034,7 +3039,7 @@ def export(ide=None, target=None, source=False, profile=["debug"], clean=False,
# Find the root of the program
program = Program(getcwd(), True)
if not no_requirements:
program.check_requirements(True)
program.check_requirements()
# Remember the original path. this is needed for compiling only the libraries and tests for the current folder.
orig_path = getcwd()
orig_target = target
Expand Down Expand Up @@ -3088,7 +3093,7 @@ def detect():
args = remainder
# Find the root of the program
program = Program(getcwd(), False)
program.check_requirements(True)
program.check_requirements()
# Change directories to the program root to use mbed OS tools
with cd(program.path):
tools_dir = program.get_tools_dir()
Expand Down