From a25bbf23c64c912b39bf2020df73c0db97b5bc40 Mon Sep 17 00:00:00 2001 From: Marcus Tillmanns Date: Fri, 26 Jan 2024 10:33:29 +0100 Subject: [PATCH] Utils: Fix error prone default value of appendOrSet It was not readily clear that Environment::appendOrSet/prependOrSet needed a value for "sep", otherwise it would just concat the values without separator. This got apparent when looking at usages of appendOrSet. Instead there are now three options, "Auto", "Colon" or "Semicolon" with the default being "Auto", which determines the separator based on the Environment::OsType. Usages of appendOrSet and prependOrSet are also fixed with this commit. Change-Id: I868e14ce38f50be9cd07292fe3f22663a62d116d Reviewed-by: hjk --- src/libs/utils/environment.cpp | 52 ++++++++++--------- src/libs/utils/environment.h | 34 +++++++----- .../cmakeprojectmanager/presetsmacros.cpp | 6 +-- src/plugins/debugger/cdb/cdbengine.cpp | 5 +- src/plugins/debugger/dap/pydapengine.cpp | 4 +- src/plugins/python/pythonlanguageclient.cpp | 8 +-- .../qmldesigner/puppetenvironmentbuilder.cpp | 6 +-- src/plugins/qmljstools/qmljsmodelmanager.cpp | 4 +- src/plugins/qnx/qnxrunconfiguration.cpp | 8 +-- src/plugins/terminal/shellintegration.cpp | 8 +-- tests/auto/environment/tst_environment.cpp | 6 +-- 11 files changed, 70 insertions(+), 71 deletions(-) diff --git a/src/libs/utils/environment.cpp b/src/libs/utils/environment.cpp index d71d76c8b8a..b1143c7fab4 100644 --- a/src/libs/utils/environment.cpp +++ b/src/libs/utils/environment.cpp @@ -138,7 +138,7 @@ void Environment::appendOrSetPath(const FilePath &value) QTC_CHECK(value.osType() == m_dict.m_osType); if (value.isEmpty()) return; - appendOrSet("PATH", value.nativePath(), OsSpecificAspects::pathListSeparator(osType())); + appendOrSet("PATH", value.nativePath()); } void Environment::prependOrSetPath(const FilePath &value) @@ -146,20 +146,20 @@ void Environment::prependOrSetPath(const FilePath &value) QTC_CHECK(value.osType() == m_dict.m_osType); if (value.isEmpty()) return; - prependOrSet("PATH", value.nativePath(), OsSpecificAspects::pathListSeparator(osType())); + prependOrSet("PATH", value.nativePath()); } void Environment::prependOrSetPath(const QString &directories) { - prependOrSet("PATH", directories, OsSpecificAspects::pathListSeparator(osType())); + prependOrSet("PATH", directories); } -void Environment::appendOrSet(const QString &key, const QString &value, const QString &sep) +void Environment::appendOrSet(const QString &key, const QString &value, PathSeparator sep) { addItem(Item{std::in_place_index_t(), key, value, sep}); } -void Environment::prependOrSet(const QString &key, const QString &value, const QString &sep) +void Environment::prependOrSet(const QString &key, const QString &value, PathSeparator sep) { addItem(Item{std::in_place_index_t(), key, value, sep}); } @@ -167,21 +167,20 @@ void Environment::prependOrSet(const QString &key, const QString &value, const Q void Environment::prependOrSetLibrarySearchPath(const FilePath &value) { QTC_CHECK(value.osType() == osType()); - const QChar sep = OsSpecificAspects::pathListSeparator(osType()); switch (osType()) { case OsTypeWindows: { - prependOrSet("PATH", value.nativePath(), sep); + prependOrSet("PATH", value.nativePath()); break; } case OsTypeMac: { const QString nativeValue = value.nativePath(); - prependOrSet("DYLD_LIBRARY_PATH", nativeValue, sep); - prependOrSet("DYLD_FRAMEWORK_PATH", nativeValue, sep); + prependOrSet("DYLD_LIBRARY_PATH", nativeValue); + prependOrSet("DYLD_FRAMEWORK_PATH", nativeValue); break; } case OsTypeLinux: case OsTypeOtherUnix: { - prependOrSet("LD_LIBRARY_PATH", value.nativePath(), sep); + prependOrSet("LD_LIBRARY_PATH", value.nativePath()); break; } default: @@ -404,12 +403,10 @@ void Environment::prependToPath(const FilePaths &values) m_dict.clear(); for (int i = values.size(); --i >= 0; ) { const FilePath value = values.at(i); - m_changeItems.append(Item{ - std::in_place_index_t(), - QString("PATH"), - value.nativePath(), - value.pathListSeparator() - }); + m_changeItems.append(Item{std::in_place_index_t(), + QString("PATH"), + value.nativePath(), + PathSeparator::Auto}); } } @@ -417,12 +414,10 @@ void Environment::appendToPath(const FilePaths &values) { m_dict.clear(); for (const FilePath &value : values) { - m_changeItems.append(Item{ - std::in_place_index_t(), - QString("PATH"), - value.nativePath(), - value.pathListSeparator() - }); + m_changeItems.append(Item{std::in_place_index_t(), + QString("PATH"), + value.nativePath(), + PathSeparator::Auto}); } } @@ -469,7 +464,7 @@ const NameValueDictionary &Environment::resolved() const m_dict.m_values.insert(DictKey(key, m_dict.nameCaseSensitivity()), {value, true}); } else { // Prepend unless it is already there - const QString toPrepend = value + sep; + const QString toPrepend = value + pathListSeparator(sep); if (!it.value().first.startsWith(toPrepend)) it.value().first.prepend(toPrepend); } @@ -483,7 +478,7 @@ const NameValueDictionary &Environment::resolved() const m_dict.m_values.insert(DictKey(key, m_dict.nameCaseSensitivity()), {value, true}); } else { // Prepend unless it is already there - const QString toAppend = sep + value; + const QString toAppend = pathListSeparator(sep) + value; if (!it.value().first.endsWith(toAppend)) it.value().first.append(toAppend); } @@ -512,6 +507,15 @@ Environment Environment::appliedToEnvironment(const Environment &base) const return res; } +QChar Environment::pathListSeparator(PathSeparator sep) const +{ + if (sep == PathSeparator::Semicolon) + return QLatin1Char(';'); + else if (sep == PathSeparator::Colon) + return QLatin1Char(':'); + return OsSpecificAspects::pathListSeparator(osType()); +} + /*! Returns the value of \a key in \QC's modified system environment. \sa Utils::Environment::systemEnvironment diff --git a/src/libs/utils/environment.h b/src/libs/utils/environment.h index 679b28f0e08..542343aa2a9 100644 --- a/src/libs/utils/environment.h +++ b/src/libs/utils/environment.h @@ -22,6 +22,8 @@ namespace Utils { class QTCREATOR_UTILS_EXPORT Environment final { public: + enum class PathSeparator { Auto, Colon, Semicolon }; + Environment(); explicit Environment(OsType osType); explicit Environment(const QStringList &env, OsType osType = HostOsInfo::hostOs()); @@ -43,8 +45,12 @@ public: QStringList toStringList() const; QProcessEnvironment toProcessEnvironment() const; - void appendOrSet(const QString &key, const QString &value, const QString &sep = QString()); - void prependOrSet(const QString &key, const QString &value, const QString &sep = QString()); + void appendOrSet(const QString &key, + const QString &value, + PathSeparator sep = PathSeparator::Auto); + void prependOrSet(const QString &key, + const QString &value, + PathSeparator sep = PathSeparator::Auto); void appendOrSetPath(const FilePath &value); void prependOrSetPath(const FilePath &value); @@ -89,6 +95,8 @@ public: static void modifySystemEnvironment(const EnvironmentItems &list); // use with care!!! static void setSystemEnvironment(const Environment &environment); // don't use at all!!! + QChar pathListSeparator(PathSeparator sep) const; + enum Type { SetSystemEnvironment, SetFixedDictionary, @@ -102,17 +110,17 @@ public: }; using Item = std::variant< - std::monostate, // SetSystemEnvironment dummy - NameValueDictionary, // SetFixedDictionary - std::tuple, // SetValue (key, value, enabled) - std::tuple, // SetFallbackValue (key, value) - QString, // UnsetValue (key) - std::tuple, // PrependOrSet (key, value, separator) - std::tuple, // AppendOrSet (key, value, separator) - NameValueItems, // Modify - std::monostate, // SetupEnglishOutput - FilePath // SetupSudoAskPass (file path of qtc-askpass or ssh-askpass) - >; + std::monostate, // SetSystemEnvironment dummy + NameValueDictionary, // SetFixedDictionary + std::tuple, // SetValue (key, value, enabled) + std::tuple, // SetFallbackValue (key, value) + QString, // UnsetValue (key) + std::tuple, // PrependOrSet (key, value, separator) + std::tuple, // AppendOrSet (key, value, separator) + NameValueItems, // Modify + std::monostate, // SetupEnglishOutput + FilePath // SetupSudoAskPass (file path of qtc-askpass or ssh-askpass) + >; void addItem(const Item &item); diff --git a/src/plugins/cmakeprojectmanager/presetsmacros.cpp b/src/plugins/cmakeprojectmanager/presetsmacros.cpp index 7681d2f7e13..b7496d0c412 100644 --- a/src/plugins/cmakeprojectmanager/presetsmacros.cpp +++ b/src/plugins/cmakeprojectmanager/presetsmacros.cpp @@ -139,10 +139,8 @@ void expand(const PresetType &preset, Environment &env, const FilePath &sourceDi return presetEnv.value(macroName); }); - QString sep; bool append = true; if (key.compare("PATH", Qt::CaseInsensitive) == 0) { - sep = OsSpecificAspects::pathListSeparator(env.osType()); const int index = value.indexOf("$penv{PATH}", 0, Qt::CaseInsensitive); if (index != 0) append = false; @@ -157,9 +155,9 @@ void expand(const PresetType &preset, Environment &env, const FilePath &sourceDi expandAllButEnv(preset, sourceDirectory, value); if (append) - env.appendOrSet(key, value, sep); + env.appendOrSet(key, value); else - env.prependOrSet(key, value, sep); + env.prependOrSet(key, value); }); } diff --git a/src/plugins/debugger/cdb/cdbengine.cpp b/src/plugins/debugger/cdb/cdbengine.cpp index 90035878533..68a9c9f2301 100644 --- a/src/plugins/debugger/cdb/cdbengine.cpp +++ b/src/plugins/debugger/cdb/cdbengine.cpp @@ -410,11 +410,10 @@ void CdbEngine::setupEngine() inferiorEnvironment.set(qtLoggingToConsoleKey, "0"); static const char cdbExtensionPathVariableC[] = "_NT_DEBUGGER_EXTENSION_PATH"; - const QString pSep = OsSpecificAspects::pathListSeparator(Utils::OsTypeWindows); - inferiorEnvironment.prependOrSet(cdbExtensionPathVariableC, extensionFi.absolutePath(), pSep); + inferiorEnvironment.prependOrSet(cdbExtensionPathVariableC, extensionFi.absolutePath()); const QString oldCdbExtensionPath = qtcEnvironmentVariable(cdbExtensionPathVariableC); if (!oldCdbExtensionPath.isEmpty()) - inferiorEnvironment.appendOrSet(cdbExtensionPathVariableC, oldCdbExtensionPath, pSep); + inferiorEnvironment.appendOrSet(cdbExtensionPathVariableC, oldCdbExtensionPath); m_process.setEnvironment(inferiorEnvironment); if (!sp.inferior.workingDirectory.isEmpty()) diff --git a/src/plugins/debugger/dap/pydapengine.cpp b/src/plugins/debugger/dap/pydapengine.cpp index 90b6b915886..68935581450 100644 --- a/src/plugins/debugger/dap/pydapengine.cpp +++ b/src/plugins/debugger/dap/pydapengine.cpp @@ -83,9 +83,7 @@ public: Environment env = m_runParameters.debugger.environment; const FilePath debugPyDir = packageDir(m_cmd.executable(), "debugpy"); if (QTC_GUARD(debugPyDir.isSameDevice(m_cmd.executable()))) { - env.appendOrSet("PYTHONPATH", - debugPyDir.needsDevice() ? debugPyDir.path() : debugPyDir.toUserOutput(), - OsSpecificAspects::pathListSeparator(debugPyDir.osType())); + env.appendOrSet("PYTHONPATH", debugPyDir.path()); } m_proc.setEnvironment(env); diff --git a/src/plugins/python/pythonlanguageclient.cpp b/src/plugins/python/pythonlanguageclient.cpp index c4eb0290748..dcba4d1ac61 100644 --- a/src/plugins/python/pythonlanguageclient.cpp +++ b/src/plugins/python/pythonlanguageclient.cpp @@ -109,15 +109,11 @@ protected: Environment env = python.deviceEnvironment(); const FilePath lspPath = pyLspPath(python); if (!lspPath.isEmpty() && lspPath.exists() && QTC_GUARD(lspPath.isSameDevice(python))) { - env.appendOrSet("PYTHONPATH", - lspPath.path(), - OsSpecificAspects::pathListSeparator(env.osType())); + env.appendOrSet("PYTHONPATH", lspPath.path()); } if (!python.needsDevice()) { // todo check where to put this tempdir in remote setups - env.appendOrSet("PYTHONPATH", - m_extraPythonPath.path().toString(), - OsSpecificAspects::pathListSeparator(env.osType())); + env.appendOrSet("PYTHONPATH", m_extraPythonPath.path().toString()); } if (env.hasChanges()) setEnvironment(env); diff --git a/src/plugins/qmldesigner/puppetenvironmentbuilder.cpp b/src/plugins/qmldesigner/puppetenvironmentbuilder.cpp index 7e876816c97..d19b1d202c6 100644 --- a/src/plugins/qmldesigner/puppetenvironmentbuilder.cpp +++ b/src/plugins/qmldesigner/puppetenvironmentbuilder.cpp @@ -223,7 +223,7 @@ void PuppetEnvironmentBuilder::addImportPaths() const importPaths.prepend(QLibraryInfo::path(QLibraryInfo::Qml2ImportsPath)); constexpr auto pathSep = Utils::HostOsInfo::pathListSeparator(); - m_environment.appendOrSet("QML2_IMPORT_PATH", importPaths.join(pathSep), pathSep); + m_environment.appendOrSet("QML2_IMPORT_PATH", importPaths.join(pathSep)); qCInfo(puppetEnvirmentBuild) << "Puppet import paths:" << importPaths; } @@ -237,9 +237,9 @@ void PuppetEnvironmentBuilder::addCustomFileSelectors() const customFileSelectors.append("DesignMode"); - constexpr auto pathSep = Utils::HostOsInfo::pathListSeparator(); if (!customFileSelectors.isEmpty()) - m_environment.appendOrSet("QML_FILE_SELECTORS", customFileSelectors.join(","), pathSep); + m_environment.appendOrSet("QML_FILE_SELECTORS", + customFileSelectors.join(",")); qCInfo(puppetEnvirmentBuild) << "Puppet selectors:" << customFileSelectors; } diff --git a/src/plugins/qmljstools/qmljsmodelmanager.cpp b/src/plugins/qmljstools/qmljsmodelmanager.cpp index 2ed984f4cbb..c8265d73544 100644 --- a/src/plugins/qmljstools/qmljsmodelmanager.cpp +++ b/src/plugins/qmljstools/qmljsmodelmanager.cpp @@ -155,7 +155,9 @@ ModelManagerInterface::ProjectInfo ModelManager::defaultProjectInfoForProject( // Append QML2_IMPORT_PATH if it is defined in build configuration. // It enables qmlplugindump to correctly dump custom plugins or other dependent // plugins that are not installed in default Qt qml installation directory. - projectInfo.qmlDumpEnvironment.appendOrSet("QML2_IMPORT_PATH", bc->environment().expandedValueForKey("QML2_IMPORT_PATH"), ":"); + projectInfo.qmlDumpEnvironment.appendOrSet("QML2_IMPORT_PATH", + bc->environment().expandedValueForKey( + "QML2_IMPORT_PATH")); // Treat every target (library or application) in the build directory FilePath dir = bc->buildDirectory(); diff --git a/src/plugins/qnx/qnxrunconfiguration.cpp b/src/plugins/qnx/qnxrunconfiguration.cpp index 8b4e5498707..9f00bb8b5a0 100644 --- a/src/plugins/qnx/qnxrunconfiguration.cpp +++ b/src/plugins/qnx/qnxrunconfiguration.cpp @@ -63,10 +63,10 @@ public: setRunnableModifier([this](ProcessRunData &r) { QString libPath = qtLibraries(); if (!libPath.isEmpty()) { - r.environment.appendOrSet("LD_LIBRARY_PATH", libPath + "/lib:$LD_LIBRARY_PATH"); - r.environment.appendOrSet("QML_IMPORT_PATH", libPath + "/imports:$QML_IMPORT_PATH"); - r.environment.appendOrSet("QML2_IMPORT_PATH", libPath + "/qml:$QML2_IMPORT_PATH"); - r.environment.appendOrSet("QT_PLUGIN_PATH", libPath + "/plugins:$QT_PLUGIN_PATH"); + r.environment.prependOrSet("LD_LIBRARY_PATH", libPath + "/lib"); + r.environment.prependOrSet("QML_IMPORT_PATH", libPath + "/imports"); + r.environment.prependOrSet("QML2_IMPORT_PATH", libPath + "/qml"); + r.environment.prependOrSet("QT_PLUGIN_PATH", libPath + "/plugins"); r.environment.set("QT_QPA_FONTDIR", libPath + "/lib/fonts"); } }); diff --git a/src/plugins/terminal/shellintegration.cpp b/src/plugins/terminal/shellintegration.cpp index e989f02dd3b..60b08de5917 100644 --- a/src/plugins/terminal/shellintegration.cpp +++ b/src/plugins/terminal/shellintegration.cpp @@ -184,17 +184,13 @@ void ShellIntegration::prepareProcess(Utils::Process &process) rcPath.copyFile(tmpRc); env.set("CLINK_HISTORY_LABEL", "QtCreator"); - env.appendOrSet("CLINK_PATH", - tmpRc.parentDir().nativePath(), - OsSpecificAspects::pathListSeparator(env.osType())); + env.appendOrSet("CLINK_PATH", tmpRc.parentDir().nativePath()); } else if (cmd.executable().baseName() == "fish") { FilePath xdgDir = FilePath::fromUserInput(m_tempDir.filePath("fish_xdg_data")); FilePath subDir = xdgDir.resolvePath(QString("fish/vendor_conf.d")); QTC_ASSERT(subDir.createDir(), return); filesToCopy.fish.script.copyFile(subDir.resolvePath(filesToCopy.fish.script.fileName())); - env.appendOrSet("XDG_DATA_DIRS", - xdgDir.toUserOutput(), - OsSpecificAspects::pathListSeparator(env.osType())); + env.appendOrSet("XDG_DATA_DIRS", xdgDir.toUserOutput()); } process.setCommand(cmd); diff --git a/tests/auto/environment/tst_environment.cpp b/tests/auto/environment/tst_environment.cpp index 1b870ddd6da..64dfe426487 100644 --- a/tests/auto/environment/tst_environment.cpp +++ b/tests/auto/environment/tst_environment.cpp @@ -363,12 +363,10 @@ void tst_Environment::pathChanges() QFETCH(QString, value); QFETCH(Environment, expected); - const QString sep = OsSpecificAspects::pathListSeparator(environment.osType()); - if (prepend) - environment.prependOrSet(variable, value, sep); + environment.prependOrSet(variable, value); else - environment.appendOrSet(variable, value, sep); + environment.appendOrSet(variable, value); qDebug() << "Actual :" << environment.toStringList(); qDebug() << "Expected:" << expected.toStringList();