Properly ignore nested workspaces - we were missing the case with a n…

…o-package-root-config (#1193)

core/src/FS.purs

@@ -5,11 +5,11 @@ module Spago.FS
   , getInBetweenPaths
   , isLink
   , ls
-  , cp
   , mkdirp
   , moveSync
   , copyFileSync
   , copyFile
+  , copyTree
   , readJsonFile
   , readTextFile
   , readYamlDocFile
@@ -53,8 +53,12 @@ copyFile { src, dst } = liftAff $ FS.Aff.copyFile src dst
 
 foreign import cpImpl :: String -> String -> Effect Unit
 
-cp :: forall m. MonadEffect m => { src :: FilePath, dst :: FilePath } -> m Unit
-cp { src, dst } = liftEffect $ cpImpl src dst
+-- | Copy a file or directory. The directory can have contents.
+-- | Note: if `src` is a directory it will copy everything inside of this directory,
+-- | not the entire directory itself.
+-- | Note: if `src` is a file, `dst` cannot be a directory
+copyTree :: forall m. MonadEffect m => { src :: FilePath, dst :: FilePath } -> m Unit
+copyTree { src, dst } = liftEffect $ cpImpl src dst
 
 foreign import ensureFileSyncImpl :: String -> Effect Unit
 

src/Spago/Command/Fetch.purs

@@ -374,7 +374,7 @@ getGitPackageInLocalCache name package = do
             let commitHashLocation = Config.getPackageLocation name (GitPackage $ package { ref = ref })
             logDebug $ "Copying the repo also to " <> commitHashLocation
             FS.mkdirp $ Path.concat [ Paths.localCachePackagesPath, PackageName.print name ]
-            FS.cp { src: localPackageLocation, dst: commitHashLocation }
+            FS.copyTree { src: localPackageLocation, dst: commitHashLocation }
 
 getPackageDependencies :: forall a. PackageName -> Package -> Spago (FetchEnv a) (Maybe (Map PackageName Range))
 getPackageDependencies packageName package = case package of

src/Spago/Config.purs

@@ -46,7 +46,6 @@ import Dodo as Log
 import Effect.Uncurried (EffectFn2, EffectFn3, runEffectFn2, runEffectFn3)
 import Foreign.Object as Foreign
 import Node.Path as Path
-import Record as Record
 import Registry.Foreign.FastGlob as Glob
 import Registry.Internal.Codec as Internal.Codec
 import Registry.PackageName as PackageName
@@ -60,7 +59,6 @@ import Spago.Lock (Lockfile, PackageSetInfo)
 import Spago.Lock as Lock
 import Spago.Paths as Paths
 import Spago.Registry as Registry
-import Type.Proxy (Proxy(..))
 
 type Workspace =
   { selected :: Maybe WorkspacePackage
@@ -146,6 +144,14 @@ data Package
   | LocalPackage Core.LocalPackage
   | WorkspacePackage WorkspacePackage
 
+type ConfigReadResult =
+  { package :: Maybe Core.PackageConfig
+  , configWorkspace :: Maybe Core.WorkspaceConfig
+  , doc :: YamlDoc Core.Config
+  , hasTests :: Boolean
+  , path :: FilePath
+  }
+
 -- | Reads all the configurations in the tree and builds up the Map of local
 -- | packages to be integrated in the package set
 readWorkspace :: { maybeSelectedPackage :: Maybe PackageName, pureBuild :: Boolean } -> Spago (Registry.RegistryEnv _) Workspace
@@ -188,6 +194,7 @@ readWorkspace { maybeSelectedPackage, pureBuild } = do
 
   -- We read all of them in, and only read the package section, if any.
   let
+    readWorkspaceConfig :: FilePath -> Spago (Registry.RegistryEnv _) (Either Docc ConfigReadResult)
     readWorkspaceConfig path = do
       maybeConfig <- Core.readConfig path
       -- We try to figure out if this package has tests - look for test sources
@@ -198,27 +205,29 @@ readWorkspace { maybeSelectedPackage, pureBuild } = do
           , toDoc "Error was: "
           , indent $ toDoc eLines
           ]
-        Right { yaml: { package: Nothing } } -> Left $ toDoc $ "No package found for config at path: " <> path
-        Right { yaml: { package: Just package, workspace: configWorkspace }, doc } -> do
-          -- We store the path of the package, so we can treat it basically as a LocalPackage
-          Right $ Tuple package.name { path: Path.dirname path, package, configWorkspace, doc, hasTests }
-  { right: otherPackages, left: failedPackages } <- partitionMap identity <$> traverse readWorkspaceConfig otherConfigPaths
+        Right { yaml: { package, workspace: configWorkspace }, doc } -> do
+          Right { package, configWorkspace, doc, hasTests, path: Path.dirname path }
 
+  { right: otherPackages, left: failedPackages } <- partitionMap identity <$> traverse readWorkspaceConfig otherConfigPaths
   unless (Array.null failedPackages) do
     logWarn $ [ toDoc "Failed to read some configs:" ] <> failedPackages
 
   -- We prune any configs that use a different workspace.
   -- For reasoning, see https://github.com/purescript/spago/issues/951
+  let configPathsWithWorkspaces = otherPackages # Array.mapMaybe \readResult -> readResult.path <$ readResult.configWorkspace
+  unless (Array.null configPathsWithWorkspaces) do
+    logDebug $ "Found these paths with workspaces: " <> show configPathsWithWorkspaces
   let
-    configPathsWithWorkspaces = Array.mapMaybe (\(Tuple _ configParts) -> configParts.path <$ configParts.configWorkspace) otherPackages
-    { right: configsNoWorkspaces, left: prunedConfigs } = otherPackages # partitionMap \config -> do
-      let configPath = (snd config).path
-      if Array.any (\p -> isJust $ String.stripPrefix (Pattern p) configPath) configPathsWithWorkspaces then
-        Left configPath
+    { right: configsNoWorkspaces, left: prunedConfigs } = otherPackages # partitionMap \readResult@{ doc, path, hasTests } -> do
+      if Array.any (\p -> isJust $ String.stripPrefix (Pattern p) path) configPathsWithWorkspaces then
+        Left path
       else
-        Right $ config <#> Record.delete (Proxy :: _ "configWorkspace")
+        case readResult.package of
+          Nothing -> Left path
+          Just package ->
+            -- We store the path of the package, so we can treat it basically as a LocalPackage
+            Right (Tuple package.name { package, doc, path, hasTests })
 
-  -- TODO: this should be a logwarning, and there should be a test
   unless (Array.null prunedConfigs) do
     logDebug $ [ "Excluding configs that use a different workspace (directly or implicitly via parent directory's config):" ] <> Array.sort prunedConfigs
 

test-fixtures/monorepo/ignore-nested-workspaces/expected-stderr.txt

@@ -0,0 +1,11 @@
+Reading Spago workspace configuration...
+
+✅ Selecting package to build: ignore-nested-workspaces
+
+Downloading dependencies...
+Building...
+           Src   Lib   All
+Warnings     0     0     0
+Errors       0     0     0
+
+✅ Build succeeded.

test-fixtures/monorepo/ignore-nested-workspaces/nested/lib/spago.yaml

@@ -0,0 +1,9 @@
+package:
+  name: lib
+  dependencies:
+    - console
+    - effect
+    - prelude
+  test:
+    main: Test.Main
+    dependencies: []

test-fixtures/monorepo/ignore-nested-workspaces/nested/spago.yaml

@@ -0,0 +1 @@
+workspace: {}

test-fixtures/monorepo/ignore-nested-workspaces/spago.yaml

@@ -0,0 +1,13 @@
+package:
+  name: ignore-nested-workspaces
+  dependencies:
+    - console
+    - effect
+    - prelude
+  test:
+    main: Test.Main
+    dependencies: []
+workspace:
+  package_set:
+    registry: 50.3.2
+  extra_packages: {}

test-fixtures/monorepo/ignore-nested-workspaces/src/Main.purs

@@ -0,0 +1,11 @@
+module Main where
+
+import Prelude
+
+import Effect (Effect)
+import Effect.Console (log)
+
+main :: Effect Unit
+main = do
+  log "🍝"
+

test/Spago/Build/Polyrepo.purs

@@ -184,6 +184,11 @@ spec = Spec.describe "polyrepo" do
     spago [ "uninstall", "-p", "root", "console", "effect", "prelude" ] >>= shouldBeSuccess
     spago [ "build", "-p", "root", "--pedantic-packages" ] >>= shouldBeSuccess
 
+  Spec.it "ignore nested workspaces" \{ spago, fixture } -> do
+    FS.copyTree { src: fixture "monorepo/ignore-nested-workspaces", dst: "." }
+    spago [ "build" ] >>= shouldBeSuccess
+    spago [ "build" ] >>= shouldBeSuccessErr (fixture "monorepo/ignore-nested-workspaces/expected-stderr.txt")
+
   Spec.describe "warning censoring and error-promotion" do
     let
       setupPackageWithUnusedNameWarning packageName deps strict censorShadowedName includeTestCode = do