Add handling of the skills.json file

[forslund]

This implements the basic features for syncing the skills.json file with the server no matter the installation source

• Initial upgrade of existing/nonexisting skills.json
• Curating if skills are modified without msm noticing
• Install now takes an origin argument depending on the source of the installation
• Update will update the updated field
• locking of the skills.json file

[MatthewScholefield]

This looks like a good idea, but why specifically this is needed? (In what case does the skills.json get corrupted). If the power got pulled while msm was doing anything would the lock cause msm to fail on subsequent boots?

[forslund]

My reasoning is that some of the skill operations are rather slow and now they can occur at any time. If 2 simultaneous installations occurs one could easily be overwritten.

The lock file exists in /tmp/ which should in most cases be reset completely. Also as far as I've been able to read the lock status of fcntl locks are stored in memory and will be cleared on restart.

But I'll do a test later tonight to verify.

[forslund]

Ok, was trickier than i realized. fcntl-locks are cleared by the kernel when the holding process terminates so I had to set up a VM to do an actual hard reset and test. And there are no issues with locks staying locked after power cycle

[forslund]

I've updated this to handle more of the currently used fields and limit the number of disk writes to only do them on skills_data change.

[MatthewScholefield]

Just an idea, could be cleaner via:

entry.update(installed=time.time(), installation='installed', status='activate')

or

entry.update(
    installed=time.time(),
    installation='installed',
    status='activate'
)

What do you think?

[forslund]

That does look nice.

[MatthewScholefield]

Requiring this after any api call breaks the python api (used by the installer skill for example). I think we should either make it clear msm's python api shouldn't be used (and change the installer skill) or do something else like add a @save_skills_data decorator that functions are marked with in the MycroftSkillsManager class.

[forslund]

Since I changed the Api i updated the minor version :) But you do have a point. (And I have made the corresponding changes in the core and installer skill.

I like the @save_skills_data decorator the case I don't really know how to handle is if the MycroftSkillsManager.install is passed to the apply-method. Is there a good way to work around that?

Basically strip the decorator if it exists when running the apply.

[forslund]

I could maybe add an optional keyword parameter to the decorator that would disable the saving. In this case I could also handle the locking automatically...
Or set a flag on the function called or something.

[MatthewScholefield]

While not particularly elegant, the decorator could do something like:

def save_skills_data(func):
    @wraps(func)
    def wrapper(self, *args, **kwargs):
        will_save = False
        if not self.saving_handled:
            will_save = self.saving_handled = True
        return_val = func(self, *args, **kwargs)
        if will_save:
            self.save()
            self.saving_handled = False
        return return_val

While it seems a little hacky, I think it's better that way so that what might seem like a valid api call, MycroftSkillsManager().install('myskill'), remains indeed valid. That said, we are literally only using msm bindings in the installer skill and for like 2 lines in msk, so it's no big deal to do it this way.

[forslund]

I'll give it a go. It would be really nice if this was automagic like this.

[forslund]

Ok, done some testing and the cleanest way I've found is having the decorator adding an optional parameter and have the apply method to check for the decorator and send the disable parameter if it's there:

decorator:

def save_skills_data(func):
    @wraps(func)
    def func_wrapper(self, *args, **kwargs):
        # Check for __msm_write keyword parameter
        do_write =  kwargs.get('__msm_write', True)
        # Clean the write parameter if exists
        if '__msm_write' in kwargs:
            kwargs.pop('__msm_write')
        try:
            ret = func(self, *args, **kwargs)
        finally:
            if do_write:
                self.write_skills_data()
        return ret

    func_wrapper.save_skills_data = True
    return func_wrapper

And the apply:

def apply(self, func, skills):
        """Run a function on all skills in parallel"""

        def run_item(skill):
            try:
                if 'save_skills_data' in dir(func):
                    func(skill, __msm_write=False)
                else:
                    func(skill)
    [....]

Does this look sane?

[forslund]

Hmm, no. yours is better since mine won't work if the msm.install/update/etc is wrapped in a function...

[MatthewScholefield]

For now, this looks good!
Merging.