Migrate pedantic-packages to package config (#1075)

* Migrate pedantic-packages to package config

* Add support for SPAGO_TEST_DEBUG flag to print test's stdout/stderr

* Fix bug in Publish: die if not get graph

* Group single-package Build's pedantic_packages tests

CONTRIBUTING.md

@@ -54,6 +54,9 @@ spago build
 
 # Can of course run the tests with
 spago test
+
+# To see tests' stdout/stderr output while the tests are running, run
+SPAGO_TEST_DEBUG=1 spago
 ```
 
 ## Developing docs

README.md

@@ -1187,9 +1187,6 @@ workspace:
   build_opts:
     # Directory for the compiler products - optional, defaults to `output`.
     output: "output"
-    # Fail the build if `spago.yml` has redundant/underspecified packages.
-    # Optional boolean that defaults to `false`.
-    pedantic_packages: false
     # Specify whether to censor warnings coming from the compiler
     # for files in the `.spago` directory`.
     # Optional and can be one of two possible values
@@ -1242,6 +1239,10 @@ package:
 
   # Optional section to further customise the build for this package.
   build:
+    # Fail the build if this package's `dependencies` field has redundant/underspecified packages.
+    # Optional boolean that defaults to `false`.
+    pedantic_packages: false
+
     # Specify whether to censor warnings coming from the compiler
     # for files from this package.
     # Optional and can be one of two possible values
@@ -1305,6 +1306,11 @@ package:
     execArgs:
       - "--cli-arg"
       - "foo"
+
+    # Fail the build if this package's test's `dependencies` field has redundant/underspecified packages.
+    # Optional boolean that defaults to `false`.
+    pedantic_packages: false
+
     # Specify whether to censor warnings coming from the compiler
     # for files from this package's test code.
     # Optional and can be one of two possible values

bin/src/Main.purs

@@ -656,7 +656,6 @@ mkBundleEnv bundleArgs = do
     newWorkspace = workspace
       { buildOptions
           { output = bundleArgs.output <|> workspace.buildOptions.output
-          , pedanticPackages = bundleArgs.pedanticPackages || workspace.buildOptions.pedanticPackages
           }
       }
   esbuild <- Esbuild.getEsbuild
@@ -767,7 +766,6 @@ mkBuildEnv buildArgs dependencies = do
     newWorkspace = workspace
       { buildOptions
           { output = buildArgs.output <|> workspace.buildOptions.output
-          , pedanticPackages = buildArgs.pedanticPackages || workspace.buildOptions.pedanticPackages
           }
       -- Override the backend args from the config if they are passed in through a flag
       , backend = map
@@ -788,6 +786,7 @@ mkBuildEnv buildArgs dependencies = do
         { statVerbosity: buildArgs.statVerbosity
         , strict: buildArgs.strict
         }
+    , pedanticPackages: buildArgs.pedanticPackages
     }
 
 mkPublishEnv :: forall a. Fetch.PackageTransitiveDeps -> Spago (Fetch.FetchEnv a) (Publish.PublishEnv a)

core/src/Config.purs

@@ -126,6 +126,7 @@ type TestConfig =
   , dependencies :: Dependencies
   , censor_test_warnings :: Maybe CensorBuildWarnings
   , strict :: Maybe Boolean
+  , pedantic_packages :: Maybe Boolean
   }
 
 testConfigCodec :: JsonCodec TestConfig
@@ -135,6 +136,7 @@ testConfigCodec = CAR.object "TestConfig"
   , dependencies: dependenciesCodec
   , censor_test_warnings: CAR.optional censorBuildWarningsCodec
   , strict: CAR.optional CA.boolean
+  , pedantic_packages: CAR.optional CA.boolean
   }
 
 type BackendConfig =
@@ -151,12 +153,14 @@ backendConfigCodec = CAR.object "BackendConfig"
 type PackageBuildOptionsInput =
   { censor_project_warnings :: Maybe CensorBuildWarnings
   , strict :: Maybe Boolean
+  , pedantic_packages :: Maybe Boolean
   }
 
 packageBuildOptionsCodec :: JsonCodec PackageBuildOptionsInput
 packageBuildOptionsCodec = CAR.object "PackageBuildOptionsInput"
   { censor_project_warnings: CAR.optional censorBuildWarningsCodec
   , strict: CAR.optional CA.boolean
+  , pedantic_packages: CAR.optional CA.boolean
   }
 
 type BundleConfig =
@@ -298,15 +302,13 @@ workspaceConfigCodec = CAR.object "WorkspaceConfig"
 
 type WorkspaceBuildOptionsInput =
   { output :: Maybe FilePath
-  , pedantic_packages :: Maybe Boolean
   , censor_library_warnings :: Maybe CensorBuildWarnings
   , stat_verbosity :: Maybe StatVerbosity
   }
 
 buildOptionsCodec :: JsonCodec WorkspaceBuildOptionsInput
 buildOptionsCodec = CAR.object "WorkspaceBuildOptionsInput"
   { output: CAR.optional CA.string
-  , pedantic_packages: CAR.optional CA.boolean
   , censor_library_warnings: CAR.optional censorBuildWarningsCodec
   , stat_verbosity: CAR.optional statVerbosityCodec
   }

src/Spago/Command/Build.purs

@@ -6,7 +6,9 @@ module Spago.Command.Build
 
 import Spago.Prelude
 
+import Control.Monad.ST as ST
 import Data.Array as Array
+import Data.Array.ST as STA
 import Data.Map as Map
 import Data.Set as Set
 import Data.Traversable (sequence)
@@ -30,6 +32,7 @@ type BuildEnv a =
   , logOptions :: LogOptions
   , workspace :: Workspace
   , psaCliFlags :: PsaTypes.PsaOutputOptions
+  , pedanticPackages :: Boolean
   | a
   }
 
@@ -46,6 +49,7 @@ run opts = do
   , workspace
   , logOptions
   , psaCliFlags
+  , pedanticPackages
   } <- ask
 
   BuildInfo.writeBuildInfo
@@ -135,23 +139,27 @@ run opts = do
         Right _r ->
           logSuccess "Backend build succeeded."
 
-  when workspace.buildOptions.pedanticPackages do
+  let
+    pedanticPkgs = ST.run do
+      arr <- STA.new
+      ST.foreach selectedPackages \p -> do
+        let reportSrc = pedanticPackages || (fromMaybe false $ p.package.build >>= _.pedantic_packages)
+        let reportTest = pedanticPackages || (fromMaybe false $ p.package.test >>= _.pedantic_packages)
+        when (reportSrc || reportTest) do
+          void $ STA.push (Tuple p { reportSrc, reportTest }) arr
+      STA.unsafeFreeze arr
+  unless (Array.null pedanticPkgs) do
     logInfo $ "Looking for unused and undeclared transitive dependencies..."
-    maybeGraph <- Graph.runGraph globs opts.pursArgs
-    for_ maybeGraph \graph -> do
-      env <- ask
-      case workspace.selected of
-        Just selected -> do
-          errors <- Graph.toImportErrors selected <$> runSpago (Record.union { graph, selected } env) Graph.checkImports
-          unless (Array.null errors) do
-            die' errors
-        Nothing -> do
-          -- TODO: here we could go through all the workspace packages and run the check for each
-          -- The complication is that "dependencies" includes all the dependencies for all packages
-          errors <- map Array.fold $ for (Config.getWorkspacePackages workspace.packageSet) \selected -> do
-            Graph.toImportErrors selected <$> runSpago (Record.union { graph, selected } env) Graph.checkImports
-          unless (Array.null errors) do
-            die' errors
+    eitherGraph <- Graph.runGraph globs opts.pursArgs
+    graph <- either die pure eitherGraph
+    env <- ask
+    -- TODO: when the entire workspace is built, 
+    -- here we could go through all the workspace packages and run the check for each
+    -- The complication is that "dependencies" includes all the dependencies for all packages
+    errors <- map Array.fold $ for pedanticPkgs \(Tuple selected options) -> do
+      Graph.toImportErrors selected options <$> runSpago (Record.union { graph, selected } env) Graph.checkImports
+    unless (Array.null errors) do
+      die' errors
 
 -- TODO: if we are building with all the packages (i.e. selected = Nothing),
 -- then we can use the graph to remove outdated modules from `output`!

src/Spago/Command/Init.purs

@@ -100,7 +100,7 @@ defaultConfig { name, withWorkspace, testModuleName } = do
     pkg =
       { name
       , dependencies: [ "effect", "console", "prelude" ]
-      , test: Just { moduleMain: testModuleName, strict: Nothing, censorTestWarnings: Nothing }
+      , test: Just { moduleMain: testModuleName, strict: Nothing, censorTestWarnings: Nothing, pedanticPackages: Nothing, dependencies: Nothing }
       , build: Nothing
       }
   defaultConfig' case withWorkspace of
@@ -110,8 +110,20 @@ defaultConfig { name, withWorkspace, testModuleName } = do
 type DefaultConfigPackageOptions =
   { name :: PackageName
   , dependencies :: Array String
-  , test :: Maybe { moduleMain :: String, strict :: Maybe Boolean, censorTestWarnings :: Maybe Config.CensorBuildWarnings }
-  , build :: Maybe { strict :: Maybe Boolean, censorProjectWarnings :: Maybe Config.CensorBuildWarnings }
+  , test ::
+      Maybe
+        { moduleMain :: String
+        , strict :: Maybe Boolean
+        , censorTestWarnings :: Maybe Config.CensorBuildWarnings
+        , pedanticPackages :: Maybe Boolean
+        , dependencies :: Maybe Config.Dependencies
+        }
+  , build ::
+      Maybe
+        { strict :: Maybe Boolean
+        , censorProjectWarnings :: Maybe Config.CensorBuildWarnings
+        , pedanticPackages :: Maybe Boolean
+        }
   }
 
 type DefaultConfigWorkspaceOptions =
@@ -141,17 +153,19 @@ defaultConfig' opts =
       { name
       , dependencies: Dependencies $ Map.fromFoldable $ map mkDep dependencies
       , description: Nothing
-      , build: build <#> \{ censorProjectWarnings, strict } ->
+      , build: build <#> \{ censorProjectWarnings, strict, pedanticPackages } ->
           { censor_project_warnings: censorProjectWarnings
           , strict
+          , pedantic_packages: pedanticPackages
           }
       , run: Nothing
-      , test: test <#> \{ moduleMain, censorTestWarnings, strict } ->
-          { dependencies: Dependencies Map.empty
+      , test: test <#> \{ moduleMain, censorTestWarnings, strict, pedanticPackages, dependencies: testDeps } ->
+          { dependencies: fromMaybe (Dependencies Map.empty) testDeps
           , execArgs: Nothing
           , main: moduleMain
           , censor_test_warnings: censorTestWarnings
           , strict
+          , pedantic_packages: pedanticPackages
           }
       , publish: Nothing
       , bundle: Nothing

src/Spago/Command/Publish.purs

@@ -114,6 +114,7 @@ publish _args = do
         { statVerbosity: (Nothing :: Maybe Core.StatVerbosity)
         , strict: (Nothing :: Maybe Boolean)
         }
+    , pedanticPackages: false
     }
     ( Build.run
         { depsOnly: false
@@ -125,10 +126,13 @@ publish _args = do
   -- We then need to check that the dependency graph is accurate. If not, queue the errors
   let allDependencies = Fetch.toAllDependencies dependencies
   let globs = Build.getBuildGlobs { selected: [ selected ], withTests: false, dependencies: allDependencies, depsOnly: false }
-  maybeGraph <- Graph.runGraph globs []
-  for_ maybeGraph \graph -> do
-    graphCheckErrors <- Graph.toImportErrors selected <$> runSpago (Record.union { graph, selected } env) Graph.checkImports
-    for_ graphCheckErrors addError
+  eitherGraph <- Graph.runGraph globs []
+  case eitherGraph of
+    Right graph -> do
+      graphCheckErrors <- Graph.toImportErrors selected { reportSrc: true, reportTest: false } <$> runSpago (Record.union { graph, selected } env) Graph.checkImports
+      for_ graphCheckErrors addError
+    Left err ->
+      die err
 
   -- Check if all the packages have ranges, error if not
   let
@@ -309,6 +313,7 @@ publish _args = do
             { statVerbosity: (Nothing :: Maybe Core.StatVerbosity)
             , strict: (Nothing :: Maybe Boolean)
             }
+        , pedanticPackages: false
         }
         ( Build.run
             { depsOnly: false

src/Spago/Config.purs

@@ -70,7 +70,6 @@ type Workspace =
 
 type BuildOptions =
   { output :: Maybe FilePath
-  , pedanticPackages :: Boolean
   , censorLibWarnings :: Maybe Core.CensorBuildWarnings
   , statVerbosity :: Maybe Core.StatVerbosity
   }
@@ -381,7 +380,6 @@ readWorkspace maybeSelectedPackage = do
     buildOptions :: BuildOptions
     buildOptions =
       { output: _.output =<< workspace.build_opts
-      , pedanticPackages: fromMaybe false (_.pedantic_packages =<< workspace.build_opts)
       , censorLibWarnings: _.censor_library_warnings =<< workspace.build_opts
       , statVerbosity: _.stat_verbosity =<< workspace.build_opts
       }

src/Spago/Purs/Graph.purs

@@ -162,12 +162,19 @@ checkImports = do
 
   pure { unused, transitive, unusedTest, transitiveTest }
 
-toImportErrors :: WorkspacePackage -> ImportCheckResult -> Array Docc
-toImportErrors selected { unused, unusedTest, transitive, transitiveTest } =
-  (if Set.isEmpty unused then [] else [ unusedError false selected unused ])
-    <> (if Map.isEmpty transitive then [] else [ transitiveError false selected transitive ])
-    <> (if Set.isEmpty unusedTest then [] else [ unusedError true selected unusedTest ])
-    <> (if Map.isEmpty transitiveTest then [] else [ transitiveError true selected transitiveTest ])
+toImportErrors
+  :: WorkspacePackage
+  -> { reportSrc :: Boolean
+     , reportTest :: Boolean
+     }
+  -> ImportCheckResult
+  -> Array Docc
+toImportErrors selected opts { unused, unusedTest, transitive, transitiveTest } = join
+  [ if opts.reportSrc && (not $ Set.isEmpty unused) then [ unusedError false selected unused ] else []
+  , if opts.reportSrc && (not $ Map.isEmpty transitive) then [ transitiveError false selected transitive ] else []
+  , if opts.reportTest && (not $ Set.isEmpty unusedTest) then [ unusedError true selected unusedTest ] else []
+  , if opts.reportTest && (not $ Map.isEmpty transitiveTest) then [ transitiveError true selected transitiveTest ] else []
+  ]
 
 compileGlob :: forall a. FilePath -> Spago (LogEnv a) (Array FilePath)
 compileGlob sourcePath = do
@@ -176,15 +183,10 @@ compileGlob sourcePath = do
     logDebug [ toDoc "Encountered some globs that are not in cwd, proceeding anyways:", indent $ toDoc failed ]
   pure (succeeded <> failed)
 
-runGraph :: forall a. Set FilePath -> Array String -> Spago (PreGraphEnv a) (Maybe Purs.ModuleGraph)
-runGraph globs pursArgs = do
-  maybeGraph <- Purs.graph globs pursArgs
-  case maybeGraph of
-    Left err -> do
-      logWarn $ "Could not decode the output of `purs graph`, error: " <> CA.printJsonDecodeError err
-      pure Nothing
-    Right graph ->
-      pure $ Just graph
+runGraph :: forall a. Set FilePath -> Array String -> Spago (PreGraphEnv a) (Either String Purs.ModuleGraph)
+runGraph globs pursArgs = map (lmap toErrorMessage) $ Purs.graph globs pursArgs
+  where
+  toErrorMessage = append "Could not decode the output of `purs graph`, error: " <<< CA.printJsonDecodeError
 
 unusedError :: Boolean -> WorkspacePackage -> Set PackageName -> Docc
 unusedError isTest selected unused = toDoc

test-fixtures/check-unused-test-dependency.txt

@@ -0,0 +1,17 @@
+Reading Spago workspace configuration...
+Read the package set from the registry
+
+✅ Selecting package to build: 7368613235362d2f444a2b4f56375435646a59726b53586548
+
+Downloading dependencies...
+Building...
+           Src   Lib   All
+Warnings     0     0     0
+Errors       0     0     0
+
+✅ Build succeeded.
+
+Looking for unused and undeclared transitive dependencies...
+
+❌ Tests for package '7368613235362d2f444a2b4f56375435646a59726b53586548' declares unused dependencies - please remove them from the project config:
+  - newtype

test/Prelude.purs

@@ -106,7 +106,9 @@ check checkers execResult = do
       Left err -> err.stderr
       Right res -> res.stderr
 
-  when false do
+  printStdoutStderr <- liftEffect $ map isJust $ Process.lookupEnv "SPAGO_TEST_DEBUG"
+
+  when printStdoutStderr do
     log $ "STDOUT:\n" <> prettyPrint stdout
     log $ "STDERR:\n" <> prettyPrint stderr
   execResult `Assert.shouldSatisfy` checkers.result