This repository has been archived by the owner on Sep 8, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Pure-try-out's XDG config PR with changed Jenkinsfile #2794
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
ff9f8e8
Update identity location for VK test
forslund e20443b
Use XDG Base directories for settings, cache and runtime data
PureTryOut faf101b
Increase Jenkins timeout
forslund 2801799
Restore system locations to mycroft.configuration
forslund 8e69d46
Correct resolution order for resolving log configs
forslund 9029dc1
Slight cleanup
forslund 3fd96cf
WIP Review comments
forslund 87d22d3
fix cyclic imports with LOG
JarbasAl 4258e2f
Reorder imports of mycroft.configuration
forslund File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
import sys | ||
import io | ||
from math import ceil | ||
import xdg.BaseDirectory | ||
|
||
from .gui_server import start_qml_gui | ||
|
||
|
@@ -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): | ||
|
@@ -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(" ===============================================") | ||
forslund marked this conversation as resolved.
Show resolved
Hide resolved
|
||
LOG.warning(" == DEPRECATION WARNING ==") | ||
LOG.warning(" ===============================================") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (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) | ||
|
@@ -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))) | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theConfiguration.get()
function cleaner.There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?