CMakePM: Reuse EnvironmentAspect for CMake configure environment

EnvironmentAspect is used in the Run configuration page.

For a configuration not using presets the "Build Environment" will be
used by default. This is the pre Qt Creator 9 behavior.

With presets "System Environment" is used, which allows the environment
sepparation of configure and build steps.

Fixes: QTCREATORBUG-28513
Change-Id: Ie65c0a5ac67355642460fca9fc618a1d29f4b1bd
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

src/plugins/cmakeprojectmanager/cmakebuildconfiguration.cpp

@@ -32,6 +32,7 @@
 #include <projectexplorer/buildmanager.h>
 #include <projectexplorer/buildsteplist.h>
 #include <projectexplorer/devicesupport/idevice.h>
+#include <projectexplorer/environmentaspectwidget.h>
 #include <projectexplorer/environmentwidget.h>
 #include <projectexplorer/kitinformation.h>
 #include <projectexplorer/namedwidget.h>
@@ -92,6 +93,7 @@ const char CMAKE_QT6_TOOLCHAIN_FILE_ARG[]
 const char CMAKE_BUILD_TYPE[] = "CMake.Build.Type";
 const char CLEAR_SYSTEM_ENVIRONMENT_KEY[] = "CMake.Configure.ClearSystemEnvironment";
 const char USER_ENVIRONMENT_CHANGES_KEY[] = "CMake.Configure.UserEnvironmentChanges";
+const char BASE_ENVIRONMENT_KEY[] = "CMake.Configure.BaseEnvironment";
 
 namespace Internal {
 
@@ -172,7 +174,6 @@ CMakeBuildSettingsWidget::CMakeBuildSettingsWidget(CMakeBuildSystem *bs) :
 {
     QTC_ASSERT(bs, return);
     BuildConfiguration *bc = bs->buildConfiguration();
-    CMakeBuildConfiguration *cbc = static_cast<CMakeBuildConfiguration *>(bc);
 
     m_configureDetailsWidget = new DetailsWidget;
 
@@ -309,35 +310,6 @@ CMakeBuildSettingsWidget::CMakeBuildSettingsWidget(CMakeBuildSystem *bs) :
     m_reconfigureButton = new QPushButton(Tr::tr("Run CMake"));
     m_reconfigureButton->setEnabled(false);
 
-    auto clearBox = new QCheckBox(Tr::tr("Clear system environment"), this);
-    clearBox->setChecked(cbc->useClearConfigureEnvironment());
-
-    auto envWidget = new EnvironmentWidget(this, EnvironmentWidget::TypeLocal, clearBox);
-    envWidget->setBaseEnvironment(cbc->baseConfigureEnvironment());
-    envWidget->setBaseEnvironmentText(cbc->baseConfigureEnvironmentText());
-    envWidget->setUserChanges(cbc->userConfigureEnvironmentChanges());
-
-    const EnvironmentWidget::OpenTerminalFunc openTerminalFunc
-            = [bc](const Utils::Environment &env) {
-        Core::FileUtils::openTerminal(bc->buildDirectory(), env);
-    };
-    envWidget->setOpenTerminalFunc(openTerminalFunc);
-
-    connect(envWidget, &EnvironmentWidget::userChangesChanged, this, [cbc, envWidget] {
-        cbc->setUserConfigureEnvironmentChanges(envWidget->userChanges());
-    });
-
-    connect(clearBox, &QAbstractButton::toggled, this, [cbc, envWidget](bool checked) {
-        cbc->setUseClearConfigureEnvironment(checked);
-        envWidget->setBaseEnvironment(cbc->baseConfigureEnvironment());
-        envWidget->setBaseEnvironmentText(cbc->baseConfigureEnvironmentText());
-    });
-
-    connect(cbc, &CMakeBuildConfiguration::configureEnvironmentChanged, this, [cbc, envWidget] {
-        envWidget->setBaseEnvironment(cbc->baseConfigureEnvironment());
-        envWidget->setBaseEnvironmentText(cbc->baseConfigureEnvironmentText());
-    });
-
     using namespace Layouting;
     Grid cmakeConfiguration {
         m_filterEdit, br,
@@ -355,6 +327,11 @@ CMakeBuildSettingsWidget::CMakeBuildSettingsWidget(CMakeBuildSystem *bs) :
         }
     };
 
+    auto configureEnvironmentAspectWidget
+        = bc->aspect<ConfigureEnvironmentAspect>()->createConfigWidget();
+    configureEnvironmentAspectWidget->setContentsMargins(0, 0, 0, 0);
+    configureEnvironmentAspectWidget->layout()->setContentsMargins(0, 0, 0, 0);
+
     Column {
         Form {
             buildDirAspect,
@@ -375,8 +352,7 @@ CMakeBuildSettingsWidget::CMakeBuildSettingsWidget(CMakeBuildSystem *bs) :
                     m_reconfigureButton,
                 }
             },
-            clearBox,
-            envWidget
+            configureEnvironmentAspectWidget
         }
     }.attachTo(details, WithoutMargins);
 
@@ -1364,18 +1340,6 @@ static Utils::EnvironmentItems getEnvironmentItemsFromCMakeBuildPreset(
     return envItems;
 }
 
-// -----------------------------------------------------------------------------
-// CMakeBuildConfigurationPrivate:
-// -----------------------------------------------------------------------------
-
-class CMakeBuildConfigurationPrivate
-{
-public:
-    Utils::Environment m_configureEnvironment;
-    Utils::EnvironmentItems  m_userConfigureEnvironmentChanges;
-    bool m_clearSystemConfigureEnvironment = false;
-};
-
 } // namespace Internal
 
 // -----------------------------------------------------------------------------
@@ -1383,7 +1347,7 @@ public:
 // -----------------------------------------------------------------------------
 
 CMakeBuildConfiguration::CMakeBuildConfiguration(Target *target, Id id)
-    : BuildConfiguration(target, id), d(new Internal::CMakeBuildConfigurationPrivate())
+    : BuildConfiguration(target, id)
 {
     m_buildSystem = new CMakeBuildSystem(this);
 
@@ -1462,6 +1426,8 @@ CMakeBuildConfiguration::CMakeBuildConfiguration(Target *target, Id id)
     addAspect<BuildTypeAspect>();
     addAspect<QtSupport::QmlDebuggingAspect>(this);
 
+    addAspect<ConfigureEnvironmentAspect>(target);
+
     setInitialBuildAndCleanSteps(target);
 
     setInitializer([this, target](const BuildInfo &info) {
@@ -1566,8 +1532,8 @@ CMakeBuildConfiguration::CMakeBuildConfiguration(Target *target, Id id)
             cmd.addArg("-DCMAKE_CXX_FLAGS_INIT:STRING=%{" + QLatin1String(QT_QML_DEBUG_FLAG) + "}");
 
         CMakeProject *cmakeProject = static_cast<CMakeProject *>(target->project());
-        setUserConfigureEnvironmentChanges(getEnvironmentItemsFromCMakeConfigurePreset(cmakeProject, k));
-        updateAndEmitConfigureEnvironmentChanged();
+        aspect<ConfigureEnvironmentAspect>()->setUserEnvironmentChanges(
+            getEnvironmentItemsFromCMakeConfigurePreset(cmakeProject, k));
 
         QStringList initialCMakeArguments = cmd.splitArguments();
         addCMakeConfigurePresetToInitialArguments(initialCMakeArguments,
@@ -1585,14 +1551,11 @@ CMakeBuildConfiguration::CMakeBuildConfiguration(Target *target, Id id)
 CMakeBuildConfiguration::~CMakeBuildConfiguration()
 {
     delete m_buildSystem;
-    delete d;
 }
 
 QVariantMap CMakeBuildConfiguration::toMap() const
 {
     QVariantMap map(BuildConfiguration::toMap());
-    map.insert(QLatin1String(CLEAR_SYSTEM_ENVIRONMENT_KEY), d->m_clearSystemConfigureEnvironment);
-    map.insert(QLatin1String(USER_ENVIRONMENT_CHANGES_KEY), EnvironmentItem::toStringList(d->m_userConfigureEnvironmentChanges));
     return map;
 }
 
@@ -1627,27 +1590,6 @@ bool CMakeBuildConfiguration::fromMap(const QVariantMap &map)
         m_buildSystem->setInitialCMakeArguments(cmd.splitArguments());
     }
 
-    // Upgrading from Qt Creator version <9 to 9, if the build environment is set
-    // apply the specific values to the configure environment
-    auto settingsKey = [map](const QLatin1String &configureKey, const QLatin1String &buildKey) {
-        if (!map.contains(configureKey) && map.contains(buildKey))
-            return buildKey;
-        return configureKey;
-    };
-
-    const QLatin1String clearSystemEnvironmentKey
-        = settingsKey(QLatin1String(CLEAR_SYSTEM_ENVIRONMENT_KEY),
-                      QLatin1String(ProjectExplorer::Constants::CLEAR_SYSTEM_ENVIRONMENT_KEY));
-    const QLatin1String userEnvironmentChangesKey
-        = settingsKey(QLatin1String(USER_ENVIRONMENT_CHANGES_KEY),
-                      QLatin1String(ProjectExplorer::Constants::USER_ENVIRONMENT_CHANGES_KEY));
-
-    d->m_clearSystemConfigureEnvironment = map.value(clearSystemEnvironmentKey).toBool();
-    d->m_userConfigureEnvironmentChanges = EnvironmentItem::fromStringList(
-        map.value(userEnvironmentChangesKey).toStringList());
-
-    updateAndEmitConfigureEnvironmentChanged();
-
     return true;
 }
 
@@ -2128,64 +2070,7 @@ void CMakeBuildConfiguration::addToEnvironment(Utils::Environment &env) const
 
 Environment CMakeBuildConfiguration::configureEnvironment() const
 {
-    return d->m_configureEnvironment;
-}
-
-void CMakeBuildConfiguration::setUserConfigureEnvironmentChanges(const Utils::EnvironmentItems &diff)
-{
-    if (d->m_userConfigureEnvironmentChanges == diff)
-        return;
-    d->m_userConfigureEnvironmentChanges = diff;
-    updateAndEmitConfigureEnvironmentChanged();
-}
-
-EnvironmentItems CMakeBuildConfiguration::userConfigureEnvironmentChanges() const
-{
-    return d->m_userConfigureEnvironmentChanges;
-}
-
-bool CMakeBuildConfiguration::useClearConfigureEnvironment() const
-{
-    return d->m_clearSystemConfigureEnvironment;
-}
-
-void CMakeBuildConfiguration::setUseClearConfigureEnvironment(bool b)
-{
-    if (useClearConfigureEnvironment() == b)
-        return;
-    d->m_clearSystemConfigureEnvironment = b;
-    updateAndEmitConfigureEnvironmentChanged();
-}
-
-void CMakeBuildConfiguration::updateAndEmitConfigureEnvironmentChanged()
-{
-    Environment env = baseConfigureEnvironment();
-    env.modify(userConfigureEnvironmentChanges());
-    if (env == d->m_configureEnvironment)
-        return;
-    d->m_configureEnvironment = env;
-    emit configureEnvironmentChanged();
-}
-
-Environment CMakeBuildConfiguration::baseConfigureEnvironment() const
-{
-    Environment result;
-    if (!useClearConfigureEnvironment()) {
-        ProjectExplorer::IDevice::ConstPtr devicePtr = BuildDeviceKitAspect::device(kit());
-        result = devicePtr ? devicePtr->systemEnvironment() : Environment::systemEnvironment();
-    }
-    kit()->addToBuildEnvironment(result);
-    addToEnvironment(result);
-    result.modify(project()->additionalEnvironment());
-    return result;
-}
-
-QString CMakeBuildConfiguration::baseConfigureEnvironmentText() const
-{
-    if (useClearConfigureEnvironment())
-        return Tr::tr("Clean Environment");
-    else
-        return Tr::tr("System Environment");
+    return aspect<ConfigureEnvironmentAspect>()->environment();
 }
 
 QString CMakeBuildSystem::cmakeBuildType() const
@@ -2355,5 +2240,111 @@ BuildTypeAspect::BuildTypeAspect()
     setDefaultValue("Unknown");
 }
 
+// -----------------------------------------------------------------------------
+// ConfigureEnvironmentAspect:
+// -----------------------------------------------------------------------------
+class ConfigureEnvironmentAspectWidget final : public ProjectExplorer::EnvironmentAspectWidget
+{
+public:
+    ConfigureEnvironmentAspectWidget(ConfigureEnvironmentAspect *aspect,
+                                     ProjectExplorer::Target *target)
+        : EnvironmentAspectWidget(aspect)
+    {
+        envWidget()->setOpenTerminalFunc([target](const Environment &env) {
+            if (BuildConfiguration *bc = target->activeBuildConfiguration())
+                Core::FileUtils::openTerminal(bc->buildDirectory(), env);
+        });
+    }
+};
+
+ConfigureEnvironmentAspect::ConfigureEnvironmentAspect(ProjectExplorer::Target *target)
+{
+    setIsLocal(true);
+    setConfigWidgetCreator(
+        [this, target] { return new ConfigureEnvironmentAspectWidget(this, target); });
+    addSupportedBaseEnvironment(Tr::tr("Clean Environment"), {});
+    setLabelText(Tr::tr("Base environment for the CMake configure step:"));
+
+    const int systemEnvIndex = addSupportedBaseEnvironment(Tr::tr("System Environment"), [target] {
+        IDevice::ConstPtr device = BuildDeviceKitAspect::device(target->kit());
+        return device ? device->systemEnvironment() : Environment::systemEnvironment();
+    });
+
+    const int buildEnvIndex = addSupportedBaseEnvironment(Tr::tr("Build Environment"), [target] {
+        Environment env;
+        if (BuildConfiguration *bc = target->activeBuildConfiguration()) {
+            env = bc->environment();
+        } else { // Fallback for targets without buildconfigurations:
+            env = target->kit()->buildEnvironment();
+        }
+        return env;
+    });
+
+    connect(target,
+            &Target::activeBuildConfigurationChanged,
+            this,
+            &EnvironmentAspect::environmentChanged);
+    connect(target,
+            &Target::buildEnvironmentChanged,
+            this,
+            &EnvironmentAspect::environmentChanged);
+
+
+    const CMakeConfigItem presetItem = CMakeConfigurationKitAspect::cmakePresetConfigItem(
+        target->kit());
+
+    setBaseEnvironmentBase(presetItem.isNull() ? buildEnvIndex : systemEnvIndex);
+
+    connect(target->project(),
+            &Project::environmentChanged,
+            this,
+            &EnvironmentAspect::environmentChanged);
+
+    connect(KitManager::instance(), &KitManager::kitUpdated, this, [this, target](const Kit *k) {
+        if (target->kit() == k)
+            emit EnvironmentAspect::environmentChanged();
+    });
+
+    addModifier([target](Utils::Environment &env) {
+        // This will add ninja to path
+        if (BuildConfiguration *bc = target->activeBuildConfiguration()) {
+            bc->addToEnvironment(env);
+        }
+        target->kit()->addToBuildEnvironment(env);
+        env.modify(target->project()->additionalEnvironment());
+    });
+}
+
+void ConfigureEnvironmentAspect::fromMap(const QVariantMap &map)
+{
+    // Match the key values from Qt Creator 9.0.0/1 to the ones from EnvironmentAspect
+    const bool cleanSystemEnvironment = map.value(QLatin1String(CLEAR_SYSTEM_ENVIRONMENT_KEY))
+                                            .toBool();
+    const QStringList userEnvironmentChanges
+        = map.value(QLatin1String(USER_ENVIRONMENT_CHANGES_KEY)).toStringList();
+
+    const int baseEnvironmentIndex
+        = map.value(QLatin1String(BASE_ENVIRONMENT_KEY), baseEnvironmentBase()).toInt();
+
+    QVariantMap tmpMap;
+    tmpMap.insert(QLatin1String(BASE_KEY), cleanSystemEnvironment ? 0 : baseEnvironmentIndex);
+    tmpMap.insert(QLatin1String(CHANGES_KEY), userEnvironmentChanges);
+
+    ProjectExplorer::EnvironmentAspect::fromMap(tmpMap);
+}
+
+void ConfigureEnvironmentAspect::toMap(QVariantMap &map) const
+{
+    QVariantMap tmpMap;
+    ProjectExplorer::EnvironmentAspect::toMap(tmpMap);
+
+    const int baseKey = tmpMap.value(BASE_KEY).toInt();
+
+    map.insert(QLatin1String(CLEAR_SYSTEM_ENVIRONMENT_KEY), baseKey == 0);
+    map.insert(QLatin1String(BASE_ENVIRONMENT_KEY), baseKey);
+    map.insert(QLatin1String(USER_ENVIRONMENT_CHANGES_KEY),
+               tmpMap.value(CHANGES_KEY).toStringList());
+}
+
 } // namespace Internal
 } // namespace CMakeProjectManager

src/plugins/cmakeprojectmanager/cmakebuildconfiguration.h

@@ -8,6 +8,7 @@
 
 #include <projectexplorer/buildaspects.h>
 #include <projectexplorer/buildconfiguration.h>
+#include <projectexplorer/environmentaspect.h>
 
 namespace CMakeProjectManager {
 class CMakeProject;
@@ -15,7 +16,6 @@ class CMakeProject;
 namespace Internal {
 
 class CMakeBuildSystem;
-class CMakeBuildConfigurationPrivate;
 class CMakeBuildSettingsWidget;
 class CMakeProjectImporter;
 
@@ -45,14 +45,6 @@ public:
     void addToEnvironment(Utils::Environment &env) const override;
 
     Utils::Environment configureEnvironment() const;
-    void setUserConfigureEnvironmentChanges(const Utils::EnvironmentItems &diff);
-    Utils::EnvironmentItems userConfigureEnvironmentChanges() const;
-    bool useClearConfigureEnvironment() const;
-    void setUseClearConfigureEnvironment(bool b);
-    void updateAndEmitConfigureEnvironmentChanged();
-
-    Utils::Environment baseConfigureEnvironment() const;
-    QString baseConfigureEnvironmentText() const;
 
 signals:
     void signingFlagsChanged();
@@ -76,8 +68,6 @@ private:
 
     friend class Internal::CMakeBuildSettingsWidget;
     friend class Internal::CMakeBuildSystem;
-
-    Internal::CMakeBuildConfigurationPrivate *d = nullptr;
 };
 
 class CMAKE_EXPORT CMakeBuildConfigurationFactory
@@ -148,5 +138,16 @@ public:
     using Utils::StringAspect::update;
 };
 
+class ConfigureEnvironmentAspect final: public ProjectExplorer::EnvironmentAspect
+{
+    Q_OBJECT
+
+public:
+    ConfigureEnvironmentAspect(ProjectExplorer::Target *target);
+
+    void fromMap(const QVariantMap &map);
+    void toMap(QVariantMap &map) const;
+};
+
 } // namespace Internal
 } // namespace CMakeProjectManager

src/plugins/projectexplorer/environmentaspect.cpp

@@ -11,9 +11,6 @@
 
 using namespace Utils;
 
-static const char BASE_KEY[] = "PE.EnvironmentAspect.Base";
-static const char CHANGES_KEY[] = "PE.EnvironmentAspect.Changes";
-
 namespace ProjectExplorer {
 
 // --------------------------------------------------------------------
@@ -77,25 +74,31 @@ void EnvironmentAspect::addModifier(const EnvironmentAspect::EnvironmentModifier
     m_modifiers.append(modifier);
 }
 
-void EnvironmentAspect::addSupportedBaseEnvironment(const QString &displayName,
+int EnvironmentAspect::addSupportedBaseEnvironment(const QString &displayName,
                                                    const std::function<Environment()> &getter)
 {
     BaseEnvironment baseEnv;
     baseEnv.displayName = displayName;
     baseEnv.getter = getter;
     m_baseEnvironments.append(baseEnv);
+    const int index = m_baseEnvironments.size() - 1;
     if (m_base == -1)
-        setBaseEnvironmentBase(m_baseEnvironments.size() - 1);
+        setBaseEnvironmentBase(index);
+
+    return index;
 }
 
-void EnvironmentAspect::addPreferredBaseEnvironment(const QString &displayName,
+int EnvironmentAspect::addPreferredBaseEnvironment(const QString &displayName,
                                                    const std::function<Environment()> &getter)
 {
     BaseEnvironment baseEnv;
     baseEnv.displayName = displayName;
     baseEnv.getter = getter;
     m_baseEnvironments.append(baseEnv);
-    setBaseEnvironmentBase(m_baseEnvironments.size() - 1);
+    const int index = m_baseEnvironments.size() - 1;
+    setBaseEnvironmentBase(index);
+
+    return index;
 }
 
 void EnvironmentAspect::fromMap(const QVariantMap &map)

src/plugins/projectexplorer/environmentaspect.h

@@ -34,9 +34,9 @@ public:
     Utils::EnvironmentItems userEnvironmentChanges() const { return m_userChanges; }
     void setUserEnvironmentChanges(const Utils::EnvironmentItems &diff);
 
-    void addSupportedBaseEnvironment(const QString &displayName,
+    int addSupportedBaseEnvironment(const QString &displayName,
                                     const std::function<Utils::Environment()> &getter);
-    void addPreferredBaseEnvironment(const QString &displayName,
+    int addPreferredBaseEnvironment(const QString &displayName,
                                     const std::function<Utils::Environment()> &getter);
 
     QString currentDisplayName() const;
@@ -53,6 +53,9 @@ public:
         Utils::Environment environment;
     };
 
+    using Utils::BaseAspect::setupLabel;
+    using Utils::BaseAspect::label;
+
 signals:
     void baseEnvironmentChanged();
     void userEnvironmentChangesChanged(const Utils::EnvironmentItems &diff);
@@ -64,6 +67,9 @@ protected:
 
     void setIsLocal(bool local) { m_isLocal = local; }
 
+    static constexpr char BASE_KEY[] = "PE.EnvironmentAspect.Base";
+    static constexpr char CHANGES_KEY[] = "PE.EnvironmentAspect.Changes";
+
 private:
     // One possible choice in the Environment aspect.
     struct BaseEnvironment {

src/plugins/projectexplorer/environmentaspectwidget.cpp

@@ -32,8 +32,14 @@ EnvironmentAspectWidget::EnvironmentAspectWidget(EnvironmentAspect *aspect)
     auto baseEnvironmentWidget = new QWidget;
     m_baseLayout = new QHBoxLayout(baseEnvironmentWidget);
     m_baseLayout->setContentsMargins(0, 0, 0, 0);
-    auto label = new QLabel(Tr::tr("Base environment for this run configuration:"), this);
-    m_baseLayout->addWidget(label);
+
+    auto label = [aspect]() {
+        if (aspect->labelText().isEmpty())
+            aspect->setLabelText(Tr::tr("Base environment for this run configuration:"));
+        aspect->setupLabel();
+        return aspect->label();
+    };
+    m_baseLayout->addWidget(label());
 
     m_baseEnvironmentComboBox = new QComboBox;
     for (const QString &displayName : m_aspect->displayNames())