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

Always install default skills before initial load #2644

Merged
merged 2 commits into from
Aug 11, 2020
Merged

Always install default skills before initial load #2644

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0756da7
Always install default skills before initial load
forslund Jul 29, 2020
49faade
Don't include blacklisted skills in defaults
forslund Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions mycroft/skills/skill_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,10 @@ def run(self):
"""Load skills and update periodically from disk and internet."""
self._remove_git_locks()
self._connected_event.wait()
if not self.skill_updater.defaults_installed():
LOG.info('Not all default skills are installed, '
'performing skill update...')
self.skill_updater.update_skills()
self._load_on_startup()

# Sync backend and skills.
Expand Down
9 changes: 9 additions & 0 deletions mycroft/skills/skill_updater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,15 @@ def install_or_update(self, skill):
raise
self.installed_skills.add(skill.name)

def defaults_installed(self):
"""Check if all default skills are installed.

Returns:
True if all default skills are installed, else False.
"""
defaults = self.msm.list_all_defaults()[self.msm.platform]
return all([skill.is_local for skill in defaults])
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to account for people blacklisting default Skills?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, blacklisting skills still installs them, it just doesn't load them afaik. It's an update we can do but we should probably change the update method as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've pushed an update to exclude blacklisted skills from the default check. I've also looked at excluding blacklisted skills from the install/update procedure which is fairly easy to add, if we think that should be added as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Working great.

Yeah I think that makes sense too. If someone has blacklisted a Skill we don't need to install it. Though if there is some security update, perhaps we do want to update?

I see that update and install are handled separately so we could do updates but not installs? Really though, this is a nice to have so I'm going to merge this PR as is and we can do the above in the future.


def _get_device_skill_state(self, skill_name):
"""Get skill data structure from name."""
device_skill_state = {}
Expand Down