-
Notifications
You must be signed in to change notification settings - Fork 1.3k
#805 Removed unused dependencies from requirements.txt #806
Conversation
| configobj==5.0.6 | ||
| wikipedia==1.4.0 | ||
| requests==2.13.0 | ||
| pyOpenSSL==16.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pyOpenSSL is used, I think websockets depend on it (and/or requests). For certain OS's the 16.0.0 package will cause problems and 16.2.0 or 17.0.0 is needed to make mycroft work across platforms.
We can probably remove it and let it be installed implicitly when the package requiring it is installed but it might cause problems if an older OS is upgraded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I looked into that earlier, and requests depends on pyOpenSSL only if you use the security version of it, which would be requests@security. We don't use that, so we should be good there. What issues were you seeing where pyOpenSSL not being installed led to errors, and what platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the original issue leading to upgrading the version listed in requirements: #705
Seems to be urllib3
forslund
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see, this looks good. I'll leave the comment about pyOpenSSL for you to consider. I think it probably will be fine the way it is but we should be aware if an issue arises.
|
Can we merge this one? |
|
I want to do a little bit of looking into the PyOpenSSL issue first just to double check, and possibly change this to updating more requirements. |
|
@ethanaward can you look at #690 and if you think it's ok remove uuid from requirements.txt along with the rest of your updates. I've uninstalled it locally and mycroft works just fine with the one included in the standard library |
penrods
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this change once you are satisfied, Ethan.
|
@forslund is right, looks like uuid isn't needed. I'll rebase and remove it as well. |
db576a0 to
de4adb9
Compare
|
Tests are passing, this is good to go! |
This removes several dependencies from requirements.txt, as they are no longer used. I've tested Mycroft without these dependencies on desktop and everything seems to be working. I'd recommend testing on Picroft and the Mark 1 to make sure there aren't any issues there.