Feature: Requirements config option and handling #833

screamerbg · 2019-02-01T12:08:24Z

This PR tunes how Mbed OS python requirements are being installed:

Add support for (global & local) config flag 'no_requirements', which disables the built in check for installed python modules.
Mbed CLI should attempt to auto-install requirements when:
- Codebased is being imported (mbed import, mbed add, mbed deploy, mbed new)
- Codebased is being updated/checked out (mbed update)
- Certificates are being installed or changed (mbed dm)
Mbed CLI should only check and NOT try to auto-install requirements when:
- Code is being compiled, tested or exported
- Any Mbed CLI operation that doesn't imply network connectivity

As thoroughly described here #756.

…check for installed python modules. Additionally Mbed CLI will only report but not attempt to install missing python modules during `mbed compile`, `mbed test`, `mbed export`, `mbed detect`

screamerbg · 2019-02-01T13:35:08Z

Related to #795, #782 and #756

theotherjimmy

I think most people opting out of dependency updates will not want extra prints in their logs.

theotherjimmy · 2019-02-01T16:07:14Z

mbed/mbed.py

+    def check_requirements(self, show_warning_only=False):
+        skip_requirements = self.get_cfg("NO_REQUIREMENTS", False)
+        if skip_requirements:
+            action("Skipping installed requirements check due to configuration flag.")


Could we make this a verbose only print?

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.

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

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.

setup.py

bridadan

This PR should not come in as is due to a few issues listed below.

mbed/mbed.py

bridadan · 2019-02-07T18:08:34Z

mbed/mbed.py

@@ -2977,7 +2982,7 @@ def test_(toolchain=None, target=None, compile_list=False, run_list=False,
 def dev_mgmt(toolchain=None, target=None, source=False, profile=False, build=False):
    orig_path = getcwd()
    program = Program(getcwd(), True)
-    program.check_requirements(True)
+    program.check_requirements()


This is a behavior change, this will now cause the CLI to halt and exit with a non-zero return code if the requirements aren't present before running the mbed dm command.

I'm not necessarily against this, it just wasn't documented in the PR description or the commit message so I missed this at first.

To clarify the behavior:

Mbed CLI should attempt to auto-install requirements when:

Codebased is being imported (mbed import, mbed add, mbed deploy, mbed new)

Codebased is being updated/checked out (mbed update)

Certificates are being installed or changed (mbed dm)

Mbed CLI should only check and NOT try to auto-install requirements when:

Code is being compiled, tested or exported

Any Mbed CLI operation that doesn't imply network connectivity

As thoroughly described here #756. Also as mentioned in my first comment to this PR.

Related to #795, #782 and #756

bridadan · 2019-02-07T18:08:43Z

mbed/mbed.py

@@ -1681,7 +1686,7 @@ def post_action(self, check_reqs=True):
            shutil.copy(os.path.join(mbed_tools_path, 'default_settings.py'), os.path.join(self.path, 'mbed_settings.py'))

        if check_reqs:
-            self.check_requirements(True)
+            self.check_requirements()


This is a behavior change, this will now cause the CLI to halt and exit with a non-zero return code if the requirements aren't present after cloning (and auto-installing) mbed-os.

I'm not necessarily against this, it just wasn't documented in the PR description or the commit message so I missed this at first.

To clarify the behavior:

Mbed CLI should attempt to auto-install requirements when:

Codebased is being imported (mbed import, mbed add, mbed deploy, mbed new)

Codebased is being updated/checked out (mbed update)

Certificates are being installed or changed (mbed dm)

Mbed CLI should only check and NOT try to auto-install requirements when:

Code is being compiled, tested or exported

Any Mbed CLI operation that doesn't imply network connectivity

As thoroughly described here #756. Also as mentioned in my first comment to this PR.

Related to #795, #782 and #756

bridadan · 2019-02-07T18:13:15Z

mbed/mbed.py

@@ -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 not show_warning_only:


Oh hang on, so now the show_warning_only is being used to select if things should be auto installed? That doesn't sound right.

show_warning_only = show a warning and do not auto-install

@screamerbg Perhaps it could use a different name then?

@theotherjimmy Proposal?

require_install?and invert the logic so that True raises an error and False warns.

bridadan

Everything looks OK to me with the exception of the comment about the show_warning_only. My opinion is that flags for logging and modifying install behavior should be separate.

While I'd prefer to have the "Skipping installed requirements" print to only be printed in verbose, we can revisit this in a separate PR. Right now this feature is being requested by many folks so I'd prefer to have a solution for them. Thanks very much for contributing this @screamerbg .

screamerbg · 2019-02-11T17:01:45Z

To clarify the behavior:

Mbed CLI should attempt to auto-install requirements when:
- Codebased is being imported (mbed import, mbed add, mbed deploy, mbed new)
- Codebased is being updated/checked out (mbed update)
- Certificates are being installed or changed (mbed dm)
Mbed CLI should only check and NOT try to auto-install requirements when:
- Code is being compiled, tested or exported
- Any Mbed CLI operation that doesn't imply network connectivity

As thoroughly described here #756. Also as mentioned in my first comment to this PR.

Related to #795, #782 and #756

In addition to this, the behavior above can be overridden using the NO_REQUIREMENTS config flag, which is a severe deviation from the standard and established behavior of Mbed CLI (NOT RECOMMENDED) and it should be made clearly visible at all times.

bridadan · 2019-02-11T17:59:34Z

@screamerbg Thanks for the clarifications, I hadn't seen that issue before, that helps give the context.

I still would prefer to have the two flags separate in check_requirements. If you like I could add that in a follow up PR? If that works for you I think we can bring this in!

bridadan

@screamerbg, oh man, my GitHub didn't refresh! I see that you've pushed updated flags for that function. Looks good, thanks very much!

screamerbg · 2019-02-11T18:54:50Z

Hey man, no worries! I could have made that clearer, but I had to rush to meetings 😄

theotherjimmy

A follow up PR is fine for these updates.

Introduce config flag 'no_requirements', which disables the built in …

b8924fd

…check for installed python modules. Additionally Mbed CLI will only report but not attempt to install missing python modules during `mbed compile`, `mbed test`, `mbed export`, `mbed detect`

screamerbg requested review from cmonr and theotherjimmy February 1, 2019 12:08

theotherjimmy suggested changes Feb 1, 2019

View reviewed changes

bridadan suggested changes Feb 7, 2019

View reviewed changes

Remove version change

a675604

bridadan reviewed Feb 11, 2019

View reviewed changes

screamerbg mentioned this pull request Feb 11, 2019

Release 1.9.0 #842

Merged

Change flag names based on PR comments

fb420f0

bridadan approved these changes Feb 11, 2019

View reviewed changes

theotherjimmy approved these changes Feb 11, 2019

View reviewed changes

bridadan merged commit 308dc9f into ARMmbed:master Feb 11, 2019

theotherjimmy mentioned this pull request Feb 11, 2019

mbed compile can access the network, through pip #756

Closed

TacoGrandeTX mentioned this pull request Mar 11, 2019

MBED python requirements issues ? #782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Requirements config option and handling #833

Feature: Requirements config option and handling #833

screamerbg commented Feb 1, 2019 •

edited

Loading

screamerbg commented Feb 1, 2019

theotherjimmy left a comment

theotherjimmy Feb 1, 2019

screamerbg Feb 1, 2019

bridadan Feb 4, 2019

screamerbg Feb 8, 2019

bridadan left a comment

bridadan Feb 7, 2019

screamerbg Feb 11, 2019

bridadan Feb 7, 2019

screamerbg Feb 11, 2019

bridadan Feb 7, 2019

screamerbg Feb 8, 2019 •

edited

Loading

theotherjimmy Feb 8, 2019

screamerbg Feb 8, 2019

theotherjimmy Feb 8, 2019

bridadan left a comment

screamerbg commented Feb 11, 2019

bridadan commented Feb 11, 2019 •

edited

Loading

bridadan left a comment

screamerbg commented Feb 11, 2019

theotherjimmy left a comment

Feature: Requirements config option and handling #833

Feature: Requirements config option and handling #833

Conversation

screamerbg commented Feb 1, 2019 • edited Loading

screamerbg commented Feb 1, 2019

theotherjimmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

screamerbg Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridadan left a comment

Choose a reason for hiding this comment

screamerbg commented Feb 11, 2019

bridadan commented Feb 11, 2019 • edited Loading

bridadan left a comment

Choose a reason for hiding this comment

screamerbg commented Feb 11, 2019

theotherjimmy left a comment

Choose a reason for hiding this comment

screamerbg commented Feb 1, 2019 •

edited

Loading

screamerbg Feb 8, 2019 •

edited

Loading

bridadan commented Feb 11, 2019 •

edited

Loading