Do not attempt to load subpackages nested under malformed config

Author: fsoikin

src/Spago/Config.purs

@@ -182,9 +182,13 @@ discoverWorkspace options cwd = do
   logInfo "Reading Spago workspace configuration..."
   logDebug $ "Discovering nearest workspace " <> spagoYaml <> " starting at " <> Path.quote cwd
 
-  { workspace, rootPath } /\ { loadedPackages, closestPackage } <-
+  { workspace, rootPath } /\ upTree@{ closestPackage } <-
     State.runStateT (walkDirectoriesUpFrom cwd)
-      { loadedPackages: Map.empty, otherWorkspaceRoots: [], misnamedConfigs: [], closestPackage: Nothing }
+      { loadedPackages: Map.empty, misnamedConfigs: [], closestPackage: Nothing }
+
+  { loadedPackages } <-
+    State.execStateT (loadSubprojectConfigs rootPath)
+      { loadedPackages: upTree.loadedPackages, blockedSubtrees: [] }
 
   migrateConfigsWhereNeeded rootPath loadedPackages
 
@@ -229,6 +233,7 @@ discoverWorkspace options cwd = do
         }
     }
   where
+  readConfig' :: ∀ s. _ -> State.StateT s _ _
   readConfig' = State.lift <<< readConfig
 
   walkDirectoriesUpFrom dir = do
@@ -249,7 +254,6 @@ discoverWorkspace options cwd = do
       Just { doc, yaml: { workspace: Just workspace, package } } -> do
         -- Finally, found the "workspace" config!
         rootPath <- Path.mkRoot dir
-        loadSubprojectConfigs rootPath
         pure { workspace: { config: workspace, doc, rootPackage: package }, rootPath }
       _ -> do
         -- No workspace in this directory => recur to parent directory (unless it's already root)
@@ -277,19 +281,20 @@ discoverWorkspace options cwd = do
       let
         configFile = dir </> spagoYaml
         alreadyLoaded = st.loadedPackages # Map.member configFile
-        anotherParentWorkspace = st.otherWorkspaceRoots # Array.find (_ `Path.isPrefixOf` dir)
-      case alreadyLoaded, anotherParentWorkspace of
+        blockedSubtree = st.blockedSubtrees # Array.find (_ `Path.isPrefixOf` dir)
+      case alreadyLoaded, blockedSubtree of
         true, _ ->
           pure unit
-        _, Just ws -> do
-          logDebug $ "Not trying to load " <> Path.quote configFile <> " because it belongs to a different workspace at " <> Path.quote ws
+        _, Just b -> do
+          logDebug $ "Not trying to load " <> Path.quote configFile <> " because it is nested under " <> Path.quote b
           pure unit
         false, Nothing ->
           readConfig' configFile >>= case _ of
-            Left _ ->
-              logWarn $ "Failed to read config at " <> Path.quote configFile
+            Left _ -> do
+              logWarn [ "Failed to read config at " <> Path.quote configFile, "Skipping it and all its subprojects" ]
+              State.modify_ \s -> s { blockedSubtrees = Array.cons dir s.blockedSubtrees }
             Right { yaml: { workspace: Just _ } } ->
-              State.modify_ \s -> s { otherWorkspaceRoots = Array.cons dir s.otherWorkspaceRoots }
+              State.modify_ \s -> s { blockedSubtrees = Array.cons dir s.blockedSubtrees }
             Right config@{ yaml: { package: Just package } } -> do
               logDebug $ "Loaded a subproject config at " <> Path.quote configFile
               State.modify_ \s -> s { loadedPackages = Map.insert dir { package, config } s.loadedPackages }

test-fixtures/config/malformed-configs/a/b/c/spago.yaml

@@ -0,0 +1,3 @@
+package:
+  name: c
+  dependencies: []

test-fixtures/config/malformed-configs/a/b/spago.yaml

@@ -0,0 +1,3 @@
+package:
+  title: b # `title` is not a valid field, this config will error out on parsing.
+  dependencies: []

test-fixtures/config/malformed-configs/a/d/spago.yaml

@@ -0,0 +1,3 @@
+package:
+  name: d
+  dependencies: []

test-fixtures/config/malformed-configs/a/spago.yaml

@@ -0,0 +1,3 @@
+package:
+  name: a
+  dependencies: []

test-fixtures/config/malformed-configs/from-root.txt

@@ -0,0 +1,9 @@
+Reading Spago workspace configuration...
+‼ Failed to read config at "<test-dir>/a/b/spago.yaml"
+Skipping it and all its subprojects
+
+✘ Selected package bogus was not found in the local packages.
+All available packages:
+  a
+  d
+  root

test-fixtures/config/malformed-configs/spago.yaml

@@ -0,0 +1,6 @@
+package:
+  name: root
+  dependencies: []
+workspace:
+  packageSet:
+    registry: 0.0.1

test/Spago/Config.purs

@@ -11,6 +11,7 @@ import Registry.Version as Version
 import Spago.Core.Config (SetAddress(..))
 import Spago.Core.Config as C
 import Spago.FS as FS
+import Spago.Path as Path
 import Spago.Paths as Paths
 import Spago.Yaml as Yaml
 import Test.Spec (Spec)
@@ -118,6 +119,20 @@ spec =
           Paths.chdir $ testCwd </> "a" </> "b" </> "d"
           spago [ "build" ] >>= shouldBeFailureErr' (fixture "config/misnamed-configs/from-d.txt")
 
+        Spec.it "warns about a malformed config, but stops parsing down the tree" \{ spago, fixture, testCwd } -> do
+          FS.copyTree { src: fixture "config/malformed-configs", dst: testCwd }
+
+          -- Running with "-p bogus" to get Spago to list all available
+          -- packages. Packages `b` and `c` shouldn't be in that list because
+          -- b's config is malformatted, so Spago should warn about it and stop
+          -- loading configs down the tree from `b`, thus skipping `c`.
+          spago [ "build", "-p", "bogus" ] >>= checkOutputs'
+            { result: isLeft
+            , stdoutFile: Nothing
+            , stderrFile: Just (fixture "config/malformed-configs/from-root.txt")
+            , sanitize: String.trim >>> String.replaceAll (String.Pattern $ Path.toRaw testCwd) (String.Replacement "<test-dir>")
+            }
+
     where
     shouldFailWith result expectedError =
       case result of