From ff503740ce8c0f6d44d0b5df555f5f60325994a2 Mon Sep 17 00:00:00 2001 From: Eike Ziller Date: Mon, 12 Aug 2019 14:44:29 +0200 Subject: [PATCH] Run CMake via canonical path instead of warning Running CMake via a non-canonical path (including '..' in the path, or via a symlink) can result in funny errors. de6faa0f15b265d4b6c85135a2c28c881971b46b added a warning for that condition. Unfortunately the auto-detection of CMake can return a path to a symlink (e.g. for CMake from brew on macOS). Also it is helpful to use a symlink to e.g. manage different CMake versions. Instead of warning about the condition, and forcing the user to resolve it manually, simply run CMake via its canonical path when actually running it from Qt Creator. Change-Id: I95623b45c5436a6d61c1419b7aba23e2a73a0650 Reviewed-by: Tobias Hunger --- .../cmakekitinformation.cpp | 7 ---- .../cmakeprojectmanager/cmakesettingspage.cpp | 15 +++------ src/plugins/cmakeprojectmanager/cmaketool.cpp | 32 ++++++------------- src/plugins/cmakeprojectmanager/cmaketool.h | 8 ++--- .../cmaketoolsettingsaccessor.cpp | 2 +- 5 files changed, 16 insertions(+), 48 deletions(-) diff --git a/src/plugins/cmakeprojectmanager/cmakekitinformation.cpp b/src/plugins/cmakeprojectmanager/cmakekitinformation.cpp index 9dfdd03ddce..8c4ee8877d4 100644 --- a/src/plugins/cmakeprojectmanager/cmakekitinformation.cpp +++ b/src/plugins/cmakeprojectmanager/cmakekitinformation.cpp @@ -257,13 +257,6 @@ Tasks CMakeKitAspect::validate(const Kit *k) const "version 3.0 or later.").arg(QString::fromUtf8(version.fullVersion)), Utils::FilePath(), -1, Core::Id(ProjectExplorer::Constants::TASK_CATEGORY_BUILDSYSTEM)); } - if (!tool->isExecutablePathCanonical()) { - result << Task(Task::Warning, - CMakeTool::nonCanonicalPathToCMakeExecutableWarningMessage(), - Utils::FilePath(), - -1, - Core::Id(ProjectExplorer::Constants::TASK_CATEGORY_BUILDSYSTEM)); - } } return result; } diff --git a/src/plugins/cmakeprojectmanager/cmakesettingspage.cpp b/src/plugins/cmakeprojectmanager/cmakesettingspage.cpp index d0420851113..53be9710379 100644 --- a/src/plugins/cmakeprojectmanager/cmakesettingspage.cpp +++ b/src/plugins/cmakeprojectmanager/cmakesettingspage.cpp @@ -96,7 +96,7 @@ public: CMakeToolTreeItem(const CMakeTool *item, bool changed) : m_id(item->id()) , m_name(item->displayName()) - , m_executable(item->cmakeExecutable()) + , m_executable(item->filePath()) , m_isAutoRun(item->isAutoRun()) , m_autoCreateBuildDirectory(item->autoCreateBuildDirectory()) , m_autodetected(item->isAutoDetected()) @@ -130,7 +130,6 @@ public: m_pathExists = fi.exists(); m_pathIsFile = fi.isFile(); m_pathIsExecutable = fi.isExecutable(); - m_pathIsCanonical = CMakeTool::isCanonicalPath(m_executable); } CMakeToolTreeItem() = default; @@ -175,8 +174,6 @@ public: error = QCoreApplication::translate( "CMakeProjectManager::Internal::CMakeToolTreeItem", "CMake executable path is not executable."); - } else if (!m_pathIsCanonical) { - error = CMakeTool::nonCanonicalPathToCMakeExecutableWarningMessage(); } if (result.isEmpty() || error.isEmpty()) return QString("%1%2").arg(result).arg(error); @@ -190,9 +187,6 @@ public: const bool hasError = !m_pathExists || !m_pathIsFile || !m_pathIsExecutable; if (hasError) return Utils::Icons::CRITICAL.icon(); - const bool hasWarning = !m_pathIsCanonical; - if (hasWarning) - return Utils::Icons::WARNING.icon(); return QVariant(); } } @@ -207,7 +201,6 @@ public: bool m_pathExists = false; bool m_pathIsFile = false; bool m_pathIsExecutable = false; - bool m_pathIsCanonical = false; bool m_autoCreateBuildDirectory = false; bool m_autodetected = false; bool m_changed = true; @@ -272,7 +265,7 @@ void CMakeToolItemModel::reevaluateChangedFlag(CMakeToolTreeItem *item) const { CMakeTool *orig = CMakeToolManager::findById(item->m_id); item->m_changed = !orig || orig->displayName() != item->m_name - || orig->cmakeExecutable() != item->m_executable; + || orig->filePath() != item->m_executable; //make sure the item is marked as changed when the default cmake was changed CMakeTool *origDefTool = CMakeToolManager::defaultCMakeTool(); @@ -333,7 +326,7 @@ void CMakeToolItemModel::apply() item->m_changed = false; if (CMakeTool *cmake = CMakeToolManager::findById(item->m_id)) { cmake->setDisplayName(item->m_name); - cmake->setCMakeExecutable(item->m_executable); + cmake->setFilePath(item->m_executable); cmake->setAutorun(item->m_isAutoRun); cmake->setAutoCreateBuildDirectory(item->m_autoCreateBuildDirectory); } else { @@ -346,7 +339,7 @@ void CMakeToolItemModel::apply() : CMakeTool::ManualDetection; auto cmake = std::make_unique(detection, item->m_id); cmake->setDisplayName(item->m_name); - cmake->setCMakeExecutable(item->m_executable); + cmake->setFilePath(item->m_executable); if (!CMakeToolManager::registerCMakeTool(std::move(cmake))) item->m_changed = true; } diff --git a/src/plugins/cmakeprojectmanager/cmaketool.cpp b/src/plugins/cmakeprojectmanager/cmaketool.cpp index 695ae5c483e..0311f967727 100644 --- a/src/plugins/cmakeprojectmanager/cmaketool.cpp +++ b/src/plugins/cmakeprojectmanager/cmaketool.cpp @@ -145,7 +145,7 @@ CMakeTool::CMakeTool(const QVariantMap &map, bool fromSdk) : if (!fromSdk) m_isAutoDetected = map.value(CMAKE_INFORMATION_AUTODETECTED, false).toBool(); - setCMakeExecutable(Utils::FilePath::fromString(map.value(CMAKE_INFORMATION_COMMAND).toString())); + setFilePath(Utils::FilePath::fromString(map.value(CMAKE_INFORMATION_COMMAND).toString())); } CMakeTool::~CMakeTool() = default; @@ -155,7 +155,7 @@ Core::Id CMakeTool::createId() return Core::Id::fromString(QUuid::createUuid().toString()); } -void CMakeTool::setCMakeExecutable(const Utils::FilePath &executable) +void CMakeTool::setFilePath(const Utils::FilePath &executable) { if (m_executable == executable) return; @@ -167,6 +167,11 @@ void CMakeTool::setCMakeExecutable(const Utils::FilePath &executable) CMakeToolManager::notifyAboutUpdate(this); } +Utils::FilePath CMakeTool::filePath() const +{ + return m_executable; +} + void CMakeTool::setAutorun(bool autoRun) { if (m_isAutoRun == autoRun) @@ -238,9 +243,9 @@ Utils::FilePath CMakeTool::cmakeExecutable() const if (Utils::HostOsInfo::isMacHost() && m_executable.endsWith(".app")) { const Utils::FilePath toTest = m_executable.pathAppended("Contents/bin/cmake"); if (toTest.exists()) - return toTest; + return toTest.canonicalPath(); } - return m_executable; + return m_executable.canonicalPath(); } bool CMakeTool::isAutoRun() const @@ -356,25 +361,6 @@ CMakeTool::ReaderType CMakeTool::readerType() const return m_readerType.value(); } -bool CMakeTool::isCanonicalPath(const Utils::FilePath &path) -{ - const QString canonicalPath = path.toFileInfo().canonicalFilePath(); - return canonicalPath == path.toString(); -} - -bool CMakeTool::isExecutablePathCanonical() const -{ - return isCanonicalPath(cmakeExecutable()); -} - -QString CMakeTool::nonCanonicalPathToCMakeExecutableWarningMessage() -{ - return QCoreApplication::translate( - "CMakeProjectManager::CMakeTool", - "CMake executable path is not canonical and contains \"..\", \".\" " - "or a symbolic link. This might trigger bugs in CMake."); -} - void CMakeTool::readInformation(CMakeTool::QueryType type) const { if ((type == QueryType::GENERATORS && !m_introspection->m_generators.isEmpty()) diff --git a/src/plugins/cmakeprojectmanager/cmaketool.h b/src/plugins/cmakeprojectmanager/cmaketool.h index ef39f0f0d38..ab24ed4e416 100644 --- a/src/plugins/cmakeprojectmanager/cmaketool.h +++ b/src/plugins/cmakeprojectmanager/cmaketool.h @@ -89,10 +89,11 @@ public: Core::Id id() const { return m_id; } QVariantMap toMap () const; - void setCMakeExecutable(const Utils::FilePath &executable); void setAutorun(bool autoRun); void setAutoCreateBuildDirectory(bool autoBuildDir); + void setFilePath(const Utils::FilePath &executable); + Utils::FilePath filePath() const; Utils::FilePath cmakeExecutable() const; bool isAutoRun() const; bool autoCreateBuildDirectory() const; @@ -112,11 +113,6 @@ public: ReaderType readerType() const; - static bool isCanonicalPath(const Utils::FilePath &path); - bool isExecutablePathCanonical() const; - - static QString nonCanonicalPathToCMakeExecutableWarningMessage(); - private: enum class QueryType { GENERATORS, diff --git a/src/plugins/cmakeprojectmanager/cmaketoolsettingsaccessor.cpp b/src/plugins/cmakeprojectmanager/cmaketoolsettingsaccessor.cpp index a34f4633189..b126ded5f7c 100644 --- a/src/plugins/cmakeprojectmanager/cmaketoolsettingsaccessor.cpp +++ b/src/plugins/cmakeprojectmanager/cmaketoolsettingsaccessor.cpp @@ -110,7 +110,7 @@ static std::vector> autoDetectCMakeTools() std::vector> found; foreach (const FilePath &command, suspects) { auto item = std::make_unique(CMakeTool::AutoDetection, CMakeTool::createId()); - item->setCMakeExecutable(command); + item->setFilePath(command); item->setDisplayName(CMakeToolManager::tr("System CMake at %1").arg(command.toUserOutput())); found.emplace_back(std::move(item));