Skip to content
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

hard crash if one Xpress function not found #116

Merged
merged 1 commit into from
Nov 21, 2023
Merged

hard crash if one Xpress function not found #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions ortools/base/dynamic_library.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include <functional>
#include <stdexcept>
#include <string>
#include <vector>

#include "ortools/base/logging.h"

Expand All @@ -31,7 +30,6 @@
#endif

class DynamicLibrary {
static constexpr size_t kMaxFunctionsNotFound = 10;
public:
DynamicLibrary() : library_handle_(nullptr) {}

Expand Down Expand Up @@ -59,10 +57,6 @@ static constexpr size_t kMaxFunctionsNotFound = 10;

bool LibraryIsLoaded() const { return library_handle_ != nullptr; }

const std::vector<std::string>& FunctionsNotFound() const {
return functions_not_found_;
}

template <typename T>
std::function<T> GetFunction(const char* function_name) {
#if defined(_MSC_VER) || defined(__MINGW32__) || defined(__MINGW64__)
Expand All @@ -73,10 +67,9 @@ static constexpr size_t kMaxFunctionsNotFound = 10;
const void* function_address = dlsym(library_handle_, function_name);
#endif // MinGW.

// We don't really need the full list of missing functions,
// just a few are enough.
if (!function_address && functions_not_found_.size() < kMaxFunctionsNotFound)
functions_not_found_.push_back(function_name);
CHECK(function_address)
<< "Error: could not find function " << std::string(function_name)
<< " in " << library_name_;

return TypeParser<T>::CreateFunction(function_address);
}
Expand All @@ -100,7 +93,6 @@ static constexpr size_t kMaxFunctionsNotFound = 10;
private:
void* library_handle_ = nullptr;
std::string library_name_;
std::vector<std::string> functions_not_found_;

template <typename T>
struct TypeParser {};
Expand Down
26 changes: 10 additions & 16 deletions ortools/xpress/environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ std::function<int(XPRSprob prob, void (XPRS_CC *f_message)(XPRSprob cbprob, void
std::function<int(XPRSprob prob, const char* flags)> XPRSlpoptimize = nullptr;
std::function<int(XPRSprob prob, const char* flags)> XPRSmipoptimize = nullptr;

absl::Status LoadXpressFunctions(DynamicLibrary* xpress_dynamic_library) {
void LoadXpressFunctions(DynamicLibrary* xpress_dynamic_library) {
// This was generated with the parse_header_xpress.py script.
// See the comment at the top of the script.

Expand Down Expand Up @@ -158,15 +158,6 @@ absl::Status LoadXpressFunctions(DynamicLibrary* xpress_dynamic_library) {
xpress_dynamic_library->GetFunction(&XPRSaddcbmessage, "XPRSaddcbmessage");
xpress_dynamic_library->GetFunction(&XPRSlpoptimize, "XPRSlpoptimize");
xpress_dynamic_library->GetFunction(&XPRSmipoptimize, "XPRSmipoptimize");


auto notFound = xpress_dynamic_library->FunctionsNotFound();
if (!notFound.empty()) {
return absl::NotFoundError(absl::StrCat("Could not find the following functions (list may not be exhaustive). [",
absl::StrJoin(notFound, "', '"),
"]. Please make sure that your XPRESS install is up-to-date (>= 9.0.0)."));
}
return absl::OkStatus();
}

void printXpressBanner(bool error) {
Expand All @@ -184,17 +175,18 @@ std::vector<std::string> XpressDynamicLibraryPotentialPaths() {
std::vector<std::string> potential_paths;

// Look for libraries pointed by XPRESSDIR first.
const char* xpress_home_from_env = getenv("XPRESSDIR");
if (xpress_home_from_env != nullptr) {
const char* xpressdir_from_env = getenv("XPRESSDIR");
if (xpressdir_from_env != nullptr) {
LOG(INFO) << "Environment variable XPRESSDIR = " << xpressdir_from_env;
#if defined(_MSC_VER) // Windows
potential_paths.push_back(
absl::StrCat(xpress_home_from_env, "\\bin\\xprs.dll"));
absl::StrCat(xpressdir_from_env, "\\bin\\xprs.dll"));
#elif defined(__APPLE__) // OS X
potential_paths.push_back(
absl::StrCat(xpress_home_from_env, "/lib/libxprs.dylib"));
absl::StrCat(xpressdir_from_env, "/lib/libxprs.dylib"));
#elif defined(__GNUC__) // Linux
potential_paths.push_back(
absl::StrCat(xpress_home_from_env, "/lib/libxprs.so"));
absl::StrCat(xpressdir_from_env, "/lib/libxprs.so"));
#else
LOG(ERROR) << "OS Not recognized by xpress/environment.cc."
<< " You won't be able to use Xpress.";
Expand Down Expand Up @@ -244,7 +236,9 @@ absl::Status LoadXpressDynamicLibrary(std::string& xpresspath) {
}

if (xpress_library.LibraryIsLoaded()) {
xpress_load_status = LoadXpressFunctions(&xpress_library);
LOG(INFO) << "Loading all Xpress functions";
LoadXpressFunctions(&xpress_library);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error handling isn't so great here. Shouldn't we return an absl::status here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot find a way to do it.
LoadXpressFunctions calls DynamicLibrary.GetFunction(const char* function_name)

DynamicLibrary.GetFunction(const char* function_name) executes the CHECK macro that seems to throw a fatal error. I tried to catch it but i probably did not figure out how to do it properly.
Gurobi interface uses the same approach you see here and in the end we can consider that
not having Xpress installed is ok
having the wrong xpress installation is worse

Copy link
Member Author

@sgatto sgatto Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am open to change this behavior but at the moment I cannot see how to do it without touching the dynamic_library.h file

Copy link
Collaborator

Choose a reason for hiding this comment

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

If Google are open to change, it would be easier to suggest an overhaul of dynamic_library.h like we had before

xpress_load_status = absl::OkStatus();
} else {
xpress_load_status = absl::NotFoundError(
absl::StrCat("Could not find the Xpress shared library. Looked in: [",
Expand Down
Loading