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

Read skills from XDG Base directory specification locations #2803

Closed
wants to merge 1 commit into from
Closed

Read skills from XDG Base directory specification locations #2803

wants to merge 1 commit into from

Conversation

PureTryOut
Copy link
Contributor

@PureTryOut PureTryOut commented Jan 15, 2021

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 devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jan 15, 2021
@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@pep8speaks
Copy link

pep8speaks commented Jan 16, 2021

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
Copy link

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

14 similar comments
@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@devops-mycroft
Copy link

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

@krisgesling krisgesling added Status: Work in progress PR being actively worked on, not yet ready for review. Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap. labels Jan 22, 2021
@krisgesling
Copy link
Contributor

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
Copy link
Contributor Author

PureTryOut commented Feb 11, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

PureTryOut commented Aug 10, 2021

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
Copy link
Contributor Author

I still don't understand the unit test failure 🤔

@krisgesling
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

And now our good friend VK is failing again 😠

@codecov-commenter
Copy link

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
Impacted file tree graph

@@           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
Copy link
Contributor Author

PureTryOut commented Aug 30, 2021

@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
Copy link
Contributor

It looks like this line:

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Hopefully it passes now, fingers crossed crossed_fingers

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

@krisgesling
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

PureTryOut commented Sep 10, 2021

  1. 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
Copy link
Contributor

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
Copy link
Contributor Author

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

@krisgesling
Copy link
Contributor

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
Copy link
Contributor Author

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

@krisgesling
Copy link
Contributor

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.

@forslund forslund mentioned this pull request Feb 16, 2022
@PureTryOut
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@krisgesling
Copy link
Contributor

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.

Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

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)

mycroft/skills/skill_manager.py Outdated Show resolved Hide resolved
Also move over skills from data_dir if they still exist
@PureTryOut
Copy link
Contributor Author

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

@forslund
Copy link
Collaborator

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

@PureTryOut PureTryOut closed this by deleting the head repository Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Status: Work in progress PR being actively worked on, not yet ready for review. Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap.
Projects
No open projects
Status: High priority
Development

Successfully merging this pull request may close these issues.

10 participants