Added osvr_server command-line options by godbyk · Pull Request #529 · OSVR/OSVR-Core

godbyk · 2017-03-09T20:58:27Z

This PR adds command-line options to the server for adjusting the log verbosity and also allows the server to run without specifying a configuration file (in which case it will assume an empty configuration { }).

$ ./osvr_server --help
Command Line Options:
  --help                display this help message
  -v [ --verbose ]      enable verbose logging
  -d [ --debug ]        enable debug logging

Verbose logging sets the log level to debug and debug logging sets the log level to trace.

This subsumes PR #293 by @Outurnate.

…check on creating config file

…absense of config file

…logging-improvements

JeroMiya

See questions/requests. Not sure, but I think this breaks server auto-start and default double-click behavior (with no command line arguments). Should look for a file with the default config file name in the CWD in that case and only use an empty config if that file does not exist either.

JeroMiya · 2017-04-05T22:24:12Z

+    opt::options_description optionsVisible("Command Line Options");
+    opt::positional_options_description optionsPositional;
+
+    optionsPositional.add("config", -1);


Does this make the config option the default option when only one argument is passed (like when you drag a config file over the server in windows explorer)? Just making sure we don't change the current behavior in that case.

Yes. So if you run osvr_server blah.json then it will use blah.json as the configuration file. The same thing happens if you drag and drop blah.json onto the osvr_server.exe program in Windows.

JeroMiya · 2017-04-05T22:24:56Z

+        log->debug("Verbose logging enabled.");
+    }
+
+    configName = values["config"].as<std::string>();


what does this line do if no command line arguments are passed?

@JeroMiya If no configuration file is specified, it will check to see if the file returned by osvr::server::getDefaultConfigFilename() exists and is readable. If it is, then we'll use it. Otherwise, we'll start the server with an empty { } configuration.

JeroMiya · 2017-04-05T22:29:40Z

-        log->info()
-            << "Using default config file - pass a filename on the command "
-               "line to use a different one.";
+        server = osvr::server::configureServerFromString("{ }");


We need to also check for a file called osvr::server::getDefaultConfigFilename() in the current working directory, so that we don't break server auto-start and double-click behavior (i.e. starting with no arguments should load the default config file name from the CWD). Only if the config is not specified, and the default-named config file is not found in the CWD, should we be using a blank config as is done here.

If no configuration file is specified on the command line, we assume the user specified the filename returned by osvr::server::getDefaultConfigFilename(). If that file exists, we'll use it. If it doesn't, then we'll start the server with an empty { } configuration.

godbyk · 2017-04-05T22:50:51Z

If no configuration file is specified, we default to using osvr::server::getDefaultConfigFilename(). If that fails (e.g., that file doesn't exist), then we'll use an empty { } configuration.

gfrolov

Just a couple clarifications are needed for my comments, but otherwise it looks good

gfrolov · 2017-04-06T18:48:21Z

+        configPath = boost::none;
+    }
+
+    if (configPath) {


I'm assuming this check fails if you provide an empty string to boost::optional<fs::path> configPath(configName) on line 111 ?

@mars979 I think so, since I would expect fs::exists() to return false on an empty string.

Ok, sounds good.

gfrolov · 2017-04-06T18:50:19Z

+            srvConfig.loadConfig(json);
            ret = srvConfig.constructServer();
        } catch (std::exception &e) {
            log->error()


Should we log the std::exception error here ?

@mars979 We are logging the exception here. (Or are you suggesting we shouldn't log it?)

@godbyk , I meant to ask if log->error() automatically logs the std::exception &e or do you need to explicitly add log->error(e.what()) (or something similar)

@mars979 Ah, gotcha. We're logging it a couple lines down. (clang-format's line-wrapping hinders the readability of the line.)

Ah, it was just wrapped to next line. My bad.

Outurnate and others added 6 commits November 24, 2015 19:10

osvr_server support for cmdline options and blank config generation

4f79020

Change --config to positional option; remove from help; added safety …

d2bbb3b

…check on creating config file

Split up config reading and loading. Server now uses blank config in …

cea5f20

…absense of config file

Merge branch 'master' of https://github.com/Outurnate/OSVR-Core into …

3edd956

…logging-improvements

Removed unused code; added missing logger instantiation.

33a3984

Added verbose and debug logging command-line options to osvr_server.

130c9d4

godbyk added the in progress label Mar 9, 2017

Appease Windows. Export symbols. Work around lousy C++11 support.

1cea73b

JeroMiya requested changes Apr 5, 2017

View reviewed changes

JeroMiya approved these changes Apr 6, 2017

View reviewed changes

gfrolov requested changes Apr 6, 2017

View reviewed changes

gfrolov approved these changes Apr 6, 2017

View reviewed changes

JeroMiya mentioned this pull request Apr 6, 2017

User directory config files #534

Merged

godbyk merged commit 80a44d6 into OSVR:master Apr 12, 2017

godbyk removed the in progress label Apr 12, 2017

godbyk deleted the logging-improvements branch April 12, 2017 23:01

Conversation

godbyk commented Mar 9, 2017

Uh oh!

JeroMiya 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!

godbyk commented Apr 5, 2017

Uh oh!

gfrolov 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants