cmake-gui: settle preset application before Configure/Generate

Selecting a configure preset applies on the worker thread, but Configure
and Generate stayed enabled and could snapshot the stale cache model
first. Tokenize preset application, disable the dependent actions until
the completion lands, and accept only the latest request.

Fixes: #28085

Source/QtDialog/CMakeSetupDialog.cxx

@@ -294,6 +294,8 @@ void CMakeSetupDialog::initialize()
                    this, &CMakeSetupDialog::updatePresets);
   QObject::connect(this->CMakeThread->cmakeInstance(), &QCMake::presetChanged,
                    this, &CMakeSetupDialog::updatePreset);
+  QObject::connect(this->CMakeThread->cmakeInstance(), &QCMake::presetApplied,
+                   this, &CMakeSetupDialog::onPresetApplied);
   QObject::connect(this->CMakeThread->cmakeInstance(),
                    &QCMake::presetLoadError, this,
                    &CMakeSetupDialog::showPresetLoadError);
@@ -442,6 +444,11 @@ void CMakeSetupDialog::doConfigure()
     return;
   }
 
+  // Preset still applying: its cache values aren't in the model yet.
+  if (this->PresetApplicationPending) {
+    return;
+  }
+
   if (!prepareConfigure()) {
     return;
   }
@@ -550,6 +557,12 @@ void CMakeSetupDialog::doGenerate()
     return;
   }
 
+  // Preset still applying: prepareConfigure() would read worker state that
+  // applyPreset() is still mutating.
+  if (this->PresetApplicationPending) {
+    return;
+  }
+
   // see if we need to configure
   // we'll need to configure if:
   //   the configure step hasn't been done yet
@@ -730,11 +743,37 @@ void CMakeSetupDialog::updatePresets(QVector<QCMakePreset> const& presets)
   this->Preset->setToolTip(presets.isEmpty() ? PRESETS_DISABLED_TOOLTIP : "");
 
   if (!this->DeferredPreset.isNull()) {
-    this->Preset->setPresetName(this->DeferredPreset);
+    QString const deferred = this->DeferredPreset;
     this->DeferredPreset = QString{};
+    // An available preset selection submits a real request and keeps the gate
+    // engaged; an unavailable one applies nothing, so release the gate here.
+    this->Preset->setPresetName(deferred);
+    if (this->Preset->presetName() != deferred) {
+      this->PresetApplicationPending = false;
+      this->updateCommandState();
+    }
   }
 }
 
+void CMakeSetupDialog::onPresetApplied(quint64 requestId, QString const& name,
+                                       QCMakePropertyList const& properties)
+{
+  // Latest request wins; ignore a superseded completion.
+  if (requestId != this->LatestPresetRequestId) {
+    return;
+  }
+
+  this->CacheValues->cacheModel()->setProperties(properties);
+  // Reconcile in case the combo drifted from what actually applied.
+  if (this->Preset->presetName() != name) {
+    this->Preset->blockSignals(true);
+    this->Preset->setPresetName(name);
+    this->Preset->blockSignals(false);
+  }
+  this->PresetApplicationPending = false;
+  this->updateCommandState();
+}
+
 void CMakeSetupDialog::updatePreset(QString const& name)
 {
   if (this->Preset->presetName() != name) {
@@ -806,9 +845,18 @@ void CMakeSetupDialog::onBinaryDirectoryChanged(QString const& dir)
 
 void CMakeSetupDialog::onBuildPresetChanged(QString const& name)
 {
-  QMetaObject::invokeMethod(this->CMakeThread->cmakeInstance(), "setPreset",
+  // Applying is async: gate the dependent actions now, until the completion
+  // lands.
+  quint64 const requestId = ++this->LatestPresetRequestId;
+  this->PresetApplicationPending = true;
+  // First Generate after an apply must reconfigure with the new inputs.
+  this->ConfigureNeeded = true;
+  this->updateCommandState();
+
+  QMetaObject::invokeMethod(this->CMakeThread->cmakeInstance(), "applyPreset",
                             Qt::QueuedConnection, Q_ARG(QString, name),
-                            Q_ARG(bool, !this->StartupBinaryDirectory));
+                            Q_ARG(bool, !this->StartupBinaryDirectory),
+                            Q_ARG(quint64, requestId));
   this->StartupBinaryDirectory = false;
 }
 
@@ -820,6 +868,12 @@ void CMakeSetupDialog::setSourceDirectory(QString const& dir)
 void CMakeSetupDialog::setDeferredPreset(QString const& preset)
 {
   this->DeferredPreset = preset;
+  // A --preset isn't applied until presets load; gate now so nothing runs
+  // with default inputs during that startup window.
+  if (!preset.isNull()) {
+    this->PresetApplicationPending = true;
+    this->updateCommandState();
+  }
 }
 
 void CMakeSetupDialog::showProgress(QString const& /*msg*/, float percent)
@@ -1206,11 +1260,66 @@ void CMakeSetupDialog::enterState(CMakeSetupDialog::State s)
     this->GenerateButton->setText(tr("&Stop"));
   } else if (s == ReadyConfigure || s == ReadyGenerate) {
     this->setEnabledState(true);
-    this->GenerateButton->setEnabled(true);
-    this->GenerateAction->setEnabled(true);
-    this->ConfigureButton->setEnabled(true);
     this->ConfigureButton->setText(tr("&Configure"));
     this->GenerateButton->setText(tr("&Generate"));
+    this->updateCommandState();
+  }
+}
+
+void CMakeSetupDialog::updateCommandState()
+{
+  bool const pending = this->PresetApplicationPending;
+
+  switch (this->CurrentState) {
+    case Interrupting:
+      this->ConfigureButton->setEnabled(false);
+      this->GenerateButton->setEnabled(false);
+      this->ConfigureAction->setEnabled(false);
+      this->GenerateAction->setEnabled(false);
+      this->OpenProjectButton->setEnabled(false);
+      break;
+    case Configuring:
+      this->ConfigureButton->setEnabled(true); // acts as Stop
+      this->GenerateButton->setEnabled(false);
+      this->ConfigureAction->setEnabled(false);
+      this->GenerateAction->setEnabled(false);
+      this->OpenProjectButton->setEnabled(false);
+      break;
+    case Generating:
+      this->GenerateButton->setEnabled(true); // acts as Stop
+      this->ConfigureButton->setEnabled(false);
+      this->ConfigureAction->setEnabled(false);
+      this->GenerateAction->setEnabled(false);
+      this->OpenProjectButton->setEnabled(false);
+      break;
+    case ReadyConfigure:
+    case ReadyGenerate: {
+      bool const enabled = !pending;
+      this->ConfigureButton->setEnabled(enabled);
+      this->GenerateButton->setEnabled(enabled);
+      this->ConfigureAction->setEnabled(enabled);
+      this->GenerateAction->setEnabled(enabled);
+
+      // Gate everything that reads or mutates the still-stale model, but keep
+      // the combo live so the user can pick a different preset.
+      this->CacheValues->cacheModel()->setEditEnabled(enabled);
+      this->SourceDirectory->setEnabled(enabled);
+      this->BrowseSourceDirectoryButton->setEnabled(enabled);
+      this->BinaryDirectory->setEnabled(enabled);
+      this->BrowseBinaryDirectoryButton->setEnabled(enabled);
+      this->ReloadCacheAction->setEnabled(enabled);
+      this->DeleteCacheAction->setEnabled(enabled);
+      this->ReloadPresetsButton->setEnabled(enabled);
+      this->AddEntry->setEnabled(enabled);
+      this->Environment->setEnabled(enabled);
+      this->Preset->setEnabled(!this->Preset->presets().isEmpty());
+      if (enabled) {
+        this->selectionChanged(); // let selection re-enable Remove
+      } else {
+        this->RemoveEntry->setEnabled(false);
+      }
+      break;
+    }
   }
 }
 

Source/QtDialog/CMakeSetupDialog.h

@@ -59,6 +59,8 @@ protected slots:
   void updateBinaryDirectory(QString const& dir);
   void updatePresets(QVector<QCMakePreset> const& presets);
   void updatePreset(QString const& name);
+  void onPresetApplied(quint64 requestId, QString const& name,
+                       QCMakePropertyList const& properties);
   void showPresetLoadError(QString const& dir, QString const& message);
   void showProgress(QString const& msg, float percent);
   void setEnabledState(bool);
@@ -105,6 +107,9 @@ protected:
     Generating
   };
   void enterState(State s);
+  // Recompute command/control availability from state and pending.  Kept out
+  // of enterState() since pending can toggle without a lifecycle change.
+  void updateCommandState();
 
   void closeEvent(QCloseEvent*);
   void dragEnterEvent(QDragEnterEvent*);
@@ -123,6 +128,10 @@ protected:
   State CurrentState;
   QString DeferredPreset;
   bool StartupBinaryDirectory = false;
+  // Only the completion matching LatestPresetRequestId is accepted; a request
+  // is outstanding while PresetApplicationPending holds.
+  quint64 LatestPresetRequestId = 0;
+  bool PresetApplicationPending = false;
 
   QTextCharFormat ErrorFormat;
   QTextCharFormat MessageFormat;

Source/QtDialog/QCMake.cxx

@@ -35,6 +35,25 @@ static QString sanitizedDirPath(QString const& dir)
   return result;
 }
 
+namespace {
+// Holds a flag set for the duration of a scope; clears it even on early exit.
+class ScopedFlag
+{
+public:
+  explicit ScopedFlag(bool& flag)
+    : Flag(flag)
+  {
+    this->Flag = true;
+  }
+  ~ScopedFlag() { this->Flag = false; }
+  ScopedFlag(ScopedFlag const&) = delete;
+  ScopedFlag& operator=(ScopedFlag const&) = delete;
+
+private:
+  bool& Flag;
+};
+}
+
 QCMake::QCMake(QObject* p)
   : QObject(p)
   , StartEnvironment(QProcessEnvironment::systemEnvironment())
@@ -44,6 +63,7 @@ QCMake::QCMake(QObject* p)
   qRegisterMetaType<QCMakePropertyList>();
   qRegisterMetaType<QProcessEnvironment>();
   qRegisterMetaType<QVector<QCMakePreset>>();
+  qRegisterMetaType<quint64>("quint64");
 
   cmSystemTools::DisableRunCommandOutput();
   cmSystemTools::SetRunCommandHideConsole(true);
@@ -135,7 +155,9 @@ void QCMake::setBinaryDirectory(QString const& _dir)
     }
 
     QCMakePropertyList props = this->properties();
-    emit this->propertiesChanged(props);
+    if (!this->ApplyingPreset) {
+      emit this->propertiesChanged(props);
+    }
     cmValue homeDir = state->GetCacheEntryValue("CMAKE_HOME_DIRECTORY");
     if (homeDir) {
       setSourceDirectory(QString(homeDir->c_str()));
@@ -168,7 +190,9 @@ void QCMake::setPreset(QString const& name, bool setBinary)
 {
   if (this->PresetName != name) {
     this->PresetName = name;
-    emit this->presetChanged(this->PresetName);
+    if (!this->ApplyingPreset) {
+      emit this->presetChanged(this->PresetName);
+    }
 
     if (!name.isNull()) {
       std::string presetName(name.toStdString());
@@ -191,10 +215,24 @@ void QCMake::setPreset(QString const& name, bool setBinary)
         }
       }
     }
-    emit this->propertiesChanged(this->properties());
+    if (!this->ApplyingPreset) {
+      emit this->propertiesChanged(this->properties());
+    }
   }
 }
 
+void QCMake::applyPreset(QString const& name, bool setBinary,
+                         quint64 requestId)
+{
+  {
+    // Silence the intermediate, untagged notifications so the completion below
+    // is the only model update the UI trusts for this request.
+    ScopedFlag applying(this->ApplyingPreset);
+    this->setPreset(name, setBinary);
+  }
+  emit this->presetApplied(requestId, this->PresetName, this->properties());
+}
+
 void QCMake::setGenerator(QString const& gen)
 {
   if (this->Generator != gen) {

Source/QtDialog/QCMake.h

@@ -81,6 +81,8 @@ public slots:
   void setBinaryDirectory(QString const& dir);
   /// set the preset name to use
   void setPreset(QString const& name, bool setBinary = true);
+  /// apply a preset on behalf of the UI, emitting a tokenized completion
+  void applyPreset(QString const& name, bool setBinary, quint64 requestId);
   /// set the desired generator to use
   void setGenerator(QString const& generator);
   /// set the desired generator to use
@@ -149,6 +151,9 @@ signals:
   void presetsChanged(QVector<QCMakePreset> const& presets);
   /// signal when the selected preset changes
   void presetChanged(QString const& name);
+  /// signal when a tokenized preset application finishes, with its properties
+  void presetApplied(quint64 requestId, QString const& name,
+                     QCMakePropertyList const& vars);
   /// signal when there's an error reading the presets files
   void presetLoadError(QString const& dir, QString const& error);
   /// signal for progress events
@@ -195,4 +200,5 @@ protected:
   QAtomicInt InterruptFlag;
   QProcessEnvironment StartEnvironment;
   QProcessEnvironment Environment;
+  bool ApplyingPreset = false;
 };

Tests/CMakeGUI/CMakeGUITest.cmake

@@ -188,6 +188,38 @@ run_cmake_gui_test(presetArg:noExist
   )
 run_cmake_gui_test(changingPresets)
 
+run_cmake_gui_test(presetApplyOrdering:apply
+  DO_CONFIGURE
+  CONFIGURE_ARGS -DRACE_VALUE:STRING=old
+  ARGS
+    -S "${CMakeGUITest_BINARY_DIR}/presetApplyOrdering-apply/src"
+    -B "${CMakeGUITest_BINARY_DIR}/presetApplyOrdering-apply/build"
+  )
+run_cmake_gui_test(presetApplyOrdering:replace
+  DO_CONFIGURE
+  CONFIGURE_ARGS -DRACE_VALUE:STRING=old
+  ARGS
+    -S "${CMakeGUITest_BINARY_DIR}/presetApplyOrdering-replace/src"
+    -B "${CMakeGUITest_BINARY_DIR}/presetApplyOrdering-replace/build"
+  )
+run_cmake_gui_test(presetApplyOrdering:replaceCycle
+  DO_CONFIGURE
+  CONFIGURE_ARGS -DRACE_VALUE:STRING=old
+  ARGS
+    -S "${CMakeGUITest_BINARY_DIR}/presetApplyOrdering-replaceCycle/src"
+    -B "${CMakeGUITest_BINARY_DIR}/presetApplyOrdering-replaceCycle/build"
+  )
+run_cmake_gui_test(presetApplyStartup:available
+  ARGS
+    -S "${CMakeGUITest_BINARY_DIR}/presetApplyStartup-available/src"
+    "--preset=raceA"
+  )
+run_cmake_gui_test(presetApplyStartup:unavailable
+  ARGS
+    -S "${CMakeGUITest_BINARY_DIR}/presetApplyStartup-unavailable/src"
+    "--preset=doesNotExist"
+  )
+
 if("${CMakeGUITest_GENERATOR}" MATCHES "Make|Ninja|FASTBuild")
   run_cmake_gui_test(instrumentation)
   set(instrumentation_build_dir

Tests/CMakeGUI/CMakeGUITest.cxx

@@ -506,6 +506,138 @@ void CMakeGUITest::changingPresets()
   QCOMPARE(this->m_window->Preset->isEnabled(), false);
 }
 
+namespace {
+QString modelValue(QCMakeCacheModel* model, QString const& key)
+{
+  for (auto const& prop : model->properties()) {
+    if (prop.Key == key) {
+      return prop.Value.toString();
+    }
+  }
+  return QString{};
+}
+
+QString readCacheValue(QString const& buildDir, QString const& key)
+{
+  QFile cache(buildDir + "/CMakeCache.txt");
+  if (!cache.open(QIODevice::ReadOnly | QIODevice::Text)) {
+    return QString{};
+  }
+  QString const prefix = key + ":";
+  while (!cache.atEnd()) {
+    QString const line = QString::fromLocal8Bit(cache.readLine()).trimmed();
+    if (line.startsWith(prefix)) {
+      int const eq = line.indexOf('=');
+      if (eq >= 0) {
+        return line.mid(eq + 1);
+      }
+    }
+  }
+  return QString{};
+}
+}
+
+void CMakeGUITest::presetApplyOrdering()
+{
+  QFETCH(QString, scenario);
+  QFETCH(QString, buildDir);
+  QFETCH(QString, expectedValue);
+
+  auto* cmake = this->m_window->findChild<QCMakeThread*>()->cmakeInstance();
+
+  // Let the initial cache load from the preconfigured build settle.
+  loopSleep();
+
+  QSignalSpy configureDoneSpy(cmake, &QCMake::configureDone);
+  QVERIFY(configureDoneSpy.isValid());
+
+  // Select and configure in one main-thread run, with no yield between, to
+  // hit the race.
+  this->m_window->Preset->setPresetName("raceA");
+  if (scenario == "replace" || scenario == "replaceCycle") {
+    this->m_window->Preset->setPresetName("raceB");
+  }
+  if (scenario == "replaceCycle") {
+    this->m_window->Preset->setPresetName("raceA");
+  }
+
+  // Dependent actions must be off the instant the request is submitted.
+  QVERIFY(!this->m_window->ConfigureButton->isEnabled());
+  QVERIFY(!this->m_window->GenerateButton->isEnabled());
+
+  // Activating while a preset applies must not configure with the stale model.
+  this->m_window->ConfigureButton->click();
+  QCOMPARE(configureDoneSpy.count(), 0);
+
+  // Gate lifts only once the accepted completion has installed the model.
+  QTRY_VERIFY(this->m_window->ConfigureButton->isEnabled());
+
+  // The click during the pending window did nothing.
+  QCOMPARE(configureDoneSpy.count(), 0);
+
+  // Model holds the winning preset, not the preconfigured value.
+  QCOMPARE(modelValue(this->m_window->CacheValues->cacheModel(), "RACE_VALUE"),
+           expectedValue);
+
+  // A real configure writes that value through to the cache.
+  this->tryConfigure();
+  QCOMPARE(readCacheValue(buildDir, "RACE_VALUE"), expectedValue);
+}
+
+void CMakeGUITest::presetApplyOrdering_data()
+{
+  QTest::addColumn<QString>("scenario");
+  QTest::addColumn<QString>("buildDir");
+  QTest::addColumn<QString>("expectedValue");
+
+  QTest::newRow("apply") << "apply"
+                         << CMakeGUITest_BINARY_DIR
+    "/presetApplyOrdering-apply/build"
+                         << "presetA";
+  QTest::newRow("replace") << "replace"
+                           << CMakeGUITest_BINARY_DIR
+    "/presetApplyOrdering-replace/build"
+                           << "presetB";
+  QTest::newRow("replaceCycle")
+    << "replaceCycle"
+    << CMakeGUITest_BINARY_DIR "/presetApplyOrdering-replaceCycle/build"
+    << "presetA";
+}
+
+void CMakeGUITest::presetApplyStartup()
+{
+  QFETCH(QString, requestedPreset);
+  QFETCH(bool, available);
+
+  // The deferred gate must have the actions off at startup, before presets
+  // load.  Deterministic: pending was set on the main thread before the event
+  // loop processed any worker result.
+  QVERIFY(!this->m_window->ConfigureButton->isEnabled());
+  QVERIFY(!this->m_window->GenerateButton->isEnabled());
+
+  // Gate lifts once presets resolve, applied or not.
+  QTRY_VERIFY(this->m_window->ConfigureButton->isEnabled());
+  QVERIFY(this->m_window->GenerateButton->isEnabled());
+
+  if (available) {
+    QCOMPARE(this->m_window->Preset->presetName(), requestedPreset);
+    QCOMPARE(
+      modelValue(this->m_window->CacheValues->cacheModel(), "RACE_VALUE"),
+      QString("presetA"));
+  } else {
+    QVERIFY(this->m_window->Preset->presetName() != requestedPreset);
+  }
+}
+
+void CMakeGUITest::presetApplyStartup_data()
+{
+  QTest::addColumn<QString>("requestedPreset");
+  QTest::addColumn<bool>("available");
+
+  QTest::newRow("available") << "raceA" << true;
+  QTest::newRow("unavailable") << "doesNotExist" << false;
+}
+
 void SetupDefaultQSettings()
 {
   QSettings::setDefaultFormat(QSettings::IniFormat);

Tests/CMakeGUI/CMakeGUITest.h

@@ -28,4 +28,8 @@ private slots:
   void presetArg();
   void presetArg_data();
   void changingPresets();
+  void presetApplyOrdering();
+  void presetApplyOrdering_data();
+  void presetApplyStartup();
+  void presetApplyStartup_data();
 };

Tests/CMakeGUI/presetApplyOrdering-apply/CMakeLists.txt.in

@@ -0,0 +1,2 @@
+cmake_minimum_required(VERSION 3.18)
+project(presetApplyOrdering NONE)

Tests/CMakeGUI/presetApplyOrdering-apply/CMakePresets.json.in

@@ -0,0 +1,27 @@
+{
+  "version": 3,
+  "configurePresets": [
+    {
+      "name": "raceA",
+      "generator": "@CMakeGUITest_GENERATOR@",
+      "binaryDir": "${sourceParentDir}/build",
+      "cacheVariables": {
+        "RACE_VALUE": {
+          "type": "STRING",
+          "value": "presetA"
+        }
+      }
+    },
+    {
+      "name": "raceB",
+      "generator": "@CMakeGUITest_GENERATOR@",
+      "binaryDir": "${sourceParentDir}/build",
+      "cacheVariables": {
+        "RACE_VALUE": {
+          "type": "STRING",
+          "value": "presetB"
+        }
+      }
+    }
+  ]
+}

Tests/CMakeGUI/presetApplyOrdering-replace/CMakeLists.txt.in

@@ -0,0 +1,2 @@
+cmake_minimum_required(VERSION 3.18)
+project(presetApplyOrdering NONE)

Tests/CMakeGUI/presetApplyOrdering-replace/CMakePresets.json.in

@@ -0,0 +1,27 @@
+{
+  "version": 3,
+  "configurePresets": [
+    {
+      "name": "raceA",
+      "generator": "@CMakeGUITest_GENERATOR@",
+      "binaryDir": "${sourceParentDir}/build",
+      "cacheVariables": {
+        "RACE_VALUE": {
+          "type": "STRING",
+          "value": "presetA"
+        }
+      }
+    },
+    {
+      "name": "raceB",
+      "generator": "@CMakeGUITest_GENERATOR@",
+      "binaryDir": "${sourceParentDir}/build",
+      "cacheVariables": {
+        "RACE_VALUE": {
+          "type": "STRING",
+          "value": "presetB"
+        }
+      }
+    }
+  ]
+}

Tests/CMakeGUI/presetApplyOrdering-replaceCycle/CMakeLists.txt.in

@@ -0,0 +1,2 @@
+cmake_minimum_required(VERSION 3.18)
+project(presetApplyOrdering NONE)

Tests/CMakeGUI/presetApplyOrdering-replaceCycle/CMakePresets.json.in

@@ -0,0 +1,27 @@
+{
+  "version": 3,
+  "configurePresets": [
+    {
+      "name": "raceA",
+      "generator": "@CMakeGUITest_GENERATOR@",
+      "binaryDir": "${sourceParentDir}/build",
+      "cacheVariables": {
+        "RACE_VALUE": {
+          "type": "STRING",
+          "value": "presetA"
+        }
+      }
+    },
+    {
+      "name": "raceB",
+      "generator": "@CMakeGUITest_GENERATOR@",
+      "binaryDir": "${sourceParentDir}/build",
+      "cacheVariables": {
+        "RACE_VALUE": {
+          "type": "STRING",
+          "value": "presetB"
+        }
+      }
+    }
+  ]
+}

Tests/CMakeGUI/presetApplyStartup-available/CMakePresets.json.in

@@ -0,0 +1,16 @@
+{
+  "version": 3,
+  "configurePresets": [
+    {
+      "name": "raceA",
+      "generator": "@CMakeGUITest_GENERATOR@",
+      "binaryDir": "${sourceParentDir}/build",
+      "cacheVariables": {
+        "RACE_VALUE": {
+          "type": "STRING",
+          "value": "presetA"
+        }
+      }
+    }
+  ]
+}

Tests/CMakeGUI/presetApplyStartup-unavailable/CMakePresets.json.in

@@ -0,0 +1,16 @@
+{
+  "version": 3,
+  "configurePresets": [
+    {
+      "name": "raceA",
+      "generator": "@CMakeGUITest_GENERATOR@",
+      "binaryDir": "${sourceParentDir}/build",
+      "cacheVariables": {
+        "RACE_VALUE": {
+          "type": "STRING",
+          "value": "presetA"
+        }
+      }
+    }
+  ]
+}