Skip to content

Commit

Permalink
Fix the graph check not warning about undeclared cross-package import…
Browse files Browse the repository at this point in the history
…s between local packages (#1198)
  • Loading branch information
f-f authored Mar 11, 2024
1 parent fdaa7e6 commit ebb99a0
Show file tree
Hide file tree
Showing 9 changed files with 73 additions and 5 deletions.
3 changes: 2 additions & 1 deletion src/Spago/Command/Build.purs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ run opts = do
graph <- either die pure eitherGraph
env <- ask
checkResults <- map Array.fold $ for pedanticPkgs \(Tuple selected options) -> do
Graph.toImportErrors selected options <$> runSpago (Record.union { selected } env) (Graph.checkImports graph)
Graph.toImportErrors selected options
<$> runSpago (Record.union { selected, workspacePackages: selectedPackages } env) (Graph.checkImports graph)
unless (Array.null checkResults) do
die $ Graph.formatImportErrors checkResults

Expand Down
3 changes: 2 additions & 1 deletion src/Spago/Command/Publish.purs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ publish _args = do
eitherGraph <- Graph.runGraph globs []
case eitherGraph of
Right graph -> do
graphCheckErrors <- Graph.toImportErrors selected { reportSrc: true, reportTest: false } <$> runSpago (Record.union { selected } env) (Graph.checkImports graph)
graphCheckErrors <- Graph.toImportErrors selected { reportSrc: true, reportTest: false }
<$> runSpago (Record.union { selected, workspacePackages: Config.getWorkspacePackages env.workspace.packageSet } env) (Graph.checkImports graph)
for_ graphCheckErrors (addError <<< Graph.formatImportErrors <<< pure)
Left err ->
die err
Expand Down
7 changes: 4 additions & 3 deletions src/Spago/Purs/Graph.purs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ module Spago.Purs.Graph
import Spago.Prelude

import Data.Array as Array
import Data.Array.NonEmpty as NEA
import Data.Codec.Argonaut.Common as CA
import Data.Codec.Argonaut.Record as CAR
import Data.Map as Map
Expand Down Expand Up @@ -94,6 +93,7 @@ getModuleGraphWithPackage (ModuleGraph graph) = do
allPackages = allDependencies
# Map.union (Map.fromFoldable $ map mkPackageEntry selected)
# Map.union testPackages

pathToPackage :: Map FilePath PackageName <- map (Map.fromFoldable <<< Array.fold)
$ for (Map.toUnfoldable allPackages)
\(Tuple name package) -> do
Expand Down Expand Up @@ -174,15 +174,16 @@ type ImportedPackages = Map PackageName (Map ModuleName (Set ModuleName))

type ImportsGraphEnv a =
{ selected :: WorkspacePackage
, workspacePackages :: NonEmptyArray WorkspacePackage
, dependencies :: Fetch.PackageTransitiveDeps
, logOptions :: LogOptions
| a
}

checkImports :: forall a. ModuleGraph -> Spago (ImportsGraphEnv a) ImportCheckResult
checkImports graph = do
env@{ selected } <- ask
packageGraph <- runSpago (Record.union { selected: NEA.singleton selected } env) $ getModuleGraphWithPackage graph
env@{ selected, workspacePackages } <- ask
packageGraph <- runSpago (Record.union { selected: workspacePackages } env) $ getModuleGraphWithPackage graph

let
dropValues = Map.mapMaybe (const (Just Map.empty))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
Reading Spago workspace configuration...

✅ Selecting 2 packages to build:
pedantic-cross-package-imports
subpackage

Downloading dependencies...
Building...
Src Lib All
Warnings 0 0 0
Errors 0 0 0

✅ Build succeeded.

Looking for unused and undeclared transitive dependencies...

❌ Found unused and/or undeclared transitive dependencies:

Sources for package 'subpackage' import the following transitive dependencies - please add them to the project dependencies, or remove the imports:
pedantic-cross-package-imports
from `Sub.Module`, which imports:
Main

These errors can be fixed by running the below command(s):
spago install -p subpackage pedantic-cross-package-imports
10 changes: 10 additions & 0 deletions test-fixtures/monorepo/pedantic-cross-package-imports/spago.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package:
name: pedantic-cross-package-imports
dependencies:
- console
- effect
- prelude
workspace:
package_set:
registry: 50.3.2
extra_packages: {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module Main where

import Prelude

import Effect (Effect)
import Effect.Console (log)

main :: Effect Unit
main = do
log "🍝"
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package:
name: subpackage
dependencies:
- effect
- prelude
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module Sub.Module where

import Prelude
import Main as M
import Effect (Effect)

main :: Effect Unit
main = M.main
7 changes: 7 additions & 0 deletions test/Spago/Build/Monorepo.purs
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ spec = Spec.describe "monorepo" do
, " from `" <> mkModName package <> "`, which imports:"
, " " <> pkgModName
]

Spec.it "when package config has 'pedantic_packages: true', build fails with expected errors" \{ spago, fixture } -> do
FS.copyTree { src: fixture "monorepo/pedantic-config", dst: "." }

Expand Down Expand Up @@ -243,3 +244,9 @@ spec = Spec.describe "monorepo" do
unless (Array.null unfoundTexts) do
Assert.fail $ "STDERR did not contain expected texts:\n" <> (Array.intercalate "\n\n" unfoundTexts) <> "\n\nStderr was:\n" <> stdErr
spago [ "build", "--pedantic-packages" ] >>= check { stdout: mempty, stderr: hasExpectedErrors, result: isLeft }

Spec.it "prevents cross-package imports between local packages" \{ spago, fixture } -> do
FS.copyTree { src: fixture "monorepo/pedantic-cross-package-imports", dst: "." }

spago [ "build" ] >>= shouldBeSuccess
spago [ "build", "--pedantic-packages" ] >>= shouldBeFailureErr (fixture "monorepo/pedantic-cross-package-imports/expected-stderr.txt")

0 comments on commit ebb99a0

Please sign in to comment.