User directory config files by JeroMiya · Pull Request #534 · OSVR/OSVR-Core

JeroMiya · 2017-04-06T22:03:17Z

Do not merge yet. Please merge this PR first:
#529

Then I'll rebase this on those changes.

This pull request includes and supersedes this one:
#265

This adds search-paths behavior for the osvr config file, including prioritizing a user-specific configuration if present, before falling back to the current CWD behavior.

…pty config if one is not found

godbyk · 2017-04-07T00:31:25Z

+            std::string configSubpath = "config";
+
+#if defined(OSVR_LINUX)
+            // There's currently no great location for storing log files in the


We can delete the first paragraph of the comment and update the second para to reflect XDG_CONFIG_HOME.

godbyk · 2017-04-07T00:37:06Z

+            // specific non-essential data files should be stored. If
+            // $XDG_CACHE_HOME is either not set or empty, a default equal to
+            // $HOME/.cache should be used.
+            auto xdg_cache_dir = getEnvironmentVariable("XDG_CONFIG_HOME");


On Linux, if you're searching for an existing configuration file (as opposed to determining where to write a new one), then you will want to check the XDG_CONFIG_DIRS paths as well. From the XDG Base Directory Specification:

$XDG_CONFIG_DIRS defines the preference-ordered set of base directories to search for configuration files in addition to the $XDG_CONFIG_HOME base directory. The directories in $XDG_CONFIG_DIRS should be separated with a colon :.

If $XDG_CONFIG_DIRS is either not set or empty, a value equal to /etc/xdg should be used.

The order of base directories denotes their importance; the first directory listed is the most important. When the same information is defined in multiple places the information defined relative to the more important base directory takes precedent. [...] The base directory defined by $XDG_CONFIG_HOME is considered more important than any of the base directories defined by $XDG_CONFIG_DIRS.

I'm going to put this as a TODO for now. The function this is in currently only returns one directory, and this change would require it to return multiple directories.

godbyk · 2017-04-13T21:59:20Z

-                log->error() << "'" << configName << "' is special file.";
-                return -1;
-            }
+            log->error() << "Could not find specified config file.";


The error message could be more specific (or removed if configureServerFromFirstFileInList() prints a better one). Possible failure cases: file not found, file couldn't be opened, file couldn't be prased, etc.

godbyk · 2017-04-13T22:01:20Z

+
+// Standard includes
+#include <vector>
+


Don't forget #include <string>.

godbyk · 2017-04-13T22:04:55Z

+namespace osvr {
+namespace server {
+
+    ServerPtr configureServerFromString(std::string const &json) {


Thanks for moving this to the .cpp file. I meant to do that when I was originally refactoring and it slipped my mind later.

godbyk · 2017-04-13T22:08:01Z

+        }
+        log->error() << "Could not open config file!";
+        log->error() << "Attempted the following files:";
+        for (auto i = configNames.begin(); i != configNames.end(); i++) {


You could use the more succinct C++11 loop: for (const auto& configName : configName) { log->error() << configName; }.

This code was removed - the config file attempts are logged elsewhere so this was redundant.

godbyk · 2017-04-13T22:09:34Z

 #include <iostream>
-#include <sstream>
+#include <vector>



Need #include <string>. And it looks like you can ditch iostream and fstream and exception (move 'em to the .cpp file). Same with the Log headers above.

godbyk · 2017-04-13T22:12:48Z

+#if defined(OSVR_LINUX)
+            // $XDG_CONFIG_HOME defines the base directory relative to which user
+            // specific non-essential data files should be stored. If
+            // $XDG_CACHE_HOME is either not set or empty, a default equal to


s/XDG_CACHE_HOME/XDG_CONFIG_HOME/

godbyk · 2017-04-13T22:13:27Z

+            // $XDG_CONFIG_HOME defines the base directory relative to which user
+            // specific non-essential data files should be stored. If
+            // $XDG_CACHE_HOME is either not set or empty, a default equal to
+            // $HOME/.cache should be used.


s/cache/config/g throughout, I think.

godbyk · 2017-04-13T22:24:44Z

+
+            names.push_back(userConfig.string());
+            names.push_back(configFileName);
+


There are more places worth checking, too.

On Linux:

$XDG_CONFIG_HOME/osvr/osvr_server_config.json

For each colon-separated dir in $XDG_CONFIG_DIRS: $prefix/osvr/osvr_server_config.json

/etc/xdg if $XDG_CONFIG_DIRS is not set or empty.

/etc/osvr_server_config.json for old-school paths

/usr/local/etc/osvr_server_config.json for old-school paths

On macOS:

$HOME/Library/Application Support/OSVR/config/osvr_server_config.json

/Library/Application Support/OSVR/config/osvr_server_config.json

wherever brew installs it by default (I think /usr/local/Cellar/osvr-core/bin/osvr_server_config.json, maybe)

/etc/osvr_server_config.json for old-school paths

/usr/local/etc/osvr_server_config.json for old-school paths

Moved to new issue: #537

godbyk · 2017-04-13T22:25:31Z


 namespace osvr {
 namespace server {
-    namespace detail {


Oh, wow! This is ancient. I thought we killed it ages ago.

godbyk · 2017-04-13T22:29:05Z

+
+        for (const auto name : configNames) {
+            std::ifstream config(name);
+            if (config.good()) {


Log which file you're running with so it's easier to troubleshoot later.

It's logged in configureServerFromFile, but I adjusted the log there to be more consistent with the others in wording.

do we even need to check the member good() or can we just use the explicit bool conversion operator?

operator bool would return true at EOF while good() would return false. So calling good() avoids reading an empty file.

alrighty then, things I didn't know about iostreams.

godbyk

Just a couple minor quibbles, but otherwise I think it looks good!

godbyk · 2017-04-17T21:04:47Z


    if (values.count("help")) {
-        std::cout << optionsVisible << std::endl;
+        log->info() << optionsVisible << std::endl;


In this particular case, we should use std::cout instead of a logger because we don't want the output lines prefixed with timestamps, etc.

godbyk · 2017-04-17T21:07:21Z

@@ -0,0 +1,73 @@
+// Internal Includes


Add standard copyright header.

date and indentation don't match but...

godbyk · 2017-04-17T21:07:33Z

@@ -0,0 +1,45 @@
+/** @file
+    @brief Auto-generated configuration header - do not edit!


This header file isn't auto-generated.

updated the comment

godbyk

👍 Looks good to me!

rpavlik · 2017-04-18T19:06:22Z

+        log->info() << "Using config file " << configFileArgument << " from command line argument.";
+        configPaths = { configFileArgument };
+    } else {
+        log->info() << "Using default config files - pass a filename on the command "


is the word "files" here a typo?

rpavlik

Minor itty bitty nitpicks.

rpavlik · 2017-04-18T19:06:53Z

-                << "' not found.  Using empty configuration.";
-            configPath = boost::none;
+    server = osvr::server::configureServerFromFirstFileInList(configPaths);
+    if (!server) {        


you've got trailing spaces - did you clang format your changes?

rpavlik · 2017-04-18T19:07:58Z

+    if (!server) {        
+        // only attempt to load the empty config if no arguments are passed.
+        if (!values.count("config")) {
+            log->info() << "Could not find a valid config file. Using default config object.";


Is this maybe better phrased "could not find config file with default name" or something like that? The user seeing this message wouldn't have passed a config file, so they might be confused as to why they're seeing this.

rpavlik · 2017-04-18T19:08:33Z

+    <http://sensics.com/osvr>
+*/
+
+// Copyright 2015 Sensics, Inc.


is this 2015 code?

rpavlik · 2017-04-18T19:09:45Z

@@ -0,0 +1,73 @@
+// Internal Includes


date and indentation don't match but...

rpavlik · 2017-04-18T19:11:19Z

+            return nullptr;
+        }
+
+        std::stringstream sstr;


I'm assuming this is how you load a whole file in a single gulp?

@rpavlik Yeah, this just loads the whole file into a string. It's the best way I could find to do it.

rpavlik · 2017-04-18T19:11:57Z

+
+        for (const auto name : configNames) {
+            std::ifstream config(name);
+            if (config.good()) {


do we even need to check the member good() or can we just use the explicit bool conversion operator?

rpavlik · 2017-05-01T20:16:37Z

👍

d235j and others added 4 commits November 7, 2015 14:08

First attempts

dc3706e

Search predefined paths for config file, and automatically load an em…

15a7b8c

…pty config if one is not found

Clean up formatting

ac1720a

Merged master and fixed a few things with the merge.

afe5262

JeroMiya added the in progress label Apr 6, 2017

godbyk reviewed Apr 7, 2017

View reviewed changes

JeroMiya mentioned this pull request Apr 11, 2017

Automatically find config data or load default #265

Closed

JeroMiya added 2 commits April 13, 2017 17:37

Merged changes from master and refactored user-config changes to match.

5848e89

Updated comment for accuracy.

b5d020b

godbyk reviewed Apr 13, 2017

View reviewed changes

JeroMiya mentioned this pull request Apr 17, 2017

Expand config search paths #537

Open

Fixes for PR review.

7e0e545

godbyk reviewed Apr 17, 2017

View reviewed changes

Minor changes per review.

3513aba

godbyk approved these changes Apr 18, 2017

View reviewed changes

rpavlik reviewed Apr 18, 2017

View reviewed changes

rpavlik requested changes Apr 18, 2017

View reviewed changes

JeroMiya added 2 commits April 18, 2017 15:51

Formatting, output message adjustment, and updated date in headers.

4ccd1b4

osvr-server log output text adjustment

64961f0

rpavlik approved these changes May 1, 2017

View reviewed changes

JeroMiya merged commit ef4b572 into master May 1, 2017

JeroMiya deleted the user-config branch May 1, 2017 20:49

JeroMiya removed the in progress label May 1, 2017


		names.push_back(userConfig.string());
		names.push_back(configFileName);

		@@ -0,0 +1,45 @@
		/** @file
		@brief Auto-generated configuration header - do not edit!

Conversation

JeroMiya commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godbyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godbyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpavlik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!