Crack the project when its MSBuild inputs change, and only once

The design time build cache key took its list of dependent files from
MSBuildAllProjects. Since MSBuild 16.9 an import no longer adds itself to
that property, so the list was the fsproj plus a handful of SDK targets and
nothing else: editing a Directory.Build.props neither invalidated the cache
nor re-cracked the project, because nothing knew the file existed. The
convention imports are now asked for by name instead, and the packages props
only when central package management is actually on, so watching it cannot
re-crack for an edit that changes nothing.

The keys themselves were built once per daemon lifetime. Which files an
evaluation depends on is decided by the evaluation, so a key kept across
cracks describes the project as it was, and a file added to the build was
never noticed. They are now forgotten at the start of every crack, which
still leaves them memoised for the length of one, where the same project can
be visited more than once.

A dev server calls watchChange on top of calling hotUpdate for every
environment, so one touch of an fsproj arrived three times and cracked the
project three times over, each one a full design time build. Deduplicating a
change across environments already existed for source files; re-cracks now go
through the same window, and watchChange defers to hotUpdate in dev, where it
is the better of the two because it can reload the browser afterwards.

Reading msbuild's stdout to the end before touching stderr can deadlock: a
child that fills the other pipe blocks writing while this side blocks
reading, and nothing between the daemon and the plugin times out. Both pipes
are now drained together. A non-empty stderr also no longer fails the call,
since msbuild and NuGet write warnings there on runs that succeed, and a real
failure is reported on stdout with the exit code set, which is why the old
message quoted stderr and named no reason at all.

Author: nojaf

ROADMAP.md

@@ -172,7 +172,7 @@ The contract is hand-mirrored today and the mirroring is positional, which is wh
 
 ## 9. Cache invalidation questions
 
-- The design-time build cache key (`Caching.fs`) hashes MSBuild inputs only. Adding/removing a `<Compile Include>` changes the fsproj hash so that is covered, but `Directory.Build.props` files outside `MSBuildAllProjects` (for instance ones pulled in via `Import` with a condition that is false at evaluation time) are not.
+- The design-time build cache key (`Caching.fs`) hashes MSBuild inputs only, and the list of inputs comes from `MSBuildAllProjects`. Since MSBuild 16.9 an import no longer adds itself to that property, so it now reports the fsproj and a handful of SDK targets and nothing else. `Directory.Build.props`, `Directory.Build.targets` and `Directory.Packages.props` are asked for by name (`DirectoryBuildPropsPath` and friends), which covers what people actually edit, but a file pulled in by an explicit `<Import>` is still invisible: changing it neither invalidates the cache nor re-cracks the project. Getting the real list means `dotnet msbuild -preprocess` or an equivalent, which is a much heavier query than the one property read the cache key does today.
 - `tryCompileProject` compiles with `NoCache = true` in `CliArgs` while the daemon maintains its own cache; confirm nothing in newer Fable.Compiler versions relies on that flag for correctness.
 
 ## 10. Rejected: writing the plugin in F# / Fable

packages/vite-plugin-fable/Fable.Daemon.Tests/DebugTests.fs

@@ -130,6 +130,120 @@ let ``changing a signature file reports a diagnostic on the implementation`` ()
             (daemon :> IDisposable).Dispose()
     }
 
+/// MSBuild reports its own failures on stdout and leaves stderr empty, so the exit code is the
+/// only signal worth failing on, and a message built from stderr alone names no reason at all.
+[<Test>]
+let ``a failing dotnet msbuild call reports what MSBuild said`` () =
+    task {
+        let fsproj =
+            FileInfo (Path.CombineNormalize (__SOURCE_DIRECTORY__, "../../../sample-project/App.fsproj"))
+
+        let run =
+            MSBuild.dotnet_msbuild NullLogger.Instance fsproj "-t:NoSuchTarget"
+            |> Async.StartAsTask
+
+        // Also that it comes back at all: draining one pipe to the end before touching the other
+        // can deadlock, and nothing on the way back to the plugin times out.
+        let! finished = Task.WhenAny (run :> Task, Task.Delay (TimeSpan.FromMinutes 2.))
+        Assert.That (finished, Is.SameAs (run :> Task), "dotnet msbuild never came back")
+
+        let error = Assert.Throws<AggregateException>(fun () -> run.Wait ())
+
+        Assert.That (
+            error.InnerException.Message,
+            Does.Contain "MSB4057",
+            "the failure did not say what MSBuild complained about"
+        )
+    }
+
+/// Since MSBuild 16.9 an import no longer adds itself to `MSBuildAllProjects`, so asking for that
+/// property alone reports the fsproj and a handful of SDK targets and misses the file people
+/// actually edit. A `Directory.Build.props` that is not a dependent file is a design time build
+/// cache that survives an edit to it, and a file the plugin never watches.
+[<Test>]
+let ``the project's Directory.Build.props is reported as a file to watch`` () =
+    task {
+        let config = sampleApp
+        let projectDir = FileInfo(config.Project).Directory.FullName
+        Directory.SetCurrentDirectory projectDir
+
+        let directoryBuildProps =
+            Path.CombineNormalize (projectDir, "Directory.Build.props")
+
+        Assert.That (
+            File.Exists directoryBuildProps,
+            Is.True,
+            "the sample project no longer has a Directory.Build.props, so this test proves nothing"
+        )
+
+        let struct (serverStream, clientStream) = FullDuplexStream.CreatePair ()
+
+        let daemon =
+            new Program.FableServer (serverStream, serverStream, NullLogger.Instance)
+
+        let client = new JsonRpc (clientStream, clientStream)
+        client.StartListening ()
+
+        try
+            let! response = daemon.ProjectChanged config
+
+            match response with
+            | ProjectChangedResult.Error error -> Assert.Fail $"expected the project to crack, got {error}"
+            | ProjectChangedResult.Success (_, _, dependentFiles) ->
+
+            Assert.That (
+                dependentFiles |> Array.map Path.GetFullPath,
+                Does.Contain directoryBuildProps,
+                $"""Directory.Build.props was not reported: %s{String.concat ", " dependentFiles}"""
+            )
+        finally
+            client.Dispose ()
+            (daemon :> IDisposable).Dispose()
+    }
+
+/// The resolver forgets its cache keys at the start of every crack, so the next one asks MSBuild
+/// again which files its evaluation depends on. The risk in forgetting them is that nothing fills
+/// them back in: `MSBuildProjectFiles` would then answer with an empty list, the plugin would watch
+/// no MSBuild inputs at all, and editing the fsproj would stop re-cracking the project.
+[<Test>]
+let ``cracking twice still reports the MSBuild files to watch`` () =
+    task {
+        let config = sampleApp
+        Directory.SetCurrentDirectory (FileInfo(config.Project).DirectoryName)
+
+        let struct (serverStream, clientStream) = FullDuplexStream.CreatePair ()
+
+        let daemon =
+            new Program.FableServer (serverStream, serverStream, NullLogger.Instance)
+
+        let client = new JsonRpc (clientStream, clientStream)
+        client.StartListening ()
+
+        let dependentFiles (response : ProjectChangedResult) =
+            match response with
+            | ProjectChangedResult.Success (_, _, dependentFiles) -> dependentFiles
+            | ProjectChangedResult.Error error -> failwith $"expected the project to crack, got {error}"
+
+        try
+            let! first = daemon.ProjectChanged config
+            let! second = daemon.ProjectChanged config
+
+            for response in [ first ; second ] do
+                let files = dependentFiles response
+
+                Assert.That (files, Is.Not.Empty, "no MSBuild files were reported to watch")
+
+                Assert.That (
+                    files
+                    |> Array.exists (fun f -> f.EndsWith ("App.fsproj", StringComparison.Ordinal)),
+                    Is.True,
+                    $"""the project file itself was not reported: %s{String.concat ", " files}"""
+                )
+        finally
+            client.Dispose ()
+            (daemon :> IDisposable).Dispose()
+    }
+
 /// The daemon keeps Fable's `File` values between compiles so an unchanged file is not read and
 /// hashed again, and forgets the ones the plugin reports as changed. A change it failed to forget
 /// would be compiled from what the file used to say, and nothing else would notice: no error, just

packages/vite-plugin-fable/Fable.Daemon/Caching.fs

@@ -218,6 +218,13 @@ let private decodeCacheKey (options : CrackerOptions) (fsproj : FileInfo) (json
         let getProperty (name : string) =
             properties.GetProperty(name).GetString()
 
+        /// MSBuild answers with an empty string for a property it has no value for, so this is only
+        /// about a property that was never asked for.
+        let tryGetProperty (name : string) : string =
+            match properties.TryGetProperty name with
+            | true, value -> value.GetString () |> Option.ofObj |> Option.defaultValue ""
+            | false, _ -> ""
+
         let paths =
             (getProperty "MSBuildAllProjects").Split(';', StringSplitOptions.RemoveEmptyEntries)
             |> Array.choose (fun path ->
@@ -247,8 +254,41 @@ let private decodeCacheKey (options : CrackerOptions) (fsproj : FileInfo) (json
         let cacheFile =
             FileInfo (Path.Combine (intermediateOutputPath, $"{fsproj.Name}%s{DesignTimeBuildExtension}"))
 
+        /// The files MSBuild imports by convention rather than by an `<Import>` in the project.
+        ///
+        /// They have to be asked for by name: since MSBuild 16.9 imports no longer add themselves
+        /// to `MSBuildAllProjects`, so a `Directory.Build.props` does not appear there at all.
+        /// Without these, editing one changed nothing the cache key knew about, the design time
+        /// build was reused, and the plugin was never told to watch the file either.
+        let conventionImports =
+            [
+                yield "DirectoryBuildPropsPath"
+                yield "DirectoryBuildTargetsPath"
+                // The SDK resolves this whether or not central package management is on. When it is
+                // off the file is found and ignored, and watching it would re-crack the project for
+                // an edit that cannot change anything.
+                if
+                    String.Equals (
+                        tryGetProperty "ManagePackageVersionsCentrally",
+                        "true",
+                        StringComparison.OrdinalIgnoreCase
+                    )
+                then
+                    yield "DirectoryPackagesPropsPath"
+            ]
+            |> List.choose (fun property ->
+                let path = tryGetProperty property
+
+                if String.IsNullOrWhiteSpace path then
+                    None
+                else
+
+                let fi = FileInfo path
+                if fi.Exists then Some fi else None
+            )
+
         let dependentFiles =
-            [ yield fsproj ; yield! paths ; yield! nugetGProps ]
+            [ yield fsproj ; yield! paths ; yield! conventionImports ; yield! nugetGProps ]
             |> List.distinctBy (fun fi -> fi.FullName)
 
         Ok
@@ -285,7 +325,7 @@ let mkProjectCacheKey
             MSBuild.dotnet_msbuild
                 logger
                 fsproj
-                $"/p:Configuration=%s{options.Configuration} --getProperty:MSBuildAllProjects --getProperty:IntermediateOutputPath --getProperty:MSBuildProjectExtensionsPath"
+                $"/p:Configuration=%s{options.Configuration} --getProperty:MSBuildAllProjects --getProperty:IntermediateOutputPath --getProperty:MSBuildProjectExtensionsPath --getProperty:DirectoryBuildPropsPath --getProperty:DirectoryBuildTargetsPath --getProperty:DirectoryPackagesPropsPath --getProperty:ManagePackageVersionsCentrally"
 
         return decodeCacheKey options fsproj json
     }

packages/vite-plugin-fable/Fable.Daemon/MSBuild.fs

@@ -21,14 +21,49 @@ let dotnet_msbuild (logger : ILogger) (fsproj : FileInfo) (args : string) : Asyn
 
         use ps = new Process ()
         ps.StartInfo <- psi
-        ps.Start () |> ignore
-        let output = ps.StandardOutput.ReadToEnd ()
-        let error = ps.StandardError.ReadToEnd ()
+
+        if not (ps.Start ()) then
+            failwith $"Could not start `dotnet msbuild` for %s{fsproj.FullName}"
+
+        // Both pipes have to be drained at the same time. Reading one to the end first blocks here
+        // until the child closes it, and a child that fills the other pipe's buffer meanwhile
+        // blocks writing to it: neither side moves again. Nothing times out on the way back, so
+        // the plugin would wait on a design time build that never finishes.
+        let readOutput = ps.StandardOutput.ReadToEndAsync ()
+        let readError = ps.StandardError.ReadToEndAsync ()
+        let! output = readOutput
+        let! error = readError
         do! ps.WaitForExitAsync ()
 
-        if ps.ExitCode <> 0 || not (String.IsNullOrWhiteSpace error) then
-            logger.LogCritical ("dotnet msbuild \"{fsproj}\" {args}\n did has {error}", fsproj.FullName, args, error)
-            failwithf $"In %s{pwd}:\ndotnet msbuild \"%s{fsproj.FullName}\" %s{args} failed with\n%s{error}"
+        // The exit code decides, and only the exit code. MSBuild and NuGet write warnings to
+        // stderr on runs that succeed, and failing on those reported a warning as a project that
+        // could not be cracked.
+        if ps.ExitCode <> 0 then
+            // MSBuild reports its own failures on stdout (`error MSB4057: ...`) and typically
+            // leaves stderr empty, so a message built from stderr alone names no reason at all.
+            let detail =
+                [ output ; error ]
+                |> List.filter (String.IsNullOrWhiteSpace >> not)
+                |> String.concat "\n"
+
+            logger.LogCritical (
+                "dotnet msbuild \"{fsproj}\" {args} exited with {exitCode}:\n{detail}",
+                fsproj.FullName,
+                args,
+                ps.ExitCode,
+                detail
+            )
+
+            failwithf
+                $"In %s{pwd}:\ndotnet msbuild \"%s{fsproj.FullName}\" %s{args} failed with exit code %i{ps.ExitCode}\n%s{detail}"
+
+        if not (String.IsNullOrWhiteSpace error) then
+            logger.LogWarning (
+                "dotnet msbuild \"{fsproj}\" {args} wrote to stderr:\n{error}",
+                fsproj.FullName,
+                args,
+                error
+            )
 
         return output.Trim ()
     }

packages/vite-plugin-fable/Fable.Daemon/Program.fs

@@ -153,6 +153,12 @@ let tryTypeCheckProject
 
                 cliArgs, CrackerOptions (cliArgs, true)
 
+            // Which files the MSBuild evaluation depends on is decided by the evaluation, so the
+            // keys from the previous crack describe the project as it was. Adding a
+            // `Directory.Build.props` or an `<Import>` changes that list, and reusing the old key
+            // would compare the new project against the old one's inputs and find them unchanged.
+            model.Resolver.ForgetCacheKeys ()
+
             let crackerResponse = getFullProjectOpts model.Resolver crackerOptions
 
             logger.LogDebug ("CrackerResponse: {crackerResponse}", crackerResponse)

packages/vite-plugin-fable/Fable.Daemon/ProjectCracking.fs

@@ -33,6 +33,17 @@ type CachedMSBuildCrackerResolver(logger : ILogger) =
         | None -> ()
         | Some cacheKey -> Caching.writeFableModulesFromCache cacheKey fableModuleFiles
 
+    /// Drop the remembered cache keys, so the next crack asks MSBuild again which files its
+    /// evaluation depends on.
+    ///
+    /// A key is worth remembering for the length of one crack, where the same project can be
+    /// visited more than once, but not beyond it: `MSBuildAllProjects` is only re-read when a key
+    /// is built, so a key kept across cracks describes the project as it was. Add a
+    /// `Directory.Build.props` or an `<Import>` and the stale key compares the new project against
+    /// the old one's inputs, finds nothing changed, and reuses a design time build that predates
+    /// the file. The new file is also never reported to the plugin, so nothing watches it.
+    member x.ForgetCacheKeys () : unit = cached.Clear ()
+
     /// Get project files to watch inside the plugin
     /// These are the fsproj and potential MSBuild import files
     member x.MSBuildProjectFiles (fsproj : FullPath) : FileInfo list =

packages/vite-plugin-fable/src/index.ts

@@ -375,14 +375,16 @@ export function createFablePlugin(
   }
 
   /**
-   * The compile for one file change, shared by every environment Vite reports it to.
+   * The work for one file change, shared by every environment Vite reports it to.
    *
    * `handleHMRUpdate` takes a single timestamp per change (`server/hmr.ts:472`) and then hands it
    * to `hotUpdate` once per environment (`:667-676`). Those calls arrive one after another, not
    * together, so the coalescing window has already flushed by the time the second one lands: a dev
    * server with the default `client` and `ssr` environments compiled every edit twice. Keying on
    * the timestamp Vite already computed makes one filesystem change mean one compile, while each
-   * environment still resolves the resulting files against its own module graph.
+   * environment still resolves the resulting files against its own module graph. The same
+   * applies to a re-crack: an MSBuild input reported to two environments cracked the project
+   * twice.
    *
    * A map rather than a single entry, because fan-outs overlap. The watcher calls
    * `onFileChange(file).catch(...)` without awaiting it (`server/index.ts:960-962`), so saving two
@@ -391,22 +393,27 @@ export function createFablePlugin(
    * environment only asks after the previous one's `hotUpdate` returned, which is already after
    * the compile that one awaited.
    */
-  const sharedSourceChanges: Map<string, Promise<BatchResult>> = new Map();
+  const sharedChanges: Map<string, Promise<BatchResult>> = new Map();
 
   /** Room for far more overlap than one fan-out can produce; this is a window, not a cache. */
-  const SHARED_SOURCE_CHANGE_LIMIT: number = 32;
-
-  function queueSourceChangeOnce(file: string, timestamp: number): Promise<BatchResult> {
+  const SHARED_CHANGE_LIMIT: number = 32;
+
+  /** Queues the work for a change once, however many environments Vite reports it to. */
+  function queueOnce(
+    file: string,
+    timestamp: number,
+    queue: (file: string) => Promise<BatchResult>,
+  ): Promise<BatchResult> {
     const key = `${file}\u0000${timestamp}`;
-    const pending: Promise<BatchResult> | undefined = sharedSourceChanges.get(key);
+    const pending: Promise<BatchResult> | undefined = sharedChanges.get(key);
     if (pending) return pending;
-    const result: Promise<BatchResult> = queueSourceChange(file);
-    sharedSourceChanges.set(key, result);
+    const result: Promise<BatchResult> = queue(file);
+    sharedChanges.set(key, result);
     // One insert can only put it one over, and a map iterates in insertion order, so this drops
     // the oldest change still on record.
-    if (sharedSourceChanges.size > SHARED_SOURCE_CHANGE_LIMIT) {
-      for (const oldest of sharedSourceChanges.keys()) {
-        sharedSourceChanges.delete(oldest);
+    if (sharedChanges.size > SHARED_CHANGE_LIMIT) {
+      for (const oldest of sharedChanges.keys()) {
+        sharedChanges.delete(oldest);
         break;
       }
     }
@@ -663,8 +670,13 @@ export function createFablePlugin(
         };
       },
     },
-    // `hotUpdate` covers dev; this is what reaches the plugin under `vite build --watch`.
+    // What reaches the plugin under `vite build --watch`. A dev server calls this as well
+    // (`server/index.ts:912`, once for the client environment per `pluginContainer.ts:1252-1254`)
+    // on top of calling `hotUpdate` for every environment, and each call used to crack the project
+    // again: one touch of an fsproj meant three full re-cracks. `hotUpdate` is the better hook of
+    // the two in dev, because it can reload the browser afterwards, so this defers to it there.
     watchChange: async function (id: string): Promise<void> {
+      if (!state.isBuild) return;
       if (state.sourceFiles.size !== 0 && state.dependentFiles.has(normalizePath(id))) {
         await queueProjectChange(normalizePath(id));
       }
@@ -685,7 +697,7 @@ export function createFablePlugin(
       // "the whole project was rebuilt".
       if (state.dependentFiles.has(normalized)) {
         logDebug("hotUpdate", `project file ${short(normalized)} changed`);
-        await queueProjectChange(normalized);
+        await queueOnce(normalized, timestamp, queueProjectChange);
         environment.hot.send({ type: "full-reload" });
         return [];
       }
@@ -697,15 +709,15 @@ export function createFablePlugin(
       if (type !== "update") {
         if (!sourceFile && !fsharpFileRegex.test(normalized)) return;
         logDebug("hotUpdate", `${short(normalized)} was ${type}d, re-cracking`);
-        await queueProjectChange(normalized);
+        await queueOnce(normalized, timestamp, queueProjectChange);
         environment.hot.send({ type: "full-reload" });
         return [];
       }
 
       if (!sourceFile) return;
 
       logDebug("hotUpdate", `enter for ${short(sourceFile)}`);
-      const result: BatchResult = await queueSourceChangeOnce(sourceFile, timestamp);
+      const result: BatchResult = await queueOnce(sourceFile, timestamp, queueSourceChange);
       logDebug("hotUpdate", `leave for ${short(sourceFile)}`);
 
       const errorDiagnostic: Diagnostic | undefined = result.diagnostics.find(