From d792491c405ce47bcd779f53fa0f9422bb3df494 Mon Sep 17 00:00:00 2001 From: Tobias Hunger Date: Thu, 27 Oct 2016 11:34:45 +0200 Subject: [PATCH] cmake-server: Better error reporting during handshake Catch more problematic input during handshake and report failure. These were caught before when trying to configure, but it is way better to get these reports early. --- Source/cmServerProtocol.cxx | 133 +++++++++++++++++---------------- Tests/Server/tc_handshake.json | 4 +- 2 files changed, 71 insertions(+), 66 deletions(-) diff --git a/Source/cmServerProtocol.cxx b/Source/cmServerProtocol.cxx index a2bdf49e2..f3ecfaa79 100644 --- a/Source/cmServerProtocol.cxx +++ b/Source/cmServerProtocol.cxx @@ -244,6 +244,30 @@ std::pair cmServerProtocol1_0::ProtocolVersion() const return std::make_pair(1, 0); } +static void setErrorMessage(std::string* errorMessage, const std::string& text) +{ + if (errorMessage) { + *errorMessage = text; + } +} + +static bool testValue(cmState* state, const std::string& key, + std::string& value, const std::string& keyDescription, + std::string* errorMessage) +{ + const std::string cachedValue = std::string(state->GetCacheEntryValue(key)); + if (!cachedValue.empty() && !value.empty() && cachedValue != value) { + setErrorMessage(errorMessage, std::string("\"") + key + + "\" is set but incompatible with configured " + + keyDescription + "value."); + return false; + } + if (value.empty()) { + value = cachedValue; + } + return true; +} + bool cmServerProtocol1_0::DoActivate(const cmServerRequest& request, std::string* errorMessage) { @@ -254,19 +278,16 @@ bool cmServerProtocol1_0::DoActivate(const cmServerRequest& request, std::string extraGenerator = request.Data[kEXTRA_GENERATOR_KEY].asString(); if (buildDirectory.empty()) { - if (errorMessage) { - *errorMessage = - std::string("\"") + kBUILD_DIRECTORY_KEY + "\" is missing."; - } + setErrorMessage(errorMessage, std::string("\"") + kBUILD_DIRECTORY_KEY + + "\" is missing."); return false; } + cmake* cm = CMakeInstance(); if (cmSystemTools::PathExists(buildDirectory)) { if (!cmSystemTools::FileIsDirectory(buildDirectory)) { - if (errorMessage) { - *errorMessage = std::string("\"") + kBUILD_DIRECTORY_KEY + - "\" exists but is not a directory."; - } + setErrorMessage(errorMessage, std::string("\"") + kBUILD_DIRECTORY_KEY + + "\" exists but is not a directory."); return false; } @@ -275,77 +296,62 @@ bool cmServerProtocol1_0::DoActivate(const cmServerRequest& request, cmState* state = cm->GetState(); // Check generator: - const std::string cachedGenerator = - std::string(state->GetCacheEntryValue("CMAKE_GENERATOR")); - if (cachedGenerator.empty() && generator.empty()) { - if (errorMessage) { - *errorMessage = - std::string("\"") + kGENERATOR_KEY + "\" is required but unset."; - } - return false; - } - if (generator.empty()) { - generator = cachedGenerator; - } - if (generator != cachedGenerator) { - if (errorMessage) { - *errorMessage = std::string("\"") + kGENERATOR_KEY + - "\" set but incompatible with configured generator."; - } + if (!testValue(state, "CMAKE_GENERATOR", generator, "generator", + errorMessage)) { return false; } // check extra generator: - const std::string cachedExtraGenerator = - std::string(state->GetCacheEntryValue("CMAKE_EXTRA_GENERATOR")); - if (!cachedExtraGenerator.empty() && !extraGenerator.empty() && - cachedExtraGenerator != extraGenerator) { - if (errorMessage) { - *errorMessage = std::string("\"") + kEXTRA_GENERATOR_KEY + - "\" is set but incompatible with configured extra generator."; - } + if (!testValue(state, "CMAKE_EXTRA_GENERATOR", extraGenerator, + "extra generator", errorMessage)) { return false; } - if (extraGenerator.empty()) { - extraGenerator = cachedExtraGenerator; - } // check sourcedir: - const std::string cachedSourceDirectory = - std::string(state->GetCacheEntryValue("CMAKE_HOME_DIRECTORY")); - if (!cachedSourceDirectory.empty() && !sourceDirectory.empty() && - cachedSourceDirectory != sourceDirectory) { - if (errorMessage) { - *errorMessage = std::string("\"") + kSOURCE_DIRECTORY_KEY + - "\" is set but incompatible with configured source directory."; - } + if (!testValue(state, "CMAKE_HOME_DIRECTORY", sourceDirectory, + "source directory", errorMessage)) { return false; } - if (sourceDirectory.empty()) { - sourceDirectory = cachedSourceDirectory; - } } } if (sourceDirectory.empty()) { - if (errorMessage) { - *errorMessage = std::string("\"") + kSOURCE_DIRECTORY_KEY + - "\" is unset but required."; - } + setErrorMessage(errorMessage, std::string("\"") + kSOURCE_DIRECTORY_KEY + + "\" is unset but required."); return false; } if (!cmSystemTools::FileIsDirectory(sourceDirectory)) { - if (errorMessage) { - *errorMessage = - std::string("\"") + kSOURCE_DIRECTORY_KEY + "\" is not a directory."; - } + setErrorMessage(errorMessage, std::string("\"") + kSOURCE_DIRECTORY_KEY + + "\" is not a directory."); return false; } if (generator.empty()) { - if (errorMessage) { - *errorMessage = - std::string("\"") + kGENERATOR_KEY + "\" is unset but required."; - } + setErrorMessage(errorMessage, std::string("\"") + kGENERATOR_KEY + + "\" is unset but required."); + return false; + } + + std::vector generators; + cm->GetRegisteredGenerators(generators); + auto baseIt = std::find_if(generators.begin(), generators.end(), + [&generator](const cmake::GeneratorInfo& info) { + return info.name == generator; + }); + if (baseIt == generators.end()) { + setErrorMessage(errorMessage, std::string("Generator \"") + generator + + "\" not supported."); + return false; + } + auto extraIt = std::find_if( + generators.begin(), generators.end(), + [&generator, &extraGenerator](const cmake::GeneratorInfo& info) { + return info.baseName == generator && info.extraName == extraGenerator; + }); + if (extraIt == generators.end()) { + setErrorMessage(errorMessage, + std::string("The combination of generator \"" + generator + + "\" and extra generator \"" + extraGenerator + + "\" is not supported.")); return false; } @@ -355,11 +361,10 @@ bool cmServerProtocol1_0::DoActivate(const cmServerRequest& request, cmGlobalGenerator* gg = cm->CreateGlobalGenerator(fullGeneratorName); if (!gg) { - if (errorMessage) { - *errorMessage = - std::string("Could not set up the requested combination of \"") + - kGENERATOR_KEY + "\" and \"" + kEXTRA_GENERATOR_KEY + "\""; - } + setErrorMessage( + errorMessage, + std::string("Could not set up the requested combination of \"") + + kGENERATOR_KEY + "\" and \"" + kEXTRA_GENERATOR_KEY + "\""); return false; } diff --git a/Tests/Server/tc_handshake.json b/Tests/Server/tc_handshake.json index 5261581c5..975bb3d32 100644 --- a/Tests/Server/tc_handshake.json +++ b/Tests/Server/tc_handshake.json @@ -59,10 +59,10 @@ { "recv": {"cookie":"zimtstern","inReplyTo":"handshake","type":"error","errorMessage":"Failed to activate protocol version: \"generator\" is unset but required."} }, { "send": {"cookie":"zimtstern","type": "handshake","protocolVersion":{"major":1},"sourceDirectory":".","buildDirectory":"/tmp/build","generator":"XXXX","extraGenerator":"CodeBlocks"} }, -{ "recv": {"cookie":"zimtstern","inReplyTo":"handshake","type":"error","errorMessage":"Failed to activate protocol version: Could not set up the requested combination of \"generator\" and \"extraGenerator\""} }, +{ "recv": {"cookie":"zimtstern","inReplyTo":"handshake","type":"error","errorMessage":"Failed to activate protocol version: Generator \"XXXX\" not supported."} }, { "send": {"cookie":"zimtstern","type": "handshake","protocolVersion":{"major":1},"sourceDirectory":".","buildDirectory":"/tmp/build","generator":"Ninja","extraGenerator":"XXXX"} }, -{ "recv": {"cookie":"zimtstern","inReplyTo":"handshake","type":"error","errorMessage":"Failed to activate protocol version: Could not set up the requested combination of \"generator\" and \"extraGenerator\""} }, +{ "recv": {"cookie":"zimtstern","inReplyTo":"handshake","type":"error","errorMessage":"Failed to activate protocol version: The combination of generator \"Ninja\" and extra generator \"XXXX\" is not supported."} }, { "send": {"cookie":"zimtstern","type": "handshake","protocolVersion":{"major":1},"sourceDirectory":".","buildDirectory":"/tmp/build","generator":"Ninja","extraGenerator":"CodeBlocks"} }, { "recv": {"cookie":"zimtstern","inReplyTo":"handshake","type":"reply"} },