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

Use XDG Base directories for settings, cache and runtime data #2578

Closed
wants to merge 2 commits into from
Closed

Use XDG Base directories for settings, cache and runtime data #2578

wants to merge 2 commits into from

Conversation

PureTryOut
Copy link
Contributor

@PureTryOut PureTryOut commented May 8, 2020

Description

This makes Mycroft use the XDG Base Directory specification.
Data is now saved to:

  • settings to $XDG_CONFIG_DIR/mycroft, which most of the time equals to ~/.config/mycroft
  • data to $XDG_DATA_DIRS/mycroft, which most of the time equals to ~/.local/share/mycroft
  • cache to $XDG_CACHE_HOME/mycroft, which most of the time equals to ~/.cache

Since this changes the default location of pretty much everything, please test it well!

Goal is to support and read from the old ~/.mycroft location if it already exists, but save to the XDG Base Directories and read from it on new installations.

Together with #2559 this should be full XDG Base Directory compliance.

How to test

First of all, run this PR with an existing installation, and check if everything is loaded from the usual locations. Then shut it down and restart to make sure it has saved to the new locations.
Then, completely remove any trace of Mycroft settings from your system. Easiest would be to fully deinstall (remove /opt/mycroft), remove ~/.mycroft, and the XDG Base directories mentioned above.
Now reinstall Mycroft (with this PR applied) and start and connect it. Check if ~/.mycroft is not created anymore and ~/.config/mycroft, ~/.local/share/mycroft and ~/.cache/mycroft are with the correct files and directories in them.

Contributor license agreement signed?

CLA [x]

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label May 8, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@pep8speaks
Copy link

pep8speaks commented May 8, 2020

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 2021-01-08 20:50:15 UTC

test/util.py Outdated Show resolved Hide resolved
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.

I still haven't finished writing the specs for this feature you've already implemented so I'll start by sending some of the notes I made on this PR this weekend.

mycroft/configuration/config.py Outdated Show resolved Hide resolved
mycroft/configuration/locations.py Show resolved Hide resolved
mycroft/filesystem/__init__.py Outdated Show resolved Hide resolved
mycroft/configuration/config.py Outdated Show resolved Hide resolved
mycroft/configuration/config.py Outdated Show resolved Hide resolved
@PureTryOut
Copy link
Contributor Author

I still haven't finished writing the specs for this feature you've already implemented

Woops, sorry. I got too excited for this 🙈

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

4 similar comments
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@PureTryOut
Copy link
Contributor Author

I still haven't finished writing the specs for this feature you've already implemented so I'll start by sending some of the notes I made on this PR this weekend.

@forslund I realize you're less active nowadays but is there a chance you have finished the specs you were writing? I'd like to finish this PR at some point 😉

@forslund
Copy link
Collaborator

Sure I'll try to have my thoughts jotted down within the week.

@forslund
Copy link
Collaborator

forslund commented Jul 9, 2020

Hi, sorry for the long delay in coming back with this. You seem to have implemented much of what I describe already.

  • FilesystemAccess

  • the mycroft skill filesystem "scratchpad": This is a very internal system and can just be moved by the startup of the skills service

  • identity location: This is sort of an internal object and can be moved but it's sort of tricky to do a clean move since it's accessed by all Mycroft processess. Easiest is probably to allow both the old and an XDG based location but when writing new identity information use the new location.

  • mycroft configuration locations: Easiest and most compatible would be to just insert the XDG config locations into the list making it something like
    [DEFAULT_CONFIG (mycroft internal), XDG_SYSTEM_CONFIG, SYSTEM_CONFIG (/etc/mycroft), USER_CONFIG (~/.mycroft), XDG_USER_CONFIG, patch]
    Possibly set the USER_CONFIG path to XDG_USER_CONFIG if the USER_CONFIG doesn't exist

  • precise model download location: Should check all the XDG_locations /share/mycroft/models and the current one. Would be good if we could make a package out of this one as well and include it in /usr/share somewhere.

  • precise binary download: This could do a move to an XDG based data directory since it's really only used by a single process. (this download should in my opinion be removed and instead a package should be used or downloaded through the setup script.

  • padatious intent cache: This could be moved the users XDG_DATA directory completely. May not need a move even. Padatious will regenerate the cache if it doesn't find it in the new location.

May not be much news but these were the thoughts I had (had to rebuild my document)

@PureTryOut
Copy link
Contributor Author

mycroft configuration locations: Easiest and most compatible would be to just insert the XDG config locations into the list making it something like
[DEFAULT_CONFIG (mycroft internal), XDG_SYSTEM_CONFIG, SYSTEM_CONFIG (/etc/mycroft), USER_CONFIG (~/.mycroft), XDG_USER_CONFIG, patch]
Possibly set the USER_CONFIG path to XDG_USER_CONFIG if the USER_CONFIG doesn't exist

I'm wondering, wouldn't it be better to migrate everything to XDG_CONFIG_PATH where possible rather than keeping things in the old location? It'd be nice to eventually get rid of the old paths entirely so there is less code, rather than doing difficult by checking multiple locations all the time. New installations would use XDG anyway so the amount of systems using the old paths would be less and less over time.

Personally I rather have some logic to move files where needed and the following:

[DEFAULT_CONFIG (mycroft internal), SYSTEM_CONFIG (MYCROFT_SYSTEM_CONFIG env or /etc/mycroft), USER_CONFIG (XDG Config path), patch]

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@forslund
Copy link
Collaborator

forslund commented Sep 9, 2020

My main reasoning was to avoid needing to have config file merging on disk, just in memory (which would use the existing system).

If the XDG version exists and a user creates the .mycroft/mycroft.conf it will need to merge the changes from that file into the the file in the XDG path. it's less things that can go wrong. If you feel like the merge is better I'm fine with that as well.

@PureTryOut
Copy link
Contributor Author

If the XDG version exists and a user creates the .mycroft/mycroft.conf

I think we should in that case just ignore the new file in the old location, it's just the wrong location to put things. We'll move over existing files from the old location, but if someone puts a new file there and then complains it isn't being used we just point them to the new location. People will get used to it eventually.
If you want we could even leave a $HOME/.mycroft/this-location-is-not-used-anymore.txt file behind with the contents telling where it is now, but I don't personally see a need for it.

I rather not merge config files from an old and a new location, it just complicates things.

@krisgesling krisgesling added Type: Enhancement - roadmapped Implementation of a feature that is a priority on the roadmap. Status: To be reviewed Concept accepted and PR has sufficient information for full review labels Sep 24, 2020
@krisgesling krisgesling added Status: Work in progress PR being actively worked on, not yet ready for review. and removed Status: To be reviewed Concept accepted and PR has sufficient information for full review labels Nov 24, 2020
@krisgesling
Copy link
Contributor

We've been recommending the use of the config manager rather than directly editing files for a little while now, but a big challenge here is that ~/.mycroft/mycroft.conf has been around for a very long time. So there are lots of references in our docs, blog posts and out in the community to it. Regardless of whether it's a correct or incorrect location, if people are following a tutorial / guide / documentation and it just quietly fails, that is a pretty frustrating experience.

So my preference would be to support the existing location in some way but flagging it as deprecated so we can remove it entirely in a future release.

@PureTryOut
Copy link
Contributor Author

Those are valid points. I now made sure to check both old and new locations and merge them together.

I'm trying to notify that the old location is deprecated and will be removed in a future version, but I can't find the prints in any output anywhere. Where are print statements supposed to show up if at all? Is there some other way I should notify the user?

@forslund
Copy link
Collaborator

forslund commented Dec 7, 2020

They should, when using the start-mycroft.sh, be output to the relevant log file in /var/log/mycroft/*.log

However I think it maybe better to use LOG.warning to print these kinds of messages.

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.

To me this mainly looks good now!

There's a couple of (possible) bugs I noted and a slight incompatibility. Not sure we should bother about the incompatibility though. It's small and should not be a critical issue. (like causing exceptions or so)

mycroft/skills/skill_updater.py Show resolved Hide resolved
bin/mycroft-config Outdated Show resolved Hide resolved
mycroft/configuration/config.py Outdated Show resolved Hide resolved
mycroft/configuration/config.py Outdated Show resolved Hide resolved
mycroft/client/speech/hotword_factory.py Outdated Show resolved Hide resolved
mycroft/client/enclosure/mark1/__init__.py Outdated Show resolved Hide resolved
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@forslund
Copy link
Collaborator

Hum hum hum, the VK test seems to hang on the QA skills tests :/ will see if I can run them manually and see if the logs say anything extra

@forslund forslund mentioned this pull request Jan 6, 2021
@forslund
Copy link
Collaborator

forslund commented Jan 6, 2021

I got really stuck on why the config patching didn't work in the VK tests, in the end it turned out to be quite simple. The __patch dict should have top priority and not the least priority.

I ran into a couple of other minor problems are still in the migration of files. Gonna add separate comments on each issue I saw.

When figuring this out I made some commits to show the needed changes https://github.com/forslund/mycroft-core/commits/PureTryOut-xdg-config

@PureTryOut
Copy link
Contributor Author

Made the changes, let's see! Honestly, VK is one big mystery to me.

@forslund
Copy link
Collaborator

forslund commented Jan 7, 2021

Just realized this will bork (and did bork) the VK test runner since it moves the identity file (and doesn't restore it)... Could you edit the Jenkinsfile@L60 to be
-v "$HOME/voight-kampff/identity:/root/.config/mycroft/identity" \

to make sure the identity file isn't moved from the shared volume.

@PureTryOut
Copy link
Contributor Author

Hmm, still failing. Seems testing the skills times out or something?

@forslund
Copy link
Collaborator

forslund commented Jan 7, 2021

Right now the identity file is broken (I think) so many of the tests using it (weather, Q and A) are failing and it gets 10 seconds timeout of all of those.

We can retry when the identity file for VK is restored

@krisgesling
Copy link
Contributor

New identity file in place, triggered a new run 🤞

@PureTryOut
Copy link
Contributor Author

PureTryOut commented Jan 7, 2021

Uh, seems it ignores the updated Jenkinsfile file and keeps mounting the file to /root/.mycroft/identity...

https://hudson.mycroft.team/blue/organizations/jenkins/mycroft-core/detail/PR-2578/35/pipeline#step-39-log-1

@forslund
Copy link
Collaborator

forslund commented Jan 7, 2021

Connecting to https://api.github.com to check permissions of obtain list of PureTryOut for MycroftAI/mycroft-core
Loading trusted files from base branch dev at 81eae60b61071746d3f5837c1fe5b8b64ecd0e64 rather than db5a3eba75d86c000240fed66bf26bc30e98cb77
Obtained Jenkinsfile from 81eae60b61071746d3f5837c1fe5b8b64ecd0e64
‘Jenkinsfile’ has been modified in an untrusted revision

Seems like Jenkins doesn't trust you :(

I'll see if I can figure out how to make it trust you / this change of the Jenkinsfile

@forslund
Copy link
Collaborator

If you make changes don't push to this branch since it'll destroy the jenkins identity file :/ ... Update in a branch and let me know. I'll pull them into the branch for the PR I created

@PureTryOut
Copy link
Contributor Author

Done, my branch xdg-config-jarbas contains the changes.

Too bad another review with more requested changes comes in so late 😢

@j1nx
Copy link

j1nx commented Jan 18, 2021

Done, my branch xdg-config-jarbas contains the changes.

Too bad another review with more requested changes comes in so late 😢

Tried out that branch on OVOS. The following error pops up;

Traceback (most recent call last):
  File "/usr/bin/mycroft-skills", line 11, in <module>
    load_entry_point('mycroft-core==20.8.0', 'console_scripts', 'mycroft-skills')()
  File "/usr/lib/python3.8/site-packages/mycroft/skills/__main__.py", line 197, in main
    event_scheduler = EventScheduler(bus)
  File "/usr/lib/python3.8/site-packages/mycroft/skills/event_scheduler.py", line 70, in __init__
    self.schedule_file = new_path
NameError: name 'new_path' is not defined

@PureTryOut
Copy link
Contributor Author

Yup known problem @j1nx, fixed in #2794

@j1nx
Copy link

j1nx commented Jan 19, 2021

Right! To manu different branches / tries because of this VK stuff. Will check/test the other one for you.

@PureTryOut
Copy link
Contributor Author

Closing this as it's being replaced by #2794 and it seems it causes confusion by being still open. It's the same changes, but with stuff to make VK work (which I don't have permissions for in this PR).

@PureTryOut PureTryOut closed this Jan 19, 2021
@PureTryOut PureTryOut deleted the xdg-config branch August 10, 2021 07:07
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 - roadmapped Implementation of a feature that is a priority on the roadmap.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants