Skip to content

Commit

Permalink
Make options with dashes show correctly on option error.
Browse files Browse the repository at this point in the history 
Summary: There is a bug in at least Boost 1.54 where options with dashes `-` in them are
not handled correctly. Basically all but the last substring after the last dash
is returned.

See: facebook#2864

So our error string when an option had an invalid argument was just plain wrong.
Since we can't fix boost very easily and are not planning to upgrade yet, we
workaround it with this fix.

Fixes: facebook#2864

Reviewed By: @paulbiss

Differential Revision: D1681063

Signature: t1:1681063:1416505169:7664cff3c23b8fdfec995c54897eb276d44f92d6
  • Loading branch information
@JoelMarcey @hhvm-bot
JoelMarcey authored and hhvm-bot committed Nov 30, 2014
1 parent 745f82e commit 50ee3f0
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 54 deletions.
36 changes: 32 additions & 4 deletions hphp/compiler/compiler.cpp
View file
Expand Up @@ -55,6 +55,7 @@
#include <boost/program_options/positional_options.hpp>
#include <boost/program_options/variables_map.hpp>
#include <boost/program_options/parsers.hpp>
#include <boost/algorithm/string/replace.hpp>
#include <exception>

using namespace boost::program_options;
Expand Down Expand Up @@ -318,11 +319,38 @@ int prepareOptions(CompilerOptions &po, int argc, char **argv) {
p.add("inputs", -1);
variables_map vm;
try {
store(command_line_parser(argc, argv).options(desc).positional(p).run(),
vm);
notify(vm);
auto opts = command_line_parser(argc, argv).options(desc)
.positional(p).run();
try {
store(opts, vm);
notify(vm);
#if defined(BOOST_VERSION) && BOOST_VERSION <= 105400
} catch (const error_with_option_name &e) {
std::string wrong_name = e.get_option_name();
std::string right_name = get_right_option_name(opts, wrong_name);
std::string message = e.what();
if (right_name != "") {
boost::replace_all(message, wrong_name, right_name);
}
Logger::Error("Error in command line: %s", message.c_str());
cout << desc << "\n";
return -1;
#endif
} catch (const error& e) {
Logger::Error("Error in command line: %s", e.what());
cout << desc << "\n";
return -1;
}
} catch (const unknown_option& e) {
Logger::Error("Error in command line: %s\n\n", e.what());
Logger::Error("Error in command line: %s", e.what());
cout << desc << "\n";
return -1;
} catch (const error& e) {
Logger::Error("Error in command line: %s", e.what());
cout << desc << "\n";
return -1;
} catch (...) {
Logger::Error("Error in command line parsing.");
cout << desc << "\n";
return -1;
}
Expand Down
144 changes: 95 additions & 49 deletions hphp/runtime/base/program-functions.cpp
View file
Expand Up @@ -81,7 +81,7 @@
#include <boost/program_options/options_description.hpp>
#include <boost/program_options/positional_options.hpp>
#include <boost/program_options/variables_map.hpp>
#include <boost/program_options/parsers.hpp>
#include <boost/algorithm/string/replace.hpp>
#include <libgen.h>
#include <oniguruma.h>
#include <signal.h>
Expand Down Expand Up @@ -1105,6 +1105,26 @@ static void set_stack_size() {
}
}

#if defined(BOOST_VERSION) && BOOST_VERSION <= 105400
std::string get_right_option_name(const basic_parsed_options<char>& opts,
std::string& wrong_name) {
// Remove any - from the wrong name for better comparing
// since it will probably come prepended with --
wrong_name.erase(
std::remove(wrong_name.begin(), wrong_name.end(), '-'), wrong_name.end());
for (basic_option<char> opt : opts.options) {
std::string s_opt = opt.string_key;
// We are only dealing with options that have a - in them.
if (s_opt.find("-") != std::string::npos) {
if (s_opt.find(wrong_name) != std::string::npos) {
return s_opt;
}
}
}
return "";
}
#endif

static int execute_program_impl(int argc, char** argv) {
string usage = "Usage:\n\n ";
usage += argv[0];
Expand Down Expand Up @@ -1192,7 +1212,8 @@ static int execute_program_impl(int argc, char** argv) {
// is necessary so that the boost command line parser doesn't choke on
// args intended for the PHP application.
int hhvm_argc = compute_hhvm_argc(desc, argc, argv);

// Need to have a parent try for opts so I can use opts in the catch of
// one of the sub-tries below.
try {
// Invoke the boost command line parser to parse the args for HHVM.
auto opts = command_line_parser(hhvm_argc, argv)
Expand All @@ -1205,61 +1226,86 @@ static int execute_program_impl(int argc, char** argv) {
~command_line_style::allow_sticky &
~command_line_style::long_allow_adjacent)
.run();
// Manually append the args for the PHP application.
int pos = 0;
for (unsigned m = 0; m < opts.options.size(); ++m) {
const auto& bo = opts.options[m];
if (bo.string_key == "arg") {
++pos;
try {
// Manually append the args for the PHP application.
int pos = 0;
for (unsigned m = 0; m < opts.options.size(); ++m) {
const auto& bo = opts.options[m];
if (bo.string_key == "arg") {
++pos;
}
}
}
for (unsigned m = hhvm_argc; m < argc; ++m) {
string str = argv[m];
basic_option<char> bo;
bo.string_key = "arg";
bo.position_key = pos++;
bo.value.push_back(str);
bo.original_tokens.push_back(str);
bo.unregistered = false;
bo.case_insensitive = false;
opts.options.push_back(bo);
}
// Process the options
store(opts, vm);
notify(vm);
if (vm.count("interactive") /* or -a */) {
po.mode = "debug";
}
if (po.mode == "d") po.mode = "debug";
if (po.mode == "s") po.mode = "server";
if (po.mode == "t") po.mode = "translate";
if (po.mode == "") po.mode = "run";
if (po.mode == "daemon" || po.mode == "server" || po.mode == "replay" ||
po.mode == "run" || po.mode == "debug"|| po.mode == "translate") {
set_execution_mode(po.mode);
} else {
Logger::Error("Error in command line: invalid mode: %s", po.mode.c_str());
cout << desc << "\n";
return -1;
}
if (po.config.empty() && !vm.count("no-config")) {
auto default_config_file = "/etc/hhvm/php.ini";
if (access(default_config_file, R_OK) != -1) {
Logger::Verbose("Using default config file: %s", default_config_file);
po.config.push_back(default_config_file);
for (unsigned m = hhvm_argc; m < argc; ++m) {
string str = argv[m];
basic_option<char> bo;
bo.string_key = "arg";
bo.position_key = pos++;
bo.value.push_back(str);
bo.original_tokens.push_back(str);
bo.unregistered = false;
bo.case_insensitive = false;
opts.options.push_back(bo);
}
// Process the options
store(opts, vm);
notify(vm);
if (vm.count("interactive") /* or -a */) {
po.mode = "debug";
}
if (po.mode == "d") po.mode = "debug";
if (po.mode == "s") po.mode = "server";
if (po.mode == "t") po.mode = "translate";
if (po.mode == "") po.mode = "run";
if (po.mode == "daemon" || po.mode == "server" || po.mode == "replay" ||
po.mode == "run" || po.mode == "debug"|| po.mode == "translate") {
set_execution_mode(po.mode);
} else {
Logger::Error("Error in command line: invalid mode: %s",
po.mode.c_str());
cout << desc << "\n";
return -1;
}
default_config_file = "/etc/hhvm/config.hdf";
if (access(default_config_file, R_OK) != -1) {
Logger::Verbose("Using default config file: %s", default_config_file);
po.config.push_back(default_config_file);
if (po.config.empty() && !vm.count("no-config")) {
auto default_config_file = "/etc/hhvm/php.ini";
if (access(default_config_file, R_OK) != -1) {
Logger::Verbose("Using default config file: %s", default_config_file);
po.config.push_back(default_config_file);
}
default_config_file = "/etc/hhvm/config.hdf";
if (access(default_config_file, R_OK) != -1) {
Logger::Verbose("Using default config file: %s", default_config_file);
po.config.push_back(default_config_file);
}
}
// When we upgrade boost, we can remove this and also get rid of the parent
// try statement and move opts back into the original try block
#if defined(BOOST_VERSION) && BOOST_VERSION <= 105400
} catch (const error_with_option_name &e) {
std::string wrong_name = e.get_option_name();
std::string right_name = get_right_option_name(opts, wrong_name);
std::string message = e.what();
if (right_name != "") {
boost::replace_all(message, wrong_name, right_name);
}
Logger::Error("Error in command line: %s", message.c_str());
cout << desc << "\n";
return -1;
#endif
} catch (const error &e) {
Logger::Error("Error in command line: %s", e.what());
cout << desc << "\n";
return -1;
} catch (...) {
Logger::Error("Error in command line.");
cout << desc << "\n";
return -1;
}
} catch (error &e) {
} catch (const error &e) {
Logger::Error("Error in command line: %s", e.what());
cout << desc << "\n";
return -1;
} catch (...) {
Logger::Error("Error in command line.");
Logger::Error("Error in command line parsing.");
cout << desc << "\n";
return -1;
}
Expand Down
14 changes: 13 additions & 1 deletion hphp/runtime/base/program-functions.h
View file
Expand Up @@ -18,6 +18,7 @@
#define incl_HPHP_PROGRAM_FUNCTIONS_H_

#include "hphp/runtime/base/types.h"
#include <boost/program_options/parsers.hpp>

// Needed for compatibility with oniguruma-5.9.4+
#define ONIG_ESCAPE_UCHAR_COLLISION
Expand Down Expand Up @@ -63,6 +64,18 @@ std::string translate_stack(const char *hexencoded,

time_t start_time();

// Boost 1.54 has a bug where it doesn't handle options with - in them as
// it only gives us the string after the last -
// https://github.com/facebook/hhvm/issues/2864
// This won't fix the problem in 100% of cases (e.g. two options are
// used that both end in the same substring. How do you choose?) But
// that should be very rare.
#if defined(BOOST_VERSION) && BOOST_VERSION <= 105400
std::string get_right_option_name(
const boost::program_options::basic_parsed_options<char>& opts,
std::string& wrong_name);
#endif

///////////////////////////////////////////////////////////////////////////////

class ExecutionContext;
Expand Down Expand Up @@ -101,7 +114,6 @@ extern const char* const kCompilerId;

// Helper function for stats tracking with exceptions.
void bump_counter_and_rethrow(bool isPsp);

///////////////////////////////////////////////////////////////////////////////
}

Expand Down

0 comments on commit 50ee3f0

Please sign in to comment.