From d792491c405ce47bcd779f53fa0f9422bb3df494 Mon Sep 17 00:00:00 2001 From: Tobias Hunger Date: Thu, 27 Oct 2016 11:34:45 +0200 Subject: [PATCH 1/2] 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"} }, From 42ccbee11c1b3be12c5ebb9f69cb769d79d491ad Mon Sep 17 00:00:00 2001 From: Tobias Hunger Date: Thu, 27 Oct 2016 11:48:31 +0200 Subject: [PATCH 2/2] server-mode: Handle generator toolset and platform in handshake --- Help/manual/cmake-server.7.rst | 4 +++- Source/cmServerDictionary.h | 2 ++ Source/cmServerProtocol.cxx | 31 ++++++++++++++++++++++++++++++- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/Help/manual/cmake-server.7.rst b/Help/manual/cmake-server.7.rst index a72af145b..9520cc140 100644 --- a/Help/manual/cmake-server.7.rst +++ b/Help/manual/cmake-server.7.rst @@ -276,7 +276,9 @@ Protocol version 1.0 requires the following attributes to be set: * "sourceDirectory" with a path to the sources * "buildDirectory" with a path to the build directory * "generator" with the generator name - * "extraGenerator" (optional!) with the extra generator to be used. + * "extraGenerator" (optional!) with the extra generator to be used + * "platform" with the generator platform (if supported by the generator) + * "toolset" with the generator toolset (if supported by the generator) Example:: diff --git a/Source/cmServerDictionary.h b/Source/cmServerDictionary.h index 2d64cbdaf..e6a7ae6ca 100644 --- a/Source/cmServerDictionary.h +++ b/Source/cmServerDictionary.h @@ -62,6 +62,7 @@ static const std::string kMESSAGE_KEY = "message"; static const std::string kMINOR_KEY = "minor"; static const std::string kNAME_KEY = "name"; static const std::string kPATH_KEY = "path"; +static const std::string kPLATFORM_KEY = "platform"; static const std::string kPROGRESS_CURRENT_KEY = "progressCurrent"; static const std::string kPROGRESS_MAXIMUM_KEY = "progressMaximum"; static const std::string kPROGRESS_MESSAGE_KEY = "progressMessage"; @@ -77,6 +78,7 @@ static const std::string kSUPPORTED_PROTOCOL_VERSIONS = static const std::string kSYSROOT_KEY = "sysroot"; static const std::string kTARGETS_KEY = "targets"; static const std::string kTITLE_KEY = "title"; +static const std::string kTOOLSET_KEY = "toolset"; static const std::string kTRACE_EXPAND_KEY = "traceExpand"; static const std::string kTRACE_KEY = "trace"; static const std::string kTYPE_KEY = "type"; diff --git a/Source/cmServerProtocol.cxx b/Source/cmServerProtocol.cxx index f3ecfaa79..09b08fe8d 100644 --- a/Source/cmServerProtocol.cxx +++ b/Source/cmServerProtocol.cxx @@ -259,7 +259,7 @@ static bool testValue(cmState* state, const std::string& key, if (!cachedValue.empty() && !value.empty() && cachedValue != value) { setErrorMessage(errorMessage, std::string("\"") + key + "\" is set but incompatible with configured " + - keyDescription + "value."); + keyDescription + " value."); return false; } if (value.empty()) { @@ -276,6 +276,8 @@ bool cmServerProtocol1_0::DoActivate(const cmServerRequest& request, request.Data[kBUILD_DIRECTORY_KEY].asString(); std::string generator = request.Data[kGENERATOR_KEY].asString(); std::string extraGenerator = request.Data[kEXTRA_GENERATOR_KEY].asString(); + std::string toolset = request.Data[kTOOLSET_KEY].asString(); + std::string platform = request.Data[kPLATFORM_KEY].asString(); if (buildDirectory.empty()) { setErrorMessage(errorMessage, std::string("\"") + kBUILD_DIRECTORY_KEY + @@ -312,6 +314,18 @@ bool cmServerProtocol1_0::DoActivate(const cmServerRequest& request, "source directory", errorMessage)) { return false; } + + // check toolset: + if (!testValue(state, "CMAKE_GENERATOR_TOOLSET", toolset, "toolset", + errorMessage)) { + return false; + } + + // check platform: + if (!testValue(state, "CMAKE_GENERATOR_PLATFORM", platform, "platform", + errorMessage)) { + return false; + } } } @@ -354,11 +368,26 @@ bool cmServerProtocol1_0::DoActivate(const cmServerRequest& request, "\" is not supported.")); return false; } + if (!extraIt->supportsToolset && !toolset.empty()) { + setErrorMessage(errorMessage, + std::string("Toolset was provided but is not supported by " + "the requested generator.")); + return false; + } + if (!extraIt->supportsPlatform && !platform.empty()) { + setErrorMessage(errorMessage, + std::string("Platform was provided but is not supported " + "by the requested generator.")); + return false; + } const std::string fullGeneratorName = cmExternalMakefileProjectGenerator::CreateFullGeneratorName( generator, extraGenerator); + cm->SetGeneratorToolset(toolset); + cm->SetGeneratorPlatform(platform); + cmGlobalGenerator* gg = cm->CreateGlobalGenerator(fullGeneratorName); if (!gg) { setErrorMessage(