Skip to content

Added osvr_server command-line options #529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Apr 12, 2017
Merged

Added osvr_server command-line options #529

merged 7 commits into from
Apr 12, 2017

Conversation

godbyk
Copy link
Contributor

@godbyk godbyk commented Mar 9, 2017

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.

JeroMiya
JeroMiya requested changes Apr 5, 2017
View reviewed changes
Copy link
Contributor

@JeroMiya JeroMiya left a comment

Choose a reason for hiding this comment

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

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.

apps/osvr_server.cpp
opt::options_description optionsVisible("Command Line Options");
opt::positional_options_description optionsPositional;

optionsPositional.add("config", -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

apps/osvr_server.cpp
log->debug("Verbose logging enabled.");
}

configName = values["config"].as<std::string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

godbyk commented Apr 5, 2017

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
gfrolov requested changes Apr 6, 2017
View reviewed changes
Copy link
Contributor

@gfrolov gfrolov left a comment

Choose a reason for hiding this comment

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

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

apps/osvr_server.cpp
configPath = boost::none;
}

if (configPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, sounds good.

inc/osvr/Server/ConfigureServerFromFile.h
srvConfig.loadConfig(json);
ret = srvConfig.constructServer();
} catch (std::exception &e) {
log->error()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log the std::exception error here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@JeroMiya JeroMiya mentioned this pull request Apr 6, 2017
Merged
@godbyk godbyk merged commit 80a44d6 into OSVR:master Apr 12, 2017
@godbyk godbyk deleted the logging-improvements branch April 12, 2017 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JeroMiya JeroMiya JeroMiya approved these changes

@gfrolov gfrolov gfrolov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@godbyk @JeroMiya @gfrolov @Outurnate