Read skills from XDG Base directory specification locations

[PureTryOut]

Description

This makes mycroft-core read skills from all locations as specified by the XDG Base Directory specification, so in /usr/share/mycroft/skills, /usr/local/share/mycroft/skills, and $XDG_DATA_HOME/.local/share/mycroft/skills if it exists and otherwise ~/.local/share/mycroft/skills.

This makes mycroft-core read skills from the XDG Base Directory location in the user's home folder, so $XDG_DATA_HOME/.local/share/mycroft/skills if it exists and otherwise ~/.local/share/mycroft/skills.

This requires MycroftAI/mycroft-skills-manager#90 for the installation of skills part.

How to test

1. Apply Install to and read skills from the XDG home directory mycroft-skills-manager#90 to msm
2. Install some skills
3. Start Mycroft and check if skills in the required directories are being loaded properly

Contributor license agreement signed?

CLA [x]

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[pep8speaks]

Hello @PureTryOut! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-06-02 05:58:07 UTC

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[devops-mycroft]

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

[krisgesling]

Was just glancing at this as we were looking at the PR for MSM and I wondered if we also need to update scripts/prepare-msm.sh which seems to also have the /opt/mycroft/skills path hardcoded

[PureTryOut]

I wondered if we also need to update scripts/prepare-msm.sh which seems to also have the /opt/mycroft/skills path hardcoded

Good call, yes it needs to be. Fixed.

EDIT: Actually never mind, this script isn't needed at all anymore. The new skill directories don't require root rights and will be made by the pyxdg dep if they don't exist yet, so no need preparing anything. I'll just remove the script.

[PureTryOut]

It seems to me that before a decision is made the default skills directory, a decision needs to be made if they should be stored at the user level or the system level.

As I said before, my personal preference is both at the same time. But since that is impossible for now, the user level.

If there are user-level instances of mycroft, where do the logs go? Right now they are in the system-level var/logs/mycroft directory.

That is handled by #2802. It assumes users (rather than devs) will use bin/mycroft-start and it'll save the logs to ~/.local/state/mycroft when $XDG_STATE_HOME is not set..

The various XDG things are split out over a total of 3 PR's: #2802, #2794 and this one, #2803. Each PR moves some stuff over to the user's home directory, but together no data is outside of $HOME anymore. No /opt/mycroft, and no /var/log/mycroft.

[PureTryOut]

This leads to another question... how do we make a virtual environment XDG compliant? On my laptop, my virtual environments are stored in ~/.local/share/virtualenvs.

So does that mean you want everything related to that virtual environment, so configs as well as data, to be stored inside that env @chrisveilleux? In that case exporting XDG_CONFIG_HOME=~/.local/share/virtualenvs, XDG_DATA_HOME=~/.local/share/virtualenvs and XDG_STATE_HOME=~/.local/share/virtualenvs before you launch MyCroft does the trick.

[PureTryOut]

I still don't understand the unit test failure 🤔

[krisgesling]

I'm not sure the test is actually using that temp_dir. It might be better to set the dot_msm_path on the class itself:

This seems to work:

    def test_update_download_time(self):
        """Test updating the next time a download will occur."""
        skill_updater = SkillUpdater(self.message_bus_mock)
        skill_updater.dot_msm_path = self.temp_dir.joinpath('.msm')
        skill_updater.dot_msm_path.touch()
        dot_msm_mtime_before = skill_updater.dot_msm_path.stat().st_mtime
        sleep(0.5)
        skill_updater._update_download_time()
        dot_msm_mtime_after = skill_updater.dot_msm_path.stat().st_mtime
        self.assertLess(dot_msm_mtime_before, dot_msm_mtime_after)

[PureTryOut]

You seem to be right. I didn't understand why the test was failing as I didn't touch it, but now I actually looked at it it seems dot_msm_path was never used by the skill updater. So updating the download time did something but definitely not to the dot_msm_path. Your patch makes the SkillUpdater however use the new path and so it works.

Now I just don't understand how this test ever worked before 🤔

Anyways, updated.

[PureTryOut]

And now our good friend VK is failing again 😠

[codecov-commenter]

Codecov Report

Merging #2803 (d1a6252) into dev (c039aeb) will increase coverage by 0.00%.
The diff coverage is 85.71%.

❗ Current head d1a6252 differs from pull request most recent head c5c44d2. Consider uploading reports for the commit c5c44d2 to get more accurate results

@@           Coverage Diff           @@
##              dev    #2803   +/-   ##
=======================================
  Coverage   52.80%   52.81%           
=======================================
  Files         123      123           
  Lines       11104    11106    +2     
=======================================
+ Hits         5864     5866    +2     
  Misses       5240     5240           

Impacted Files	Coverage Δ
mycroft/skills/mycroft_skill/mycroft_skill.py	74.69% <ø> (ø)
mycroft/skills/settings.py	85.84% <ø> (ø)
mycroft/skills/msm_wrapper.py	52.77% <50.00%> (+2.77%)	⬆️
mycroft/skills/skill_manager.py	78.49% <100.00%> (+0.14%)	⬆️
mycroft/skills/skill_updater.py	83.87% <100.00%> (ø)
mycroft/client/speech/listener.py	48.04% <0.00%> (-0.36%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c039aeb...c5c44d2. Read the comment docs.

[PureTryOut]

@krisgesling any clue why VK is being difficult again? For some reason it seems to fail while sending emails, but the log only mentions warnings and no actual errors.

This PR is the big second part of the XDG stuff and moves the last files away from /opt, so I'd say it's quite important 🤗
After this only #2802 is left to move the log files.

[krisgesling]

It looks like this line:

mycroft-core/test/integrationtests/voight_kampff/test_setup.py

Line 207 in c5c44d2

return MycroftSkillsManager(platform, skills_dir, repo)

should be:

return MycroftSkillsManager(platform, skills_dir=skills_dir, repo=repo)

Presuming it shouldn't be old_skills_dir?

[PureTryOut]

Ah, good find, you're right!

No it shouldn't be old_skills_dir, that's just for moving over legacy setups to the new directory, and not something Voight Kampff should care about (seeing it doesn't change behaviour of skills).

Hopefully it passes now, fingers crossed 🤞

[PureTryOut]

Hopefully it passes now, fingers crossed crossed_fingers

Nope... Seems the volume skill still failed if I read it right?

[krisgesling]

Hmm, if it's volume then #2992 might fix it. I'm at pycon today, but ll try have another look in a break.

[krisgesling]

I've just been reading back through the thread and wanted to pull out the questions to make sure we cover everything.

1. User level vs system level?

The XDG changes as a whole will (by default) move everything out of the system level to the user level.

2. Directory to use for Skills

There still seems to be concern about using a "data" directory for Skills. Unfortunately there aren't a large number of options to choose from. I think the Arch wiki provides a cleaner overview of the XDG directories, but there's also the XDG specification itself. Some relevant snippets:

XDG_DATA_HOME

• Where user-specific data files should be written (analogous to /usr/share).
• Should default to $HOME/.local/share.

XDG_STATE_HOME

• Where user-specific state files should be written (analogous to /var/lib).
• Should default to $HOME/.local/state.

The $XDG_STATE_HOME contains state data that should persist between (application) restarts, but that is not important or portable enough to the user that it should be stored in $XDG_DATA_HOME. It may contain:

actions history (logs, history, recently used files, …)

current state of the application that can be reused on a restart (view, layout, open files, undo history, …)

User-specific executable files may be stored in $HOME/.local/bin. Distributions should ensure this directory shows up in the UNIX $PATH environment variable, at an appropriate place.

There's also a PR on the documentation repo that includes a new page outlining the currently proposed XDG locations:
https://github.com/MycroftAI/documentation/pull/208/files#diff-80549a11e37425500fd598dd99e4d30e002b465510f57ca8836a9b2c8bf96b0a

• $XDG_CONFIG_HOME/mycroft for configuration
• $XDG_DATA_HOME/mycroft for persistent data storage
• $XDG_CACHE_HOME/mycroft for non-essential data

Are there other possibilities that are worth considering?

3. How do we make a virtual environment XDG compliant?

Is this more of question for the future when we may have multiple virtual environments for different parts of the system?
The mycroft venv will remain in the mycroft-core directory.

Any other questions I missed?

[PureTryOut]

3. How do we make a virtual environment XDG compliant?

I'm not sure why this keeps being asked. I assume people just want their config and data separate depending on the virtual environment used, and that's perfectly possible by setting the env values properly.

With this PR included MyCroft makes use of the $XDG_CONFIG_HOME, $XDG_DATA_HOME and $XDG_CACHE_HOME (also $XDG_STATE_HOME once #2802 is in) environment variables. Just set those to the directory you want your settings and data to be saved in and voilá. This also makes it possible to have separate skills per virtual environment as that just follows $XDG_DATA_HOME (currently anyway, maybe we decide differently but it has my preference for now).

For example you can save everything to /opt/mycroft-core still if you want:

export XDG_CONFIG_HOME=/opt/mycroft-core
export XDG_DATA_HOME=/opt/mycroft-core
export XDG_CACHE_HOME=/opt/mycroft-core

[krisgesling]

Just hit an issue in MSK - we'll need to do an updated release of that to use XDG too.

Posting here so I don't forget

[PureTryOut]

@krisgesling how is that MSK release coming along? I really want this in...

[krisgesling]

Hey Bart, I haven't had time to look at MSK and it's not in our next few sprints so it will likely be some time before I can get to it.

Is this something you'd have time to look at?

[PureTryOut]

If you could link me to the issue in MSK, maybe?

[krisgesling]

Thanks! I know working on MSK is not what you set out to do.

I've created a new issue and linked it to this PR:
MycroftAI/mycroft-skills-kit#62

I think it should just be Skill and test creation but we'll want to test all the major functions to be sure.

[PureTryOut]

So now the MSK changes are merged, what's left here to do (besides rebasing, will do that in a sec)? If we need to continue the discussion on the proper location for these things then fine, but let's do that so we don't keep this hanging for longer unnecessarily.

[PureTryOut]

@krisgesling a few months later and last time I didn't get a response, can we please move this forward?

[krisgesling]

Hi Bart, I know it's gone on for a long time.

What this really needs is someone to comprehensively test it out and stake their reputation on it being solid. Then we can merge it in.

Right now, I can't provide the time it will take to do that. I'm spending every moment I have getting the Mark II's out the door to fulfill a 4 year old promise to many thousands of people. I know that is going to be frustrating, however it's our number one priority.

If any core contributor community members have time to review and test this it would be much appreciated.

[forslund]

This seems to work rather well. A part from a nitpick of an unnecessary import, I did run into a possible edge case that may or may not be something...

I had an nonstandardly named folder in the /opt/mycroft/skills (this is allowed but mainly happens in development setups). In my case what normally gets installed in a folder fallback-query.mycroft was simply in a folder called skill-query (the original name I used when I created the first draft). After startup I got errors such as:

Exception in thread Thread-4:
Traceback (most recent call last):
  File "/usr/lib/python3.8/threading.py", line 932, in _bootstrap_inner
    self.run()
  File "/home/ake/projects/python/mycroft-core/mycroft/skills/skill_manager.py", line 243, in run
    self._start_settings_update()
  File "/home/ake/projects/python/mycroft-core/mycroft/skills/skill_manager.py", line 198, in _start_settings_update
    self.upload_queue.start()
  File "/home/ake/projects/python/mycroft-core/mycroft/skills/skill_manager.py", line 56, in start
    self.send()
  File "/home/ake/projects/python/mycroft-core/mycroft/skills/skill_manager.py", line 71, in send
    loader.instance.settings_meta.upload()
  File "/home/ake/projects/python/mycroft-core/mycroft/skills/settings.py", line 245, in upload
    self._update_settings_meta()
  File "/home/ake/projects/python/mycroft-core/mycroft/skills/settings.py", line 287, in _update_settings_meta
    skill_gid=self.skill_gid,
  File "/home/ake/projects/python/mycroft-core/mycroft/skills/settings.py", line 190, in skill_gid
    skill = skills[skill_dir]
KeyError: '/home/ake/.local/share/mycroft/skills/skill-query'

Doing some checking it seems like msm had both copied the old skill folder and created a folder called fallback-query.mycroftai.

This is all first glance analyzis and I need to confirm that this is it. If it is the case I wouldn't consider this a blocker for the issue since it's mostly for devs it would occur. (And not very often at that)

[PureTryOut]

I kinda forgot what I pushed 4 months ago, can you still reproduce this @forslund?

[forslund]

I have not. It was a pretty crazy set of circumstances that only occurs with messy devs like myself.