Skip to content
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.

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

MSM rerun of skill requirements.txt once per session #1215

Merged
merged 1 commit into from Nov 9, 2017

Conversation

penrods
Copy link
Contributor

@penrods penrods commented Nov 9, 2017

A recent PR (#1192) eliminated the requirements that only applied to default
skills, not to the core itself. However this caused problems for skills that
were previously installed and had their PIP requirements satisfied by the
packages that came along with the mycroft-core Debian package previously.

As a hackinfix, we now re-run PIP on each skill during the MSM update, but
to limit the performance impact this only happens once per session. We
shouldn't to removing packages again in the future, so this should be a
one-time act that gets removed from MSM in the future (like at 18.02).

A recent PR (#1192) eliminated the requirements that only applied to default
skills, not to the core itself.  However this caused problems for skills that
were previously installed and had their PIP requirements satisfied by the
packages that came along with the mycroft-core Debian package previously.

As a hackinfix, we now re-run PIP on each skill during the MSM update, but
to limit the performance impact this only happens once per session.  We
shouldn't to removing packages again in the future, so this should be a
one-time act that gets removed from MSM in the future (like at 18.02).
@penrods penrods added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Nov 9, 2017
Copy link
Collaborator

@devs-mycroft devs-mycroft left a comment

Choose a reason for hiding this comment

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

Looks like this works, will test in packaging after merge.

@devs-mycroft devs-mycroft merged commit 558179a into dev Nov 9, 2017
@penrods penrods deleted the feature/fix-requirements branch November 9, 2017 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants