Hold the server and watch starts while a build runs, and keep a failed build across restarts

docs/guide/running-the-site.md

@@ -16,7 +16,7 @@ When the server is ready, its URL appears below the button, for example `http://
 
 ![The site view with the dev server running: in the details, under Server, a Stop button, the View site, wp-admin and Database links, and the admin credentials with the password hidden](/screenshots/dev-server-running.png)
 
-The button is disabled while a trunk update is running. The reverse is no longer true: an update can run with the server up, because it pauses the build watch for the rebuild and leaves the server serving — see [Applying patches and PRs](./applying-patches) and [Staying up to date with trunk](./trunk-updates).
+The button is disabled while a trunk update is running, and while `npm run build` is running, whether you typed it in the terminal or a patch, a pull request or a trunk update started it: the server would serve a `build/` that is still being written. A site set to start its server or watch when opened waits for that build to end. The reverse is no longer true: an update can run with the server up, because it pauses the build watch for the rebuild and leaves the server serving — see [Applying patches and PRs](./applying-patches) and [Staying up to date with trunk](./trunk-updates).
 
 ## Logging in
 
@@ -54,7 +54,7 @@ The watch and the server are independent in both directions:
 - On a Gutenberg site that is already built, starting the dev server does not start the watch: the server serves the `build/` the site has, and the URL appears in seconds. The watch is Gutenberg's `npm run dev`, which removes `build/` and rebuilds it (about twenty seconds on macOS, several minutes on Windows) before it watches, and on a built site that rebuild produces what was already there. Start it with **Start build watch** when you want edits under `packages/` compiled on save; applying a pull request or a patch, and updating trunk, rebuild on their own when no watch is running. On a Gutenberg site that has never been built, the watch still runs first, and the server waits for the **Build watcher** tab to print *Watching for changes* before the URL appears; a page opened before that would find the plugin unbuilt. The same happens after a rebuild that was cut short, a watch stopped or exited while its tab read *(building)*: `build/` may be incomplete, so the next server start sends the watch first and waits for it. Starting the watch by hand while a server is already up reopens that window: the site answers with Gutenberg's *requires files to be built* notice, or a PHP fatal, until the rebuild finishes.
 - Stopping the dev server leaves the watch running, so you can keep compile-on-save going without a server.
 - The watch exiting never touches the server.
-- You can start and stop the watch on its own, at any time, whether or not a server is up.
+- You can start and stop the watch on its own, whether or not a server is up. Its Start button is disabled while `npm run build` is running, for the same reason as the server's; its Stop button never is.
 
 While the watch is running, applying a patch, switching work items or updating trunk uses it rather than fighting it. A patch that does not move `package-lock.json` is applied and left for the watch to recompile — the checklist shows the build step skipped and names the watch as doing it. One that does move the lockfile has to install, so it pauses the watch for the duration and resumes it after, with the PHP server up throughout. On Core the install is followed by a build; on Gutenberg a patch's resumed watch rebuilds from scratch on its own, so the patch runs no separate build and the checklist says the resumed watch is doing it. A trunk update follows the same rule: on Core it builds itself before the watch resumes, on Gutenberg it resumes the watch and lets that rebuild be the one build, and the update completes on the watch's ready line. Linking, unlinking or switching a [work item](./ticket-branches) only moves the checkout, so the watch is left running and recompiles the files that changed; restoring a pull request you had parked on a work item is the one switch that pauses it, because it installs and builds afterwards, as does any switch made before the watch has finished starting.
 

src/main.js

@@ -3199,9 +3199,9 @@ ipcMain.handle('site:status', async (_e, sitePath) => {
 			}
 			: null;
 
-		return { hasNodeModules, hasBuilt, projectType: project.id, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), trunkOid, trunkDate, updateIncomplete: Boolean(work.updateIncomplete), tracTicket: m.tracTicket || null, ticketBehindTrunk, appliedPatch, pullRequest, legacy, mergeInProgress: merging };
+		return { hasNodeModules, hasBuilt, projectType: project.id, skipInitWizard: Boolean(m.skipInitWizard), initialized: Boolean(m.initialized), installFailed: Boolean(m.installFailed), buildFailed: Boolean(m.buildFailed), trunkOid, trunkDate, updateIncomplete: Boolean(work.updateIncomplete), tracTicket: m.tracTicket || null, ticketBehindTrunk, appliedPatch, pullRequest, legacy, mergeInProgress: merging };
 	} catch {
-		return { hasNodeModules: false, hasBuilt: false, projectType: getProjectType().id, skipInitWizard: false, initialized: false, installFailed: false, trunkOid: null, trunkDate: null, updateIncomplete: false, tracTicket: null, ticketBehindTrunk: false, appliedPatch: null, pullRequest: null, legacy: false, mergeInProgress: null };
+		return { hasNodeModules: false, hasBuilt: false, projectType: getProjectType().id, skipInitWizard: false, initialized: false, installFailed: false, buildFailed: false, trunkOid: null, trunkDate: null, updateIncomplete: false, tracTicket: null, ticketBehindTrunk: false, appliedPatch: null, pullRequest: null, legacy: false, mergeInProgress: null };
 	}
 });
 
@@ -4244,7 +4244,16 @@ ipcMain.handle('npm:run-script', async (event, directoryPath, scriptName, script
 		onLog: (type, data) => {
 			event.sender.send('npm:run-script:log', { runId, type, data });
 		},
-		onDone: (code) => {
+		onDone: async (code) => {
+			// The build's counterpart to installFailed, recorded before the done
+			// event for the same reason (#503): build/'s marker file is written
+			// part-way through, so a build that failed late reads as complete
+			// unless its outcome outlives the session.
+			if (scriptName === 'build') {
+				try { await mergeSiteMeta(directoryPath, { buildFailed: code !== 0 }); } catch (e) {
+					logError('npm', `could not record how the build in ${directoryPath} ended (code ${code}): ${String(e && e.message ? e.message : e)}`);
+				}
+			}
 			event.sender.send('npm:run-script:done', { runId, code });
 			untrackDirectoryChild(directoryPath, runningScripts[runId]);
 			delete runningScripts[runId];

src/renderer/dev-server-command.cjs

@@ -71,8 +71,9 @@ function watchReadyPattern(build) {
  * rebuilding (`buildInterrupted`), may have left build/ empty or half
  * written, and the marker file the status reads is not proof otherwise. In
  * those cases the watch goes first and completes build/, as before #499. A
- * paused watch means an apply is building on its own; serving meanwhile is
- * what the guide already promises.
+ * paused watch means an apply is installing or building on its own. While it
+ * installs, serving is what the guide already promises; while it builds, the
+ * start is held before it gets here (#503, buildHoldsStarts).
  *
  * @param {{hasBuilt?: boolean, watchState?: string, buildInterrupted?: boolean}} [flags]
  * @param {{watch: {readyPattern?: string}}}                                      [build]

src/renderer/hooks/use-build-watch.jsx

@@ -39,6 +39,9 @@ export function useBuildWatch({ sitePath, projectBuild, hasBuilt, runScript, kil
   // build that has to end well before a watcher is started. The page says
   // different things of the two, and only one of them was ever a watch.
   const [watchExitOf, setWatchExitOf] = useState('watch');
+  // Set while the one-off build below runs, so a build the watch started is
+  // told from one started beside it (#503).
+  const [watchOwnsBuild, setWatchOwnsBuild] = useState(false);
   // Ref mirror for the inline reads (guards, callbacks) that must not wait for a
   // re-render, the same split as the terminal's terminalRunning and
   // terminalStateRef.
@@ -230,13 +233,15 @@ export function useBuildWatch({ sitePath, projectBuild, hasBuilt, runScript, kil
       terminalKillRef.current = () => { markWatchState('idle'); killCurrent().catch(() => {}); };
       // translators: %s: the command that builds the site, npm run build.
       appendWatch(`${sprintf(__('No completed build found — running %s first…'), 'npm run build')}\n`);
+      setWatchOwnsBuild(true);
       runScript('build', {
         mirrorToNpm: false,
         // It prints in the watch's log, and that is where its failure says
         // to look: the terminal is not brought up for it.
         outputInTerminal: false,
         onLog: (chunk) => { appendWatch(chunk); },
         onDone: ({ code }) => {
+          setWatchOwnsBuild(false);
           markTerminalRunning(false);
           terminalKillRef.current = null;
           // A stop that was asked for, by the watch's button or by Ctrl+C,
@@ -326,6 +331,7 @@ export function useBuildWatch({ sitePath, projectBuild, hasBuilt, runScript, kil
     watchState,
     watchExitCode,
     watchExitOf,
+    watchOwnsBuild,
     watchCompiling,
     watchStateRef,
     watchWaitersRef,

src/renderer/hooks/use-site-scripts.jsx

@@ -3,15 +3,15 @@ import { runFailedInTerminal } from '../terminal-hints.cjs';
 
 // The npm runs a site's view starts (#554): npm install and the site's npm
 // scripts, with what the rest of the view needs to know about them. Whether an
-// install or a build is running, whether the last build failed, whether the
-// last thing to write build/ was cut short, and which run Ctrl+C or a stop
-// should reach.
+// install or a build is running, whether the last thing to write build/ was
+// cut short, and which run Ctrl+C or a stop should reach.
 //
 // Everything that runs one goes through here: the terminal, the setup, a trunk
 // update, an applied patch, a ticket switch and the build watch. They pass
 // `onLog` and `onDone` for what they do with a run's output and its end; what
 // is common to every run is here. An install that ends well marks the site
-// initialised, an install or a build that ends reloads the site's status,
+// initialised, an install or a build that ends reloads the site's status
+// (which is where whether the last build failed is read from, #503),
 // and a run's output is copied into the install log's buffer unless its
 // caller says not to, as the build watch does; `appendNpm` and `ensureStick`
 // are for that buffer.
@@ -33,11 +33,6 @@ import { runFailedInTerminal } from '../terminal-hints.cjs';
 export function useSiteScripts({ sitePath, appendNpm, ensureStick, loadStatus, onInitialized, onRunFailed }) {
   const [installing, setInstalling] = useState(false);
   const [building, setBuilding] = useState(false);
-  // The build's counterpart to installFailed — session-local, because only the
-  // install outcome is persisted (main.js records it on the site's meta). After
-  // a restart a failed build reads "Ready" again, which is the honest fallback:
-  // the app knows there is no build on disk, just not that the last attempt lost.
-  const [buildFailed, setBuildFailed] = useState(false);
   // True while the last thing to touch build/ was a watch rebuild that did not
   // finish: stopped or crashed while 'building'. build/ may then be empty or
   // half written whatever the status's marker file says, so the server does
@@ -94,20 +89,22 @@ export function useSiteScripts({ sitePath, appendNpm, ensureStick, loadStatus, o
   const runScript = useCallback((name, options = {}) => {
     const { onLog, onDone, args = [], track = true, mirrorToNpm = true, outputInTerminal = true, onStart } = options;
     ensureStick('npm');
-    // Clearing the failure here rather than on the next exit is what stops the
-    // step reading "Failed" while its own retry is streaming to the terminal.
-    if (name === 'build') { setBuilding(true); setBuildFailed(false); }
+    // The step does not read "Failed" while its own retry streams to the
+    // terminal: setup-steps holds the failure back while `building` is set.
+    if (name === 'build') setBuilding(true);
     if (track) { currentRunIdRef.current = null; stopRequestedRef.current = false; }
     return window.api.runNpmScript(sitePath, name, args, ({ data }) => {
       if (mirrorToNpm) appendNpm(data);
       if (onLog) onLog(data);
     }, async ({ code }) => {
       if (mirrorToNpm) appendNpm(`\n${name} exited with code ${code}\n`);
       if (name === 'build') {
-        setBuilding(false);
-        setBuildFailed(code !== 0);
         if (code === 0) markBuildInterrupted(false);
+        // The status first: main.js has recorded the outcome before this
+        // event, and `building` going false with the last build's outcome
+        // still on screen would show it for a moment.
         try { await loadStatus(); } catch {}
+        setBuilding(false);
         if (runFailedInTerminal({ code, outputInTerminal, stopRequested: stopRequestedRef.current })) onRunFailed();
       }
       if (track) currentRunIdRef.current = null;
@@ -139,7 +136,6 @@ export function useSiteScripts({ sitePath, appendNpm, ensureStick, loadStatus, o
   return {
     installing,
     building,
-    buildFailed,
     buildInterrupted,
     buildInterruptedRef,
     markBuildInterrupted,

src/renderer/hooks/use-site-status.jsx

@@ -30,11 +30,15 @@ import { useCallback, useEffect, useState } from 'react';
 // `ticketBehindTrunk` the same way; the dev server's start asks the main
 // process itself whether the site is built and copies that into `hasBuilt`;
 // skipping the first-run checklist sets `skipInit`; and discarding every
-// change clears `appliedPatch`, with no read after it. The other eight change
+// change clears `appliedPatch`, with no read after it. The other nine change
 // only through `loadStatus`.
 export function useSiteStatus({ sitePath, metaPatchRef }) {
   const [hasNodeModules, setHasNodeModules] = useState(false);
   const [installFailed, setInstallFailed] = useState(false);
+  // How the last npm run build ended, kept in the site's meta by main.js like
+  // the install's (#503): build/'s marker is written part-way through a build,
+  // so without it a build that failed late read as complete after a restart.
+  const [buildFailed, setBuildFailed] = useState(false);
   const [hasBuilt, setHasBuilt] = useState(false);
   const [skipInit, setSkipInit] = useState(false);
   const [statusLoading, setStatusLoading] = useState(true);
@@ -60,6 +64,7 @@ export function useSiteStatus({ sitePath, metaPatchRef }) {
       const s = await window.api.getSiteStatus(sitePath);
       setHasNodeModules(Boolean(s?.hasNodeModules));
       setInstallFailed(Boolean(s?.installFailed));
+      setBuildFailed(Boolean(s?.buildFailed));
       setHasBuilt(Boolean(s?.hasBuilt));
       setSkipInit(Boolean(s?.skipInitWizard));
       setTrunkDate(s?.trunkDate || null);
@@ -90,6 +95,7 @@ export function useSiteStatus({ sitePath, metaPatchRef }) {
   return {
     hasNodeModules,
     installFailed,
+    buildFailed,
     hasBuilt,
     setHasBuilt,
     skipInit,