From 166efd601f2536a1bf53634073d4ae4e10ea8c0d Mon Sep 17 00:00:00 2001 From: arostovtsev Date: Thu, 22 Aug 2019 17:40:11 -0700 Subject: [PATCH] Improve error handling for missing/invalid server JVM. Tell the user if the error was caused by a bad --server_javabase option, and return a standard exit code. PiperOrigin-RevId: 264957762 --- src/main/cpp/startup_options.cc | 83 +++++++++++++++---- src/main/cpp/startup_options.h | 34 +++++++- .../shell/integration/bazel_javabase_test.sh | 1 + 3 files changed, 97 insertions(+), 21 deletions(-) diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc index 602f0f843c72e6..e59e3701e5229b 100644 --- a/src/main/cpp/startup_options.cc +++ b/src/main/cpp/startup_options.cc @@ -492,16 +492,19 @@ string StartupOptions::GetEmbeddedJavabase() const { return ""; } -string StartupOptions::GetServerJavabase() const { +std::pair +StartupOptions::GetServerJavabaseAndType() const { // 1) Allow overriding the server_javabase via --server_javabase. if (!explicit_server_javabase_.empty()) { - return explicit_server_javabase_; + return std::pair(explicit_server_javabase_, + JavabaseType::EXPLICIT); } - if (default_server_javabase_.empty()) { + if (default_server_javabase_.first.empty()) { string bundled_jre_path = GetEmbeddedJavabase(); if (!bundled_jre_path.empty()) { // 2) Use a bundled JVM if we have one. - default_server_javabase_ = bundled_jre_path; + default_server_javabase_ = std::pair( + bundled_jre_path, JavabaseType::EMBEDDED); } else { // 3) Otherwise fall back to using the default system JVM. string system_javabase = GetSystemJavabase(); @@ -510,19 +513,67 @@ string StartupOptions::GetServerJavabase() const { << "Could not find system javabase. Ensure JAVA_HOME is set, or " "javac is on your PATH."; } - default_server_javabase_ = system_javabase; + default_server_javabase_ = std::pair( + system_javabase, JavabaseType::SYSTEM); } } return default_server_javabase_; } +string StartupOptions::GetServerJavabase() const { + return GetServerJavabaseAndType().first; +} + string StartupOptions::GetExplicitServerJavabase() const { return explicit_server_javabase_; } string StartupOptions::GetJvm() const { - string java_program = - blaze_util::JoinPath(GetServerJavabase(), GetJavaBinaryUnderJavabase()); + auto javabase_and_type = GetServerJavabaseAndType(); + blaze_exit_code::ExitCode sanity_check_code = + SanityCheckJavabase(javabase_and_type.first, javabase_and_type.second); + if (sanity_check_code != blaze_exit_code::SUCCESS) { + exit(sanity_check_code); + } + return blaze_util::JoinPath(javabase_and_type.first, + GetJavaBinaryUnderJavabase()); +} + +// Prints an appropriate error message and returns an appropriate error exit +// code for a server javabase which failed sanity checks. +static blaze_exit_code::ExitCode BadServerJavabaseError( + StartupOptions::JavabaseType javabase_type, + const std::map &option_sources) { + switch (javabase_type) { + case StartupOptions::JavabaseType::EXPLICIT: { + auto source = option_sources.find("server_javabase"); + string rc_file; + if (source != option_sources.end() && !source->second.empty()) { + rc_file = source->second; + } + BAZEL_LOG(ERROR) + << " The java path was specified by a '--server_javabase' option " + + (rc_file.empty() ? "on the command line" : "in " + rc_file); + return blaze_exit_code::BAD_ARGV; + } + case StartupOptions::JavabaseType::EMBEDDED: + BAZEL_LOG(ERROR) << " Internal error: embedded JDK fails sanity check."; + return blaze_exit_code::INTERNAL_ERROR; + case StartupOptions::JavabaseType::SYSTEM: + return blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR; + default: + BAZEL_LOG(ERROR) + << " Internal error: server javabase type was not initialized."; + // Fall through. + } + return blaze_exit_code::INTERNAL_ERROR; +} + +blaze_exit_code::ExitCode StartupOptions::SanityCheckJavabase( + const std::string &javabase, + StartupOptions::JavabaseType javabase_type) const { + std::string java_program = + blaze_util::JoinPath(javabase, GetJavaBinaryUnderJavabase()); if (!blaze_util::CanExecuteFile(java_program)) { if (!blaze_util::PathExists(java_program)) { BAZEL_LOG(ERROR) << "Couldn't find java at '" << java_program << "'."; @@ -531,27 +582,25 @@ string StartupOptions::GetJvm() const { << "' exists but is not executable: " << blaze_util::GetLastErrorString(); } - exit(1); + return BadServerJavabaseError(javabase_type, option_sources); } // If the full JDK is installed - string jdk_rt_jar = - blaze_util::JoinPath(GetServerJavabase(), "jre/lib/rt.jar"); + string jdk_rt_jar = blaze_util::JoinPath(javabase, "jre/lib/rt.jar"); // If just the JRE is installed - string jre_rt_jar = blaze_util::JoinPath(GetServerJavabase(), "lib/rt.jar"); + string jre_rt_jar = blaze_util::JoinPath(javabase, "lib/rt.jar"); // rt.jar does not exist in java 9+ so check for java instead - string jre_java = blaze_util::JoinPath(GetServerJavabase(), "bin/java"); - string jre_java_exe = - blaze_util::JoinPath(GetServerJavabase(), "bin/java.exe"); + string jre_java = blaze_util::JoinPath(javabase, "bin/java"); + string jre_java_exe = blaze_util::JoinPath(javabase, "bin/java.exe"); if (blaze_util::CanReadFile(jdk_rt_jar) || blaze_util::CanReadFile(jre_rt_jar) || blaze_util::CanReadFile(jre_java) || blaze_util::CanReadFile(jre_java_exe)) { - return java_program; + return blaze_exit_code::SUCCESS; } BAZEL_LOG(ERROR) << "Problem with java installation: couldn't find/access " "rt.jar or java in " - << GetServerJavabase(); - exit(1); + << javabase; + return BadServerJavabaseError(javabase_type, option_sources); } string StartupOptions::GetExe(const string &jvm, const string &jar_path) const { diff --git a/src/main/cpp/startup_options.h b/src/main/cpp/startup_options.h index 0abb4736cd63ac..4bc1caf09ed578 100644 --- a/src/main/cpp/startup_options.h +++ b/src/main/cpp/startup_options.h @@ -51,6 +51,8 @@ struct RcStartupFlag { // which displays the defaults). The actual defaults are defined // in the constructor. // +// Note that this class is not thread-safe. +// // TODO(bazel-team): The encapsulation is not quite right -- there are some // places in blaze.cc where some of these fields are explicitly modified. Their // names also don't conform to the style guide. @@ -188,8 +190,23 @@ class StartupOptions { // Returns the embedded JDK, or an empty string. std::string GetEmbeddedJavabase() const; - // Returns the GetHostJavabase. This should be called after parsing - // the --server_javabase option. + // The source of truth for the server javabase. + enum class JavabaseType { + UNKNOWN, + // An explicit --server_javabase startup option. + EXPLICIT, + // The embedded JDK. + EMBEDDED, + // The default system JVM. + SYSTEM + }; + + // Returns the server javabase and its source of truth. This should be called + // after parsing the --server_javabase option. + std::pair GetServerJavabaseAndType() const; + + // Returns the server javabase. This should be called after parsing the + // --server_javabase option. std::string GetServerJavabase() const; // Returns the explicit value of the --server_javabase startup option or the @@ -256,6 +273,14 @@ class StartupOptions { const char *arg, const char *next_arg, const std::string &rcfile, const char **value, bool *is_processed, std::string *error) = 0; + // Checks whether the given javabase contains a java executable and runtime. + // On success, returns blaze_exit_code::SUCCESS. On error, prints an error + // message and returns an appropriate exit code with which the client should + // terminate. + blaze_exit_code::ExitCode SanityCheckJavabase( + const std::string &javabase, + StartupOptions::JavabaseType javabase_type) const; + // Returns the absolute path to the user's local JDK install, to be used as // the default target javabase and as a fall-back host_javabase. This is not // the embedded JDK. @@ -304,8 +329,9 @@ class StartupOptions { // The server javabase as provided on the commandline. std::string explicit_server_javabase_; - // The server javabase to be used (computed lazily). - mutable std::string default_server_javabase_; + // The default server javabase to be used and its source of truth (computed + // lazily). Not guarded by a mutex - StartupOptions is not thread-safe. + mutable std::pair default_server_javabase_; // Startup flags that don't expect a value, e.g. "master_bazelrc". // Valid uses are "--master_bazelrc" are "--nomaster_bazelrc". diff --git a/src/test/shell/integration/bazel_javabase_test.sh b/src/test/shell/integration/bazel_javabase_test.sh index 5a79c00915707e..e71302d92d8567 100755 --- a/src/test/shell/integration/bazel_javabase_test.sh +++ b/src/test/shell/integration/bazel_javabase_test.sh @@ -78,6 +78,7 @@ function test_fallback_depot_javabase() { fi if ! bazel --batch --server_javabase=$path version >& $TEST_log; then expect_log "Couldn't find java at" + expect_log "--server_javabase" expect_not_log "Problem with java installation" fi }