From 56ed5a657f3c229dd2964db9ab352ad49a69c534 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Mon, 8 Apr 2019 10:59:40 +0200 Subject: [PATCH 01/10] Re-add support for skill_gid in settingsmeta upload --- .travis.yml | 4 + mycroft/skills/msm_wrapper.py | 43 ++++++ mycroft/skills/settings.py | 140 +++++++++++++++++--- mycroft/skills/skill_manager.py | 20 +-- requirements.txt | 2 +- test/unittests/skills/test_settings.py | 5 + test/unittests/skills/test_skill_manager.py | 18 +-- test/unittests/util/test_signal.py | 17 ++- 8 files changed, 196 insertions(+), 53 deletions(-) create mode 100644 mycroft/skills/msm_wrapper.py diff --git a/.travis.yml b/.travis.yml index c9c24b14a9..cd339943a1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,6 +6,7 @@ before_install: - sudo apt-get install -qq mpg123 portaudio19-dev libglib2.0-dev swig bison libtool autoconf libglib2.0-dev libicu-dev libfann-dev realpath - sudo apt-get install -y gcc-4.8 g++-4.8 - export CC="gcc-4.8" + - export TMPDIR="/tmp/${TRAVIS_PYTHON_VERSION}" python: - "3.4" - "3.5" @@ -15,6 +16,9 @@ python: cache: pocketsphinx-python # command to install dependencies install: + - rm -rf ${TMPDIR} + - mkdir ${TMPDIR} + - echo ${TMPDIR} - VIRTUALENV_ROOT=${VIRTUAL_ENV} ./dev_setup.sh - pip install -r requirements.txt - pip install -r test-requirements.txt diff --git a/mycroft/skills/msm_wrapper.py b/mycroft/skills/msm_wrapper.py new file mode 100644 index 0000000000..840f9b9531 --- /dev/null +++ b/mycroft/skills/msm_wrapper.py @@ -0,0 +1,43 @@ +# Copyright 2019 Mycroft AI Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +import os +from os.path import join, expanduser, exists + +from msm import MycroftSkillsManager, SkillRepo +from mycroft.util.combo_lock import ComboLock + +mycroft_msm_lock = ComboLock('/tmp/mycroft-msm.lck') + + +def create_msm(config): + """ Create msm object from config. """ + msm_config = config['skills']['msm'] + repo_config = msm_config['repo'] + data_dir = expanduser(config['data_dir']) + skills_dir = join(data_dir, msm_config['directory']) + repo_cache = join(data_dir, repo_config['cache']) + platform = config['enclosure'].get('platform', 'default') + + with mycroft_msm_lock: + # Try to create the skills directory if it doesn't exist + if not exists(skills_dir): + os.makedirs(skills_dir) + + return MycroftSkillsManager( + platform=platform, skills_dir=skills_dir, + repo=SkillRepo(repo_cache, repo_config['url'], + repo_config['branch']), + versioned=msm_config['versioned']) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index 7671d50ef1..35a89798cd 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -63,14 +63,73 @@ import json import hashlib import os import yaml +import time +import copy +import re from threading import Timer from os.path import isfile, join, expanduser -from requests.exceptions import RequestException, HTTPError +from requests.exceptions import RequestException +from msm import SkillEntry from mycroft.api import DeviceApi, is_paired from mycroft.util.log import LOG +from mycroft.util import camel_case_split from mycroft.configuration import ConfigurationManager +from .msm_wrapper import create_msm + +# This is the base needed for sending a blank settings meta entry (Tartarus) +# To this a global id is added +# TODO reduce the needed boilerplate here +BLANK_META = { + "skillMetadata": { + "sections": [ + { + "name": "", + "fields": [ + { + "type": "label", + "label": "" + } + ] + } + ] + } +} + + +msm = None +msm_creation_time = 0 + + +def build_global_id(directory, config): + """ Create global id for the skill. + + TODO: Handle dirty skill + + Arguments: + directory: skill directory + config: config for the device to fetch msm setup + """ + # Update the msm object if it's more than an hour old + global msm + global msm_creation_time + if msm is None or time.time() - msm_creation_time > 60 * 60: + msm_creation_time = time.time() + msm = create_msm(config) + + s = SkillEntry.from_folder(directory, msm) + if s.meta_info != {}: + return s.meta_info['skill_gid'], s.meta_info['display_name'] + else: # No skills meta data available, local or unsubmitted skill + return "@{}|{}".format(DeviceApi().identity.uuid, s.name), None + + +def display_name(name): + """ Splits camelcase and removes leading/trailing Skill. """ + name = re.sub(r'(^[Ss]kill|[Ss]kill$)', '', name) + return camel_case_split(name) + class DelayRequest(Exception): """ Indicate that the next request should be delayed. """ @@ -82,8 +141,10 @@ class SkillSettings(dict): also syncs to the backend for skill settings Args: - directory (str): Path to storage directory - name (str): user readable name associated with the settings + directory (str): Path to storage directory + name (str): user readable name associated with the settings + no_upload (bool): True if the upload to mycroft servers should be + disabled. """ def __init__(self, directory, name): @@ -100,6 +161,8 @@ class SkillSettings(dict): # set file paths self._settings_path = join(directory, 'settings.json') self._meta_path = _get_meta_path(directory) + self._directory = directory + self.is_alive = True self.loaded_hash = hash(json.dumps(self, sort_keys=True)) self._complete_intialization = False @@ -108,11 +171,17 @@ class SkillSettings(dict): self._user_identity = None self.changed_callback = None self._poll_timer = None + self._blank_poll_timer = None self._is_alive = True # if settingsmeta exist if self._meta_path: self._poll_skill_settings() + # if not disallowed by user upload an entry for all skills installed + elif self.config['skills']['upload_skill_manifest']: + self._blank_poll_timer = Timer(1, self._init_blank_meta) + self._blank_poll_timer.daemon = True + self._blank_poll_timer.start() def __hash__(self): """ Simple object unique hash. """ @@ -128,6 +197,8 @@ class SkillSettings(dict): self._is_alive = False if self._poll_timer: self._poll_timer.cancel() + if self._blank_poll_timer: + self._blank_poll_timer.cancel() def set_changed_callback(self, callback): """ Set callback to perform when server settings have changed. @@ -202,24 +273,43 @@ class SkillSettings(dict): return super(SkillSettings, self).__setitem__(key, value) def _load_settings_meta(self): - """ Loads settings metadata from skills path. """ - if not self._meta_path: - return None + """ Load settings metadata from the skill folder. - _, ext = os.path.splitext(self._meta_path) - json_file = True if ext.lower() == ".json" else False + If no settingsmeta exists a basic settingsmeta will be created + containing a basic identifier. - try: - with open(self._meta_path, encoding='utf-8') as f: - if json_file: - data = json.load(f) - else: - data = yaml.load(f) - return data - except Exception as e: - LOG.error("Failed to load setting file: " + self._meta_path) - LOG.error(repr(e)) - return None + Returns: + (dict) settings meta + """ + if self._meta_path and os.path.isfile(self._meta_path): + _, ext = os.path.splitext(self._meta_path) + json_file = True if ext.lower() == ".json" else False + + try: + with open(self._meta_path, encoding='utf-8') as f: + if json_file: + data = json.load(f) + else: + data = yaml.load(f) + except Exception as e: + LOG.error("Failed to load setting file: " + self._meta_path) + LOG.error(repr(e)) + data = copy.copy(BLANK_META) + else: + data = copy.copy(BLANK_META) + + # Add Information extracted from the skills-meta.json entry for the + # skill. + skill_gid, disp_name = build_global_id(self._directory, self.config) + data['skill_gid'] = skill_gid + data['display_name'] = (disp_name or data.get('name') or + display_name(self.name)) + + # Backwards compatibility: + if 'name' not in data: + data['name'] = data['display_name'] + + return data def _send_settings_meta(self, settings_meta): """ Send settingsmeta to the server. @@ -399,6 +489,18 @@ class SkillSettings(dict): settings_meta = self._load_settings_meta() self._upload_meta(settings_meta, hashed_meta) + def _init_blank_meta(self): + """ Send blank settingsmeta to remote. """ + try: + if not is_paired() and self.is_alive: + self._blank_poll_timer = Timer(60, self._init_blank_meta) + self._blank_poll_timer.daemon = True + self._blank_poll_timer.start() + else: + self.initialize_remote_settings() + except Exception as e: + LOG.exception('Failed to send blank meta: {}'.format(repr(e))) + def _poll_skill_settings(self): """ If identifier exists for this skill poll to backend to request settings and store it if it changes diff --git a/mycroft/skills/skill_manager.py b/mycroft/skills/skill_manager.py index a4989abe3b..1f9ec2b41c 100644 --- a/mycroft/skills/skill_manager.py +++ b/mycroft/skills/skill_manager.py @@ -33,7 +33,7 @@ from mycroft.util.log import LOG from mycroft.api import DeviceApi, is_paired from .core import load_skill, create_skill_descriptor, MainModule - +from .msm_wrapper import create_msm as msm_creator DEBUG = Configuration.get().get("debug", False) skills_config = Configuration.get().get("skills") @@ -132,23 +132,7 @@ class SkillManager(Thread): @staticmethod def create_msm(): - config = Configuration.get() - msm_config = config['skills']['msm'] - repo_config = msm_config['repo'] - data_dir = expanduser(config['data_dir']) - skills_dir = join(data_dir, msm_config['directory']) - # Try to create the skills directory if it doesn't exist - if not exists(skills_dir): - os.makedirs(skills_dir) - - repo_cache = join(data_dir, repo_config['cache']) - platform = config['enclosure'].get('platform', 'default') - return MycroftSkillsManager( - platform=platform, skills_dir=skills_dir, - repo=SkillRepo( - repo_cache, repo_config['url'], repo_config['branch'] - ), versioned=msm_config['versioned'] - ) + return msm_creator(Configuration.get()) def schedule_now(self, message=None): self.next_download = time.time() - 1 diff --git a/requirements.txt b/requirements.txt index 459c55c3be..31bdc05061 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,7 +23,7 @@ google-api-python-client==1.6.4 fasteners==0.14.1 PyYAML==3.13 -msm==0.7.3 +msm==0.7.4 msk==0.3.12 adapt-parser==0.3.2 padatious==0.4.6 diff --git a/test/unittests/skills/test_settings.py b/test/unittests/skills/test_settings.py index 152ddcacf0..6a34fb5f0b 100644 --- a/test/unittests/skills/test_settings.py +++ b/test/unittests/skills/test_settings.py @@ -14,6 +14,7 @@ # import json import unittest +from mock import MagicMock from os import remove from os.path import join, dirname @@ -21,6 +22,10 @@ from os.path import join, dirname from mycroft.skills.settings import SkillSettings +SkillSettings._poll_skill_settings = MagicMock() +SkillSettings._init_blank_meta = MagicMock() + + class SkillSettingsTest(unittest.TestCase): def setUp(self): try: diff --git a/test/unittests/skills/test_skill_manager.py b/test/unittests/skills/test_skill_manager.py index 60604a590a..a975ff7ee0 100644 --- a/test/unittests/skills/test_skill_manager.py +++ b/test/unittests/skills/test_skill_manager.py @@ -1,5 +1,6 @@ import unittest import mock +import copy import tempfile from os.path import exists, join from shutil import rmtree @@ -9,7 +10,6 @@ from mycroft.configuration import Configuration from mycroft.skills.skill_manager import SkillManager BASE_CONF = base_config() -BASE_CONF['data_dir'] = tempfile.mkdtemp() BASE_CONF['skills'] = { 'msm': { 'directory': 'skills', @@ -17,7 +17,7 @@ BASE_CONF['skills'] = { 'repo': { 'cache': '.skills-repo', 'url': 'https://github.com/MycroftAI/mycroft-skills', - 'branch': '18.08' + 'branch': '19.02' } }, 'update_interval': 3600, @@ -56,14 +56,16 @@ class MycroftSkillTest(unittest.TestCase): self.emitter.reset() self.temp_dir = tempfile.mkdtemp() - @mock.patch.dict(Configuration._Configuration__config, BASE_CONF) def test_create_manager(self): """ Verify that the skill manager and msm loads as expected and that the skills dir is created as needed. """ - SkillManager(self.emitter) - self.assertTrue(exists(join(BASE_CONF['data_dir'], 'skills'))) + conf = copy.deepcopy(BASE_CONF) + conf['data_dir'] = self.temp_dir + with mock.patch.dict(Configuration._Configuration__config, + BASE_CONF): + SkillManager(self.emitter) + self.assertTrue(exists(join(BASE_CONF['data_dir'], 'skills'))) - @classmethod - def tearDownClass(cls): - rmtree(BASE_CONF['data_dir']) + def tearDown(self): + rmtree(self.temp_dir) diff --git a/test/unittests/util/test_signal.py b/test/unittests/util/test_signal.py index 9db3c98cd2..93a142ee7d 100644 --- a/test/unittests/util/test_signal.py +++ b/test/unittests/util/test_signal.py @@ -15,23 +15,25 @@ import unittest from shutil import rmtree -from os.path import exists, isfile +from os.path import exists, isfile, join +from tempfile import gettempdir from mycroft.util import create_signal, check_for_signal class TestSignals(unittest.TestCase): def setUp(self): - if exists('/tmp/mycroft'): - rmtree('/tmp/mycroft') + if exists(join(gettempdir(), 'mycroft')): + rmtree(join(gettempdir(), 'mycroft')) def test_create_signal(self): create_signal('test_signal') - self.assertTrue(isfile('/tmp/mycroft/ipc/signal/test_signal')) + self.assertTrue(isfile(join(gettempdir(), + 'mycroft/ipc/signal/test_signal'))) def test_check_signal(self): - if exists('/tmp/mycroft'): - rmtree('/tmp/mycroft') + if exists(join(gettempdir(), 'mycroft')): + rmtree(join(gettempdir(), 'mycroft')) # check that signal is not found if file does not exist self.assertFalse(check_for_signal('test_signal')) @@ -39,7 +41,8 @@ class TestSignals(unittest.TestCase): create_signal('test_signal') self.assertTrue(check_for_signal('test_signal')) # Check that the signal is removed after use - self.assertFalse(isfile('/tmp/mycroft/ipc/signal/test_signal')) + self.assertFalse(isfile(join(gettempdir(), + 'mycroft/ipc/signal/test_signal'))) if __name__ == "__main__": From 693bf2c699e70761d637696923bb05175f9ee054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Mon, 8 Apr 2019 13:59:59 +0200 Subject: [PATCH 02/10] Remove BLANK boilerplate content --- mycroft/skills/settings.py | 53 ++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index 35a89798cd..a2dea10e7d 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -78,25 +78,6 @@ from mycroft.configuration import ConfigurationManager from .msm_wrapper import create_msm -# This is the base needed for sending a blank settings meta entry (Tartarus) -# To this a global id is added -# TODO reduce the needed boilerplate here -BLANK_META = { - "skillMetadata": { - "sections": [ - { - "name": "", - "fields": [ - { - "type": "label", - "label": "" - } - ] - } - ] - } -} - msm = None msm_creation_time = 0 @@ -174,6 +155,12 @@ class SkillSettings(dict): self._blank_poll_timer = None self._is_alive = True + # Add Information extracted from the skills-meta.json entry for the + # skill. + skill_gid, disp_name = build_global_id(self._directory, self.config) + self.skill_gid = skill_gid + self.display_name = disp_name + # if settingsmeta exist if self._meta_path: self._poll_skill_settings() @@ -296,13 +283,11 @@ class SkillSettings(dict): LOG.error(repr(e)) data = copy.copy(BLANK_META) else: - data = copy.copy(BLANK_META) + data = {} - # Add Information extracted from the skills-meta.json entry for the - # skill. - skill_gid, disp_name = build_global_id(self._directory, self.config) - data['skill_gid'] = skill_gid - data['display_name'] = (disp_name or data.get('name') or + # Insert skill_gid and display_name + data['skill_gid'] = self.skill_gid + data['display_name'] = (self.display_name or data.get('name') or display_name(self.name)) # Backwards compatibility: @@ -341,12 +326,13 @@ class SkillSettings(dict): if self._is_new_hash(skill_settings['identifier']): self._save_uuid(skill_settings['uuid']) self._save_hash(skill_settings['identifier']) - sections = skill_settings['skillMetadata']['sections'] - for section in sections: - for field in section["fields"]: - if "name" in field and "value" in field: - self[field['name']] = field['value'] - self.store() + if 'skillMetadata' in skill_settings: + sections = skill_settings['skillMetadata']['sections'] + for section in sections: + for field in section["fields"]: + if "name" in field and "value" in field: + self[field['name']] = field['value'] + self.store() def _load_uuid(self): """ Loads uuid @@ -393,6 +379,8 @@ class SkillSettings(dict): def _migrate_settings(self, settings_meta): """ sync settings.json and settingsmeta in memory """ meta = settings_meta.copy() + if 'skillMetadata' not in meta: + return meta self.load_skill_settings_from_file() sections = meta['skillMetadata']['sections'] for i, section in enumerate(sections): @@ -568,6 +556,9 @@ class SkillSettings(dict): dict: skills object """ meta = settings_meta.copy() + if 'skillMetadata' not in settings_meta: + return meta + sections = meta['skillMetadata']['sections'] for i, section in enumerate(sections): From f6347ae47c872b40339d9565a9cb29da5bca8716 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Fri, 12 Apr 2019 18:06:03 +0200 Subject: [PATCH 03/10] Replace hashed meta with skill_gid as identifier This also removes the notion of an owner skill and all skills may update settings on the server. --- mycroft/skills/settings.py | 145 ++++++------------------------------- 1 file changed, 21 insertions(+), 124 deletions(-) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index a2dea10e7d..a323b6d49f 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -216,34 +216,12 @@ class SkillSettings(dict): except RequestException: return - hashed_meta = self._get_meta_hash(settings_meta) - skill_settings = self._request_other_settings(hashed_meta) - # if hash is new then there is a diff version of settingsmeta - if self._is_new_hash(hashed_meta): - # first look at all other devices on user account to see - # if the settings exist. if it does then sync with device - if skill_settings: - # not_owner flags that this settings is loaded from - # another device. If a skill settings doesn't have - # not_owner, then the skill is created from that device - self['not_owner'] = True - self.save_skill_settings(skill_settings) - else: # upload skill settings if - uuid = self._load_uuid() - if uuid is not None: - self._delete_metadata(uuid) - self._upload_meta(settings_meta, hashed_meta) - else: # hash is not new - if skill_settings is not None: - self['not_owner'] = True - self.save_skill_settings(skill_settings) - else: - settings = self._request_my_settings(hashed_meta) - if settings is None: - # metadata got deleted from Home, send up - self._upload_meta(settings_meta, hashed_meta) - else: - self.save_skill_settings(settings) + settings = self._request_my_settings(self.skill_gid) + if settings is None: + # metadata got deleted from Home, send up + self._upload_meta(settings_meta, self.skill_gid) + else: + self.save_skill_settings(settings) self._complete_intialization = True @property @@ -323,15 +301,15 @@ class SkillSettings(dict): Args: skill_settings (dict): skill """ - if self._is_new_hash(skill_settings['identifier']): - self._save_uuid(skill_settings['uuid']) - self._save_hash(skill_settings['identifier']) if 'skillMetadata' in skill_settings: sections = skill_settings['skillMetadata']['sections'] for section in sections: for field in section["fields"]: if "name" in field and "value" in field: - self[field['name']] = field['value'] + # Bypass the change lock to allow server to update + # during skill init + super(SkillSettings, self).__setitem__(field['name'], + field['value']) self.store() def _load_uuid(self): @@ -392,90 +370,33 @@ class SkillSettings(dict): meta['skillMetadata']['sections'] = sections return meta - def _upload_meta(self, settings_meta, hashed_meta): + def _upload_meta(self, settings_meta, identifier): """ uploads the new meta data to settings with settings migration Args: - settings_meta (dict): from settingsmeta.json or settingsmeta.yaml - hashed_meta (str): {skill-folder}-settinsmeta.json + settings_meta (dict): settingsmeta.json or settingsmeta.yaml + identifier (str): identifier for skills meta data """ meta = self._migrate_settings(settings_meta) - meta['identifier'] = str(hashed_meta) + meta['identifier'] = identifier response = self._send_settings_meta(meta) - if response and 'uuid' in response: - self._save_uuid(response['uuid']) - if 'not_owner' in self: - del self['not_owner'] - self._save_hash(hashed_meta) def hash(self, string): """ md5 hasher for consistency across cpu architectures """ return hashlib.md5(bytes(string, 'utf-8')).hexdigest() - def _get_meta_hash(self, settings_meta): - """ Gets the hash of skill - - Args: - settings_meta (dict): settingsmeta object - Returns: - _hash (str): hashed to identify skills - """ - _hash = self.hash(json.dumps(settings_meta, sort_keys=True) + - self._user_identity) - return "{}--{}".format(self.name, _hash) - - def _save_hash(self, hashed_meta): - """ Saves hashed_meta to settings directory. - - Args: - hashed_meta (str): hash of new settingsmeta - """ - directory = self.config.get("skills")["directory"] - directory = join(directory, self.name) - directory = expanduser(directory) - hash_file = join(directory, 'hash') - os.makedirs(directory, exist_ok=True) - with open(hash_file, 'w') as f: - f.write(hashed_meta) - - def _is_new_hash(self, hashed_meta): - """ Check if stored hash is the same as current. - - If the hashed file does not exist, usually in the - case of first load, then the create it and return True - - Args: - hashed_meta (str): hash of metadata and uuid of device - Returns: - bool: True if hash is new, otherwise False - """ - directory = self.config.get("skills")["directory"] - directory = join(directory, self.name) - directory = expanduser(directory) - hash_file = join(directory, 'hash') - if isfile(hash_file): - with open(hash_file, 'r') as f: - current_hash = f.read() - return False if current_hash == str(hashed_meta) else True - return True - def update_remote(self): """ update settings state from server """ - skills_settings = None settings_meta = self._load_settings_meta() if settings_meta is None: return - hashed_meta = self._get_meta_hash(settings_meta) - if self.get('not_owner'): - skills_settings = self._request_other_settings(hashed_meta) - if not skills_settings: - skills_settings = self._request_my_settings(hashed_meta) + skills_settings = self._request_my_settings(self.skill_gid) if skills_settings is not None: self.save_skill_settings(skills_settings) self.store() else: settings_meta = self._load_settings_meta() - self._upload_meta(settings_meta, hashed_meta) + self._upload_meta(settings_meta, self.skill_gid) def _init_blank_meta(self): """ Send blank settingsmeta to remote. """ @@ -599,10 +520,11 @@ class SkillSettings(dict): with the identifier Args: - identifier (str): a hashed_meta + identifier (str): identifier (skill_gid) Returns: skill_settings (dict or None): returns a dict if matches """ + print("GETTING SETTINGS FOR {}".format(self.name)) settings = self._request_settings() if settings: # this loads the settings into memory for use in self.store @@ -612,7 +534,8 @@ class SkillSettings(dict): self._type_cast(skill_settings, to_platform='core') self._remote_settings = skill_settings return skill_settings - return None + else: + return None def _request_settings(self): """ Get all skill settings for this device from server. @@ -631,27 +554,6 @@ class SkillSettings(dict): settings = [skills for skills in settings if skills is not None] return settings - def _request_other_settings(self, identifier): - """ Retrieve skill settings from other devices by identifier - - Args: - identifier (str): identifier for this skill - Returns: - settings (dict or None): the retrieved settings or None - """ - path = \ - "/" + self._device_identity + "/userSkill?identifier=" + identifier - try: - user_skill = self.api.request({"method": "GET", "path": path}) - except RequestException: - # Some kind of Timeout, connection HTTPError, etc. - user_skill = None - if not user_skill: - return None - else: - settings = self._type_cast(user_skill[0], to_platform='core') - return settings - def _put_metadata(self, settings_meta): """ PUT settingsmeta to backend to be configured in server. used in place of POST and PATCH. @@ -717,12 +619,7 @@ class SkillSettings(dict): if self._should_upload_from_change: settings_meta = self._load_settings_meta() - hashed_meta = self._get_meta_hash(settings_meta) - uuid = self._load_uuid() - if uuid is not None: - self._delete_metadata(uuid) - self._upload_meta(settings_meta, hashed_meta) - + self._upload_meta(settings_meta, self.skill_gid) def _get_meta_path(base_directory): json_path = join(base_directory, 'settingsmeta.json') From 93a0b31b4ea01a0ada8f723005af004c8fde6803 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Tue, 23 Apr 2019 15:45:35 +0200 Subject: [PATCH 04/10] Switch endpoint for fetching settings use the userSkill endpoint instead of skill endpoint to always be able to get settings no matter who the owner is. --- mycroft/skills/settings.py | 73 ++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index a323b6d49f..4132981788 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -68,7 +68,7 @@ import copy import re from threading import Timer from os.path import isfile, join, expanduser -from requests.exceptions import RequestException +from requests.exceptions import RequestException, HTTPError from msm import SkillEntry from mycroft.api import DeviceApi, is_paired @@ -154,6 +154,7 @@ class SkillSettings(dict): self._poll_timer = None self._blank_poll_timer = None self._is_alive = True + self._meta_upload = True # Flag allowing upload of settings meta # Add Information extracted from the skills-meta.json entry for the # skill. @@ -216,12 +217,13 @@ class SkillSettings(dict): except RequestException: return - settings = self._request_my_settings(self.skill_gid) - if settings is None: - # metadata got deleted from Home, send up - self._upload_meta(settings_meta, self.skill_gid) - else: + settings = self._request_other_settings(self.skill_gid) + if settings: self.save_skill_settings(settings) + + # Always try to upload settingsmeta on startup + self._upload_meta(settings_meta, self.skill_gid) + self._complete_intialization = True @property @@ -282,18 +284,21 @@ class SkillSettings(dict): Returns: dict: uuid, a unique id for the setting meta data """ - try: - uuid = self._put_metadata(settings_meta) - return uuid - except HTTPError as e: - if e.response.status_code in [422, 500, 501]: - raise DelayRequest - else: + if self._meta_upload: + try: + uuid = self._put_metadata(settings_meta) + return uuid + except HTTPError as e: + if e.response.status_code in [422, 500, 501]: + self._meta_upload = False + raise DelayRequest + else: + LOG.error(e) + return None + + except Exception as e: LOG.error(e) return None - except Exception as e: - LOG.error(e) - return None def save_skill_settings(self, skill_settings): """ Takes skill object and save onto self @@ -390,11 +395,12 @@ class SkillSettings(dict): settings_meta = self._load_settings_meta() if settings_meta is None: return - skills_settings = self._request_my_settings(self.skill_gid) + skills_settings = self._request_other_settings(self.skill_gid) if skills_settings is not None: self.save_skill_settings(skills_settings) self.store() else: + # Settings meta doesn't exist on server push them settings_meta = self._load_settings_meta() self._upload_meta(settings_meta, self.skill_gid) @@ -515,27 +521,25 @@ class SkillSettings(dict): meta['skillMetadata']['sections'] = sections return meta - def _request_my_settings(self, identifier): - """ Get skill settings for this device associated - with the identifier - + def _request_other_settings(self, identifier): + """ Retrieve skill settings from other devices by identifier Args: - identifier (str): identifier (skill_gid) + identifier (str): identifier for this skill Returns: - skill_settings (dict or None): returns a dict if matches + settings (dict or None): the retrieved settings or None """ - print("GETTING SETTINGS FOR {}".format(self.name)) - settings = self._request_settings() - if settings: - # this loads the settings into memory for use in self.store - for skill_settings in settings: - if skill_settings['identifier'] == identifier: - skill_settings = \ - self._type_cast(skill_settings, to_platform='core') - self._remote_settings = skill_settings - return skill_settings - else: + path = \ + "/" + self._device_identity + "/userSkill?identifier=" + identifier + try: + user_skill = self.api.request({"method": "GET", "path": path}) + except RequestException: + # Some kind of Timeout, connection HTTPError, etc. + user_skill = None + if not user_skill or not user_skill[0]: return None + else: + settings = self._type_cast(user_skill[0], to_platform='core') + return settings def _request_settings(self): """ Get all skill settings for this device from server. @@ -621,6 +625,7 @@ class SkillSettings(dict): settings_meta = self._load_settings_meta() self._upload_meta(settings_meta, self.skill_gid) + def _get_meta_path(base_directory): json_path = join(base_directory, 'settingsmeta.json') yaml_path = join(base_directory, 'settingsmeta.yaml') From 4a75526de797ab74ffa27572573028e7347f20f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Wed, 24 Apr 2019 08:24:20 +0200 Subject: [PATCH 05/10] Check both endpoints for settings --- mycroft/skills/settings.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index 4132981788..4a77472ad5 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -395,7 +395,9 @@ class SkillSettings(dict): settings_meta = self._load_settings_meta() if settings_meta is None: return - skills_settings = self._request_other_settings(self.skill_gid) + skills_settings = (self._request_other_settings(self.skill_gid) or + self._request_my_settings(self.skill_gid)) + if skills_settings is not None: self.save_skill_settings(skills_settings) self.store() @@ -521,6 +523,25 @@ class SkillSettings(dict): meta['skillMetadata']['sections'] = sections return meta + def _request_my_settings(self, identifier): + """ Get skill settings for this device associated + with the identifier + Args: + identifier (str): a hashed_meta + Returns: + skill_settings (dict or None): returns a dict if matches + """ + settings = self._request_settings() + if settings: + # this loads the settings into memory for use in self.store + for skill_settings in settings: + if skill_settings['identifier'] == identifier: + skill_settings = \ + self._type_cast(skill_settings, to_platform='core') + self._remote_settings = skill_settings + return skill_settings + return None + def _request_other_settings(self, identifier): """ Retrieve skill settings from other devices by identifier Args: From 6eb2aefbaace1a2ed27b378a0f07199478c904be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Wed, 24 Apr 2019 13:57:24 +0200 Subject: [PATCH 06/10] Delete remote settings before pushing - restore storing / loading uuid - use uuid to delete before pushing settingsmeta --- mycroft/skills/settings.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index 4a77472ad5..f695962d8b 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -217,11 +217,16 @@ class SkillSettings(dict): except RequestException: return - settings = self._request_other_settings(self.skill_gid) + settings = (self._request_my_settings(self.skill_gid) or + self._request_other_settings(self.skill_gid)) if settings: self.save_skill_settings(settings) # Always try to upload settingsmeta on startup + uuid = self._load_uuid() + if uuid is not None: + self._delete_metadata(uuid) + self._upload_meta(settings_meta, self.skill_gid) self._complete_intialization = True @@ -306,6 +311,7 @@ class SkillSettings(dict): Args: skill_settings (dict): skill """ + self._save_uuid(skill_settings['uuid']) if 'skillMetadata' in skill_settings: sections = skill_settings['skillMetadata']['sections'] for section in sections: @@ -385,6 +391,10 @@ class SkillSettings(dict): meta = self._migrate_settings(settings_meta) meta['identifier'] = identifier response = self._send_settings_meta(meta) + if response and 'uuid' in response: + self._save_uuid(response['uuid']) + if 'not_owner' in self: + del self['not_owner'] def hash(self, string): """ md5 hasher for consistency across cpu architectures """ @@ -395,12 +405,12 @@ class SkillSettings(dict): settings_meta = self._load_settings_meta() if settings_meta is None: return - skills_settings = (self._request_other_settings(self.skill_gid) or - self._request_my_settings(self.skill_gid)) + # Get settings + skills_settings = (self._request_my_settings(self.skill_gid) or + self._request_other_settings(self.skill_gid)) if skills_settings is not None: self.save_skill_settings(skills_settings) - self.store() else: # Settings meta doesn't exist on server push them settings_meta = self._load_settings_meta() @@ -644,6 +654,9 @@ class SkillSettings(dict): if self._should_upload_from_change: settings_meta = self._load_settings_meta() + uuid = self._load_uuid() + if uuid is not None: + self._delete_metadata(uuid) self._upload_meta(settings_meta, self.skill_gid) From 4518a1152871bae3343cf8e65dc25df9be6d4239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Wed, 24 Apr 2019 14:27:09 +0200 Subject: [PATCH 07/10] Add safety around skillMetadata skillMetadata could be accessed in settings without it when checking if remote update was needed. --- mycroft/skills/settings.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index f695962d8b..dd696227d5 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -311,7 +311,8 @@ class SkillSettings(dict): Args: skill_settings (dict): skill """ - self._save_uuid(skill_settings['uuid']) + if 'uuid' in skill_settings: + self._save_uuid(skill_settings['uuid']) if 'skillMetadata' in skill_settings: sections = skill_settings['skillMetadata']['sections'] for section in sections: @@ -624,7 +625,8 @@ class SkillSettings(dict): @property def _should_upload_from_change(self): changed = False - if hasattr(self, '_remote_settings'): + if (hasattr(self, '_remote_settings') and + 'skillMetadata' in self._remote_settings): sections = self._remote_settings['skillMetadata']['sections'] for i, section in enumerate(sections): for j, field in enumerate(section['fields']): From 2208ee178c4cc6bc32701f8678c486851d7e1dce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Mon, 20 May 2019 16:28:43 +0200 Subject: [PATCH 08/10] Switch to getting skill_gid from msm skill entry --- mycroft/skills/settings.py | 20 +++++++++++++++----- requirements.txt | 2 +- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index dd696227d5..74706ad799 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -100,10 +100,8 @@ def build_global_id(directory, config): msm = create_msm(config) s = SkillEntry.from_folder(directory, msm) - if s.meta_info != {}: - return s.meta_info['skill_gid'], s.meta_info['display_name'] - else: # No skills meta data available, local or unsubmitted skill - return "@{}|{}".format(DeviceApi().identity.uuid, s.name), None + # If modified prepend the device uuid + return s.skill_gid, s.meta_info.get('display_name') def display_name(name): @@ -159,7 +157,7 @@ class SkillSettings(dict): # Add Information extracted from the skills-meta.json entry for the # skill. skill_gid, disp_name = build_global_id(self._directory, self.config) - self.skill_gid = skill_gid + self.__skill_gid = skill_gid self.display_name = disp_name # if settingsmeta exist @@ -171,6 +169,15 @@ class SkillSettings(dict): self._blank_poll_timer.daemon = True self._blank_poll_timer.start() + @property + def skill_gid(self): + """ Finalizes the skill gid to include device uuid if needed. """ + if is_paired(): + return self.__skill_gid.replace('@|', '@{}|'.format( + DeviceApi().identity.uuid)) + else: + return self.__skill_gid + def __hash__(self): """ Simple object unique hash. """ return hash(str(id(self)) + self.name) @@ -389,6 +396,7 @@ class SkillSettings(dict): settings_meta (dict): settingsmeta.json or settingsmeta.yaml identifier (str): identifier for skills meta data """ + LOG.debug('Uploading settings meta for {}'.format(identifier)) meta = self._migrate_settings(settings_meta) meta['identifier'] = identifier response = self._send_settings_meta(meta) @@ -413,6 +421,7 @@ class SkillSettings(dict): if skills_settings is not None: self.save_skill_settings(skills_settings) else: + LOG.debug("No Settings on server for {}".format(self.skill_gid)) # Settings meta doesn't exist on server push them settings_meta = self._load_settings_meta() self._upload_meta(settings_meta, self.skill_gid) @@ -547,6 +556,7 @@ class SkillSettings(dict): # this loads the settings into memory for use in self.store for skill_settings in settings: if skill_settings['identifier'] == identifier: + LOG.debug("Fetched settings for {}".format(identifier)) skill_settings = \ self._type_cast(skill_settings, to_platform='core') self._remote_settings = skill_settings diff --git a/requirements.txt b/requirements.txt index 31bdc05061..66061d4bda 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,7 +23,7 @@ google-api-python-client==1.6.4 fasteners==0.14.1 PyYAML==3.13 -msm==0.7.4 +msm==0.7.5 msk==0.3.12 adapt-parser==0.3.2 padatious==0.4.6 From c3ac9d8ca71ad28507d3b481b33df6353bcceaf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Tue, 21 May 2019 17:21:45 +0200 Subject: [PATCH 09/10] Remove the delete metadata calls when updating settings The calls are not implemented and functionality will likely change. --- mycroft/skills/settings.py | 59 +++----------------------------------- 1 file changed, 4 insertions(+), 55 deletions(-) diff --git a/mycroft/skills/settings.py b/mycroft/skills/settings.py index 74706ad799..e0e4730692 100644 --- a/mycroft/skills/settings.py +++ b/mycroft/skills/settings.py @@ -229,11 +229,10 @@ class SkillSettings(dict): if settings: self.save_skill_settings(settings) - # Always try to upload settingsmeta on startup - uuid = self._load_uuid() - if uuid is not None: - self._delete_metadata(uuid) + # TODO if this skill_gid is not a modified version check if a modified + # version exists on the server and delete it + # Always try to upload settingsmeta on startup self._upload_meta(settings_meta, self.skill_gid) self._complete_intialization = True @@ -318,8 +317,6 @@ class SkillSettings(dict): Args: skill_settings (dict): skill """ - if 'uuid' in skill_settings: - self._save_uuid(skill_settings['uuid']) if 'skillMetadata' in skill_settings: sections = skill_settings['skillMetadata']['sections'] for section in sections: @@ -331,48 +328,6 @@ class SkillSettings(dict): field['value']) self.store() - def _load_uuid(self): - """ Loads uuid - - Returns: - str: uuid of the previous settingsmeta - """ - directory = self.config.get("skills")["directory"] - directory = join(directory, self.name) - directory = expanduser(directory) - uuid_file = join(directory, 'uuid') - uuid = None - if isfile(uuid_file): - with open(uuid_file, 'r') as f: - uuid = f.read() - return uuid - - def _save_uuid(self, uuid): - """ Saves uuid. - - Args: - uuid (str): uuid, unique id of new settingsmeta - """ - directory = self.config.get("skills")["directory"] - directory = join(directory, self.name) - directory = expanduser(directory) - uuid_file = join(directory, 'uuid') - os.makedirs(directory, exist_ok=True) - with open(uuid_file, 'w') as f: - f.write(str(uuid)) - - def _uuid_exist(self): - """ Checks if there is an uuid file. - - Returns: - bool: True if uuid file exist False otherwise - """ - directory = self.config.get("skills")["directory"] - directory = join(directory, self.name) - directory = expanduser(directory) - uuid_file = join(directory, 'uuid') - return isfile(uuid_file) - def _migrate_settings(self, settings_meta): """ sync settings.json and settingsmeta in memory """ meta = settings_meta.copy() @@ -400,10 +355,6 @@ class SkillSettings(dict): meta = self._migrate_settings(settings_meta) meta['identifier'] = identifier response = self._send_settings_meta(meta) - if response and 'uuid' in response: - self._save_uuid(response['uuid']) - if 'not_owner' in self: - del self['not_owner'] def hash(self, string): """ md5 hasher for consistency across cpu architectures """ @@ -617,6 +568,7 @@ class SkillSettings(dict): def _delete_metadata(self, uuid): """ Delete the current skill metadata + TODO: UPDATE FOR NEW BACKEND Args: uuid (str): unique id of the skill """ @@ -666,9 +618,6 @@ class SkillSettings(dict): if self._should_upload_from_change: settings_meta = self._load_settings_meta() - uuid = self._load_uuid() - if uuid is not None: - self._delete_metadata(uuid) self._upload_meta(settings_meta, self.skill_gid) From 716c16a02749cc6d437f9542ac3517c655619e2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Tue, 21 May 2019 17:47:12 +0200 Subject: [PATCH 10/10] Finalize skill_gid in skills manifest. Before sending the skills manifest to the backend attach device uuid as needed. --- mycroft/api/__init__.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/mycroft/api/__init__.py b/mycroft/api/__init__.py index a0c219289c..0907bb479e 100644 --- a/mycroft/api/__init__.py +++ b/mycroft/api/__init__.py @@ -371,6 +371,11 @@ class DeviceApi(Api): skills = {s['name']: s for s in data['skills']} to_send['skills'] = [skills[key] for key in skills] + # Finalize skill_gid with uuid if needed + for s in to_send['skills']: + s['skill_gid'] = s.get('skill_gid', '').replace( + '@|', '@{}|'.format(self.identity.uuid)) + self.request({ "method": "PUT", "path": "/" + self.identity.uuid + "/skillJson",