Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Conversation

@ethanaward
Copy link
Contributor

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.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 38.834% when pulling 3907919 on bugfix/issues-805 into fb89ae9 on dev.

configobj==5.0.6
wikipedia==1.4.0
requests==2.13.0
pyOpenSSL==16.2.0
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

@forslund forslund Jun 13, 2017

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

Copy link
Collaborator

@forslund forslund left a 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.

@augustnmonteiro
Copy link
Contributor

Can we merge this one?

@ethanaward
Copy link
Contributor Author

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.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 40.1% when pulling db576a0 on bugfix/issues-805 into 8629bb9 on dev.

@forslund
Copy link
Collaborator

forslund commented Jul 1, 2017

@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

Copy link
Contributor

@penrods penrods left a 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.

@ethanaward
Copy link
Contributor Author

ethanaward commented Jul 6, 2017

@forslund is right, looks like uuid isn't needed. I'll rebase and remove it as well.

@ethanaward ethanaward force-pushed the bugfix/issues-805 branch from db576a0 to de4adb9 Compare July 6, 2017 17:15
@coveralls
Copy link

Coverage Status

Coverage remained the same at 39.564% when pulling de4adb9 on bugfix/issues-805 into f230289 on dev.

@ethanaward
Copy link
Contributor Author

Tests are passing, this is good to go!

@augustnmonteiro augustnmonteiro merged commit 4136246 into dev Jul 7, 2017
@augustnmonteiro augustnmonteiro deleted the bugfix/issues-805 branch July 7, 2017 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants