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

Pure-try-out's XDG config PR with changed Jenkinsfile #2794

Merged
merged 9 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Jenkinsfile
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,12 @@ pipeline {
--label build=${JOB_NAME} \
-t voight-kampff-mark-1:${BRANCH_ALIAS} .'
echo 'Running Mark I Voight-Kampff Test Suite'
timeout(time: 60, unit: 'MINUTES')
timeout(time: 90, unit: 'MINUTES')
{
sh 'mkdir -p $HOME/core/$BRANCH_ALIAS/allure'
sh 'mkdir -p $HOME/core/$BRANCH_ALIAS/mycroft-logs'
sh 'docker run \
-v "$HOME/voight-kampff/identity:/root/.mycroft/identity" \
-v "$HOME/voight-kampff/identity:/root/.config/mycroft/identity" \
-v "$HOME/core/$BRANCH_ALIAS/allure:/root/allure" \
-v "$HOME/core/$BRANCH_ALIAS/mycroft-logs:/var/log/mycroft" \
--label build=${JOB_NAME} \
Expand Down
8 changes: 4 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,20 @@ Mycroft is nothing without skills. There are a handful of default skills that a

### Pairing Information
Pairing information generated by registering with Home is stored in:
`~/.mycroft/identity/identity2.json` <b><-- DO NOT SHARE THIS WITH OTHERS!</b>
`~/.config/mycroft/identity/identity2.json` <b><-- DO NOT SHARE THIS WITH OTHERS!</b>

### Configuration
Mycroft's configuration consists of 4 possible locations:
- `mycroft-core/mycroft/configuration/mycroft.conf`(Defaults)
- [Mycroft Home](https://home.mycroft.ai) (Remote)
- `/etc/mycroft/mycroft.conf`(Machine)
- `$HOME/.mycroft/mycroft.conf`(User)
- `/etc/mycroft/mycroft.conf` (Machine)
- `$XDG_CONFIG_DIR/mycroft/mycroft.conf` (which is by default `$HOME/.config/mycroft/mycroft.conf`) (USER)

When the configuration loader starts, it looks in these locations in this order, and loads ALL configurations. Keys that exist in multiple configuration files will be overridden by the last file to contain the value. This process results in a minimal amount being written for a specific device and user, without modifying default distribution files.

### Using Mycroft Without Home

If you do not wish to use the Mycroft Home service, before starting Mycroft for the first time, create `$HOME/.mycroft/mycroft.conf` with the following contents:
If you do not wish to use the Mycroft Home service, before starting Mycroft for the first time, create `$HOME/.config/mycroft/mycroft.conf` with the following contents:

```
{
Expand Down
6 changes: 3 additions & 3 deletions bin/mycroft-config
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,13 @@ function validate_config_file() {
return $result
}

_conf_file="~/.mycroft/mycroft.conf"
_conf_file="${XDG_CONFIG_HOME:-$HOME/.config}/mycroft/mycroft.conf"
function name_to_path() {
case ${1} in
"system") _conf_file="/etc/mycroft/mycroft.conf" ;;
"user") _conf_file=$(readlink -f ~/.mycroft/mycroft.conf) ;;
"user") _conf_file=$(readlink -f ${XDG_CONFIG_HOME:-$HOME/.config}/mycroft/mycroft.conf) ;;
"default") _conf_file="$DIR/../mycroft/configuration/mycroft.conf" ;;
"remote") _conf_file="/var/tmp/mycroft_web_cache.json" ;;
"remote") _conf_file="$HOME/.cache/mycroft/web_cache.json" ;;

*)
echo "ERROR: Unknown name '${1}'."
Expand Down
3 changes: 3 additions & 0 deletions mycroft/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from mycroft.skills import (MycroftSkill, FallbackSkill,
intent_handler, intent_file_handler)
from mycroft.skills.intent_service import AdaptIntent
from mycroft.util.log import LOG

MYCROFT_ROOT_PATH = abspath(join(dirname(__file__), '..'))

Expand All @@ -33,3 +34,5 @@
'intent_handler',
'intent_file_handler',
'AdaptIntent']

LOG.init() # read log level from config
17 changes: 4 additions & 13 deletions mycroft/api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
from requests import HTTPError, RequestException

from mycroft.configuration import Configuration
from mycroft.configuration.config import DEFAULT_CONFIG, SYSTEM_CONFIG, \
USER_CONFIG
from mycroft.identity import IdentityManager, identity_lock
from mycroft.version import VersionManager
from mycroft.util import get_arch, connected, LOG
Expand Down Expand Up @@ -49,12 +47,9 @@ class Api:
def __init__(self, path):
self.path = path

# Load the config, skipping the REMOTE_CONFIG since we are
# Load the config, skipping the remote config since we are
# getting the info needed to get to it!
config = Configuration.get([DEFAULT_CONFIG,
SYSTEM_CONFIG,
USER_CONFIG],
cache=False)
config = Configuration.get(cache=False, remote=False)
config_server = config.get("server")
self.url = config_server.get("url")
self.version = config_server.get("version")
Expand Down Expand Up @@ -239,9 +234,7 @@ def activate(self, state, token):
platform_build = ""

# load just the local configs to get platform info
config = Configuration.get([SYSTEM_CONFIG,
USER_CONFIG],
cache=False)
config = Configuration.get(cache=False, remote=False)
if "enclosure" in config:
platform = config.get("enclosure").get("platform", "unknown")
platform_build = config.get("enclosure").get("platform_build", "")
Expand All @@ -263,9 +256,7 @@ def update_version(self):
platform_build = ""

# load just the local configs to get platform info
config = Configuration.get([SYSTEM_CONFIG,
USER_CONFIG],
cache=False)
config = Configuration.get(cache=False, remote=False)
if "enclosure" in config:
platform = config.get("enclosure").get("platform", "unknown")
platform_build = config.get("enclosure").get("platform_build", "")
Expand Down
6 changes: 3 additions & 3 deletions mycroft/client/enclosure/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
This provides any "enclosure" specific functionality, for example GUI or
control over the Mark-1 Faceplate.
"""
from mycroft.configuration import LocalConf, SYSTEM_CONFIG
from mycroft.configuration import Configuration
from mycroft.util.log import LOG
from mycroft.util import wait_for_exit_signal, reset_sigint_handler

Expand Down Expand Up @@ -70,8 +70,8 @@ def main(ready_hook=on_ready, error_hook=on_error, stopping_hook=on_stopping):
only the GUI bus will be started.
"""
# Read the system configuration
system_config = LocalConf(SYSTEM_CONFIG)
platform = system_config.get("enclosure", {}).get("platform")
config = Configuration.get(remote=False)
platform = config.get("enclosure", {}).get("platform")
Comment on lines 72 to +74
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to extend this to the USER_CONFIG level? Or is it just to deal with the many possible configs scenario?

Maybe worth updating the comment above it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's to deal with the many possible configs scenario yes. XDG comes with it's /etc/xdg system level configuration but Mycroft also supports plain /etc. Both of these have to be checked. We can just check those 2 directories manually here as well, which might make the Configuration.get() function cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think using all configs follows the Principle of least astonishment

If as a dev/user i change that string in the file i'm told belongs to user and it does nothing I will be scratching my head

Note that there are skills depending on this string, if you install some "enclosure skill" it will have write access to user config as part of its setup, but not to the system level.

This string is really a flag that changes behavior, its no longer a simple identifier about what hardware its running. With the mark2 loading different QML and such based on this string i can also imagine skills wanting to override this to disable stuff like the UI scaling and providing a new "skin".

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree with Jarbas on this, if I am reading his comment right. I have always been confused why certain parts of the system only need certain config files. Why can't we just always use all the configs? What benefit is there to only loading a partial list of the config files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Excluding remote is generally a good idea if we know that a setting will not originate from there. The speedup is significant.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree in excluding remote, especially because it should never touch these values, just disagree with the user config being removed

one is an optimization and you fully control it, the other is user facing and just confusing if ignored

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I think this is good including the User config in this. I guess the old version was a quick hack. Is the consensus that we leave it like it is now, since as far as I can tell it includes all local config files?

Copy link
Contributor

Choose a reason for hiding this comment

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

This includes all configs but the remote one yes. I agree with you guys here, exclude remote but otherwise use all available configs in the proper priority.

Copy link
Member

@chrisveilleux chrisveilleux Jul 12, 2021

Choose a reason for hiding this comment

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

One thing that still bugs me about this... a config that is not in the remote config right now could be added in the future. For example, when the Mark II gets the the "consumer ready" point, we should have a remote config that allows users to select their language. How do we know if doing so breaks this code (or any other code that excludes remote)?

One other question, why is parsing an extra JSON file and merging it with others such an expensive operation?


enclosure = create_enclosure(platform)
if enclosure:
Expand Down
9 changes: 7 additions & 2 deletions mycroft/client/enclosure/mark1/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,13 @@
#
import subprocess
import time
import sys
from alsaaudio import Mixer
from threading import Thread, Timer

import serial

import xdg.BaseDirectory

import mycroft.dialog
from mycroft.client.enclosure.base import Enclosure
from mycroft.api import has_been_paired
Expand All @@ -29,7 +30,7 @@
from mycroft.client.enclosure.mark1.mouth import EnclosureMouth
from mycroft.enclosure.display_manager import \
init_display_manager_bus_connection
from mycroft.configuration import Configuration, LocalConf, USER_CONFIG
from mycroft.configuration import LocalConf, USER_CONFIG
from mycroft.messagebus.message import Message
from mycroft.util import play_wav, create_signal, connected, check_for_signal
from mycroft.util.audio_test import record
Expand Down Expand Up @@ -164,6 +165,10 @@ def process(self, data):
if "unit.factory-reset" in data:
self.bus.emit(Message("speak", {
'utterance': mycroft.dialog.get("reset to factory defaults")}))
subprocess.call(
(f'rm {xdg.BaseDirectory.save_config_path("mycroft")}'
'/mycroft/identity/identity2.json'),
shell=True)
subprocess.call(
'rm ~/.mycroft/identity/identity2.json',
shell=True)
Expand Down
35 changes: 31 additions & 4 deletions mycroft/client/speech/hotword_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@
from threading import Timer, Thread
from time import time, sleep
from urllib.error import HTTPError
import xdg.BaseDirectory

from petact import install_package
import requests

from mycroft.configuration import Configuration, LocalConf, USER_CONFIG
from mycroft.util.monotonic_event import MonotonicEvent
from mycroft.configuration import Configuration, LocalConf
from mycroft.configuration.locations import OLD_USER_CONFIG
from mycroft.util.log import LOG
from mycroft.util.monotonic_event import MonotonicEvent
from mycroft.util.plugins import load_plugin

RECOGNIZER_DIR = join(abspath(dirname(__file__)), "recognizer")
Expand Down Expand Up @@ -193,9 +195,31 @@ def __init__(self, key_phrase="hey mycroft", config=None, lang="en-us"):
from precise_runner import (
PreciseRunner, PreciseEngine, ReadWriteStream
)
local_conf = LocalConf(USER_CONFIG)

# We need to save to a writeable location, but the key we need
# might be stored in a different, unwriteable, location
# Make sure we pick the key we need from wherever it's located,
# but save to a writeable location only
local_conf = LocalConf(
join(xdg.BaseDirectory.save_config_path('mycroft'), 'mycroft.conf')
)

for conf_dir in xdg.BaseDirectory.load_config_paths('mycroft'):
conf = LocalConf(join(conf_dir, 'mycroft.conf'))
# If the current config contains the precise key use it,
# otherwise continue to the next file
if conf.get('precise', None) is not None:
local_conf['precise'] = conf.get('precise', None)
break

# If the key is not found yet, it might still exist on the old
# (deprecated) location
if local_conf.get('precise', None) is None:
local_conf = LocalConf(OLD_USER_CONFIG)

if not local_conf.get('precise', {}).get('use_precise', True):
raise PreciseUnavailable

if (local_conf.get('precise', {}).get('dist_url') ==
'http://bootstrap.mycroft.ai/artifacts/static/daily/'):
del local_conf['precise']['dist_url']
Expand Down Expand Up @@ -253,7 +277,10 @@ def update_precise(self, precise_config):

@property
def folder(self):
return join(expanduser('~'), '.mycroft', 'precise')
old_path = join(expanduser('~'), '.mycroft', 'precise')
if os.path.isdir(old_path):
return old_path
return xdg.BaseDirectory.save_data_path('mycroft', 'precise')

@property
def install_destination(self):
Expand Down
36 changes: 35 additions & 1 deletion mycroft/client/text/text_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import sys
import io
from math import ceil
import xdg.BaseDirectory

from .gui_server import start_qml_gui

Expand Down Expand Up @@ -142,7 +143,7 @@ def handleNonAscii(text):
##############################################################################
# Settings

config_file = os.path.join(os.path.expanduser("~"), ".mycroft_cli.conf")
filename = "mycroft_cli.conf"


def load_mycroft_config(bus):
Expand Down Expand Up @@ -171,6 +172,35 @@ def load_settings():
global max_log_lines
global show_meter

config_file = None

# Old location
path = os.path.join(os.path.expanduser("~"), ".mycroft_cli.conf")
if os.path.isfile(path):
LOG.warning(" ===============================================")
LOG.warning(" == DEPRECATION WARNING ==")
LOG.warning(" ===============================================")
Copy link
Contributor

@JarbasAl JarbasAl Jul 9, 2021

Choose a reason for hiding this comment

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

i suggest this whole Configuration section is refactored into smaller pieces

def _warn_xdg_config(old_user_config):
    LOG.warning(" ===============================================")
    LOG.warning(" ==             DEPRECATION WARNING           ==")
    LOG.warning(" ===============================================")
    LOG.warning(f" You still have a config file at {old_user_config}")
    LOG.warning(" Note that this location is deprecated and will" +
                " not be used in the future")
    LOG.warning(" Please move it to " + join(XDG.xdg_config_home,
                                             BASE_FOLDER))
                                             
def get_xdg_config_locations():
    # This includes both the user config and
    # /etc/xdg/mycroft/mycroft.conf
    xdg_paths = list(reversed(
        [join(p, CONFIG_FILE_NAME)
         for p in XDG.load_config_paths(BASE_FOLDER)]
    ))
    return xdg_paths

see here HelloChatterbox/HolmesV@5efa749#diff-7a26324169efbe9ae270dc54e1e9476fff06521e22b5294f4e55624081b92bbdL232

(ignore the fact that i have a flag to determine if xdg is enabled or not, that is for backwards compat with mycroft-core, will remove that once this PR is finally merged)

LOG.warning(" You still have a config file at " +
path)
LOG.warning(" Note that this location is deprecated and will" +
" not be used in the future")
LOG.warning(" Please move it to " +
os.path.join(xdg.BaseDirectory.save_config_path('mycroft'),
filename))
config_file = path

# Check XDG_CONFIG_DIR
if config_file is None:
for conf_dir in xdg.BaseDirectory.load_config_paths('mycroft'):
xdg_file = os.path.join(conf_dir, filename)
if os.path.isfile(xdg_file):
config_file = xdg_file
break

# Check /etc/mycroft
if config_file is None:
config_file = os.path.join("/etc/mycroft", filename)

try:
with io.open(config_file, 'r') as f:
config = json.load(f)
Expand All @@ -196,6 +226,10 @@ def save_settings():
config["show_last_key"] = show_last_key
config["max_log_lines"] = max_log_lines
config["show_meter"] = show_meter

config_file = os.path.join(
xdg.BaseDirectory.save_config_path("mycroft"), filename)

with io.open(config_file, 'w') as f:
f.write(str(json.dumps(config, ensure_ascii=False)))

Expand Down
Loading