Try pip3 if pip is not found

[FabioRosado]

Description

Opsdroid runs smoothly again when the try/except is added to the loader.
I would like a feedback on the test if possible, I'm not sure if I'm testing it properly or not.

Fixes #385

Status

READY | UNDER DEVELOPMENT | ON HOLD

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

• Tox - tests passed
• Travis - Tests failed

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[jacobtomlinson]

Thanks for starting on this. A few thoughts:

• You've duplicated a fair bit of code here for a one character change. It would be better to create a variable with the command array in and then update it in the second call.
• It would be better to have the pip3 command in a try block too. Maybe move it out of the except and just return False if both processes fail. (Keep track of them with a variable).
• Tests are failing, could you take a look at what's causing it.

[jacobtomlinson]

#388 should resolve the CI issues you were having. I've updated your branch to include the new library.

[jacobtomlinson]

Great Travis has passed now.

[codecov]

Codecov Report

Merging #387 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #387   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          18     18           
  Lines        1049   1062   +13     
=====================================
+ Hits         1049   1062   +13

Impacted Files	Coverage Δ
opsdroid/loader.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 111bb53...1adac31. Read the comment docs.

[jacobtomlinson]

You could set the process variable to None before the try and then just pass in the except. Then do the pip3 try block and pass again in the except.

Then check if process is None and return if it still is. If either commands succeeded process will no loner be None.

    @staticmethod
    def pip_install_deps(requirements_path):
        """Pip install a requirements.txt file and wait for finish."""
        process = None
        command = ["pip", "install",
                   "--target={}".format(DEFAULT_MODULE_DEPS_PATH),
                   "--ignore-installed", "-r", requirements_path]

        try:
            process = subprocess.Popen(command,
                                        shell=False,
                                        stdout=subprocess.PIPE,
                                        stderr=subprocess.PIPE)
        except FileNotFoundError:
            pass

        try:
            command[0] = "pip3"
            process = subprocess.Popen(command,
                                        shell=False,
                                        stdout=subprocess.PIPE,
                                        stderr=subprocess.PIPE)
        except FileNotFoundError:
            pass

        if process is None:
            return False

        for output in process.communicate():
            if output != "":
                for line in output.splitlines():
                    _LOGGER.debug(str(line).strip())

        process.wait()
        return True

[FabioRosado]

Thanks for the help jacob, I wasn't sure how to implements you suggested fully the first time 👍
It seems that the test I created for the except raise isn't covering it for some reason, I'll try to play around with it and see if I can fix the issue

[jacobtomlinson]

Thinking about it we might want to raise an exception and exit instead of return False because if deps fails to install you can be reasonably sure opsdroid will not function correctly.

At the very least it probably needs to log an error saying it can't find pip.

[FabioRosado]

Is this the general function for any required installs for opsdroid? If this is just used to install requirements for skill/connectors it shouldn't be too bad, the skill would just not work I guess?

If its used to install everything then yeah I get it might be a problem

[jacobtomlinson]

It's for module dependency installs, so connectors, databases and skills.

[jacobtomlinson]

Looking good. Needs a test for both exceptions. Just needs a side effect on subprocess.Popen to raise the FileNotFoundError and the test should assert that it raises the overall error.

Also when raising if both pip installs fail perhaps it should be an OSError or a RuntimeError instead of a FileNotFoundError.

[FabioRosado]

I was just writing in gitter. I'm thinking in giving this a try:

    def test_no_pip_install(self):
        opsdroid, loader = self.setup()
        with mock.patch.object(subprocess, 'Popen') as mocked_popen:
            mocked_popen.side_effect = FileNotFoundError()
            self.assertRaises(FileNotFoundError, mocked_popen)
            with mock.patch.object(loader, 'pip_install_deps') as pip_install:
                mocked_popen.return_value.\
                    communicate.return_value = ['Test\nTest']
                loader.pip_install_deps("/path/to/some/file.txt")
                self.assertTrue(pip_install.called)

The way I think about it is that: popen raises the exception. Then it tries again and returns the right value (with pip3 I guess) and then it all passes. The code works on my end but just wanted to check with you.

I was unsure which exception to raise, I'll change it with OSError one instead.

[FabioRosado]

Hmm... okay that change in the tests didn't work

[jacobtomlinson]

I find it easier to use self.assertRaises in a with statement. That might help.

You've also patched with mock.patch.object(loader, 'pip_install_deps') further down which means your mocked_popen wont get called at all within that block.

[FabioRosado]

Oh wow... I have to say that I didn't expect this tests to pass but.... hey, they did!
I would still love to get some feedback from you @jacobtomlinson just to know if you would test this try/except block differently.

Thanks 👍

[jacobtomlinson]

This looks great! The only comment I have is that you don't need the OSError side effect in the test. The code will already be raising that error. You should be able to just remove that from the array and it's ready to be merged.