From ebb99a02dacd119e8b650855bfae308798c46c07 Mon Sep 17 00:00:00 2001 From: Fabrizio Ferrai Date: Mon, 11 Mar 2024 21:38:36 +0200 Subject: [PATCH] Fix the graph check not warning about undeclared cross-package imports between local packages (#1198) --- src/Spago/Command/Build.purs | 3 ++- src/Spago/Command/Publish.purs | 3 ++- src/Spago/Purs/Graph.purs | 7 +++--- .../expected-stderr.txt | 25 +++++++++++++++++++ .../pedantic-cross-package-imports/spago.yaml | 10 ++++++++ .../src/Main.purs | 10 ++++++++ .../sub/spago.yaml | 5 ++++ .../sub/src/Main.purs | 8 ++++++ test/Spago/Build/Monorepo.purs | 7 ++++++ 9 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 test-fixtures/monorepo/pedantic-cross-package-imports/expected-stderr.txt create mode 100644 test-fixtures/monorepo/pedantic-cross-package-imports/spago.yaml create mode 100644 test-fixtures/monorepo/pedantic-cross-package-imports/src/Main.purs create mode 100644 test-fixtures/monorepo/pedantic-cross-package-imports/sub/spago.yaml create mode 100644 test-fixtures/monorepo/pedantic-cross-package-imports/sub/src/Main.purs diff --git a/src/Spago/Command/Build.purs b/src/Spago/Command/Build.purs index ac7ec4864..1d5347dd9 100644 --- a/src/Spago/Command/Build.purs +++ b/src/Spago/Command/Build.purs @@ -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 diff --git a/src/Spago/Command/Publish.purs b/src/Spago/Command/Publish.purs index 15ab7a09d..5900a3dd1 100644 --- a/src/Spago/Command/Publish.purs +++ b/src/Spago/Command/Publish.purs @@ -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 diff --git a/src/Spago/Purs/Graph.purs b/src/Spago/Purs/Graph.purs index 47568a455..c391d554a 100644 --- a/src/Spago/Purs/Graph.purs +++ b/src/Spago/Purs/Graph.purs @@ -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 @@ -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 @@ -174,6 +174,7 @@ type ImportedPackages = Map PackageName (Map ModuleName (Set ModuleName)) type ImportsGraphEnv a = { selected :: WorkspacePackage + , workspacePackages :: NonEmptyArray WorkspacePackage , dependencies :: Fetch.PackageTransitiveDeps , logOptions :: LogOptions | a @@ -181,8 +182,8 @@ type ImportsGraphEnv 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)) diff --git a/test-fixtures/monorepo/pedantic-cross-package-imports/expected-stderr.txt b/test-fixtures/monorepo/pedantic-cross-package-imports/expected-stderr.txt new file mode 100644 index 000000000..44f77ff50 --- /dev/null +++ b/test-fixtures/monorepo/pedantic-cross-package-imports/expected-stderr.txt @@ -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 diff --git a/test-fixtures/monorepo/pedantic-cross-package-imports/spago.yaml b/test-fixtures/monorepo/pedantic-cross-package-imports/spago.yaml new file mode 100644 index 000000000..58487a4e2 --- /dev/null +++ b/test-fixtures/monorepo/pedantic-cross-package-imports/spago.yaml @@ -0,0 +1,10 @@ +package: + name: pedantic-cross-package-imports + dependencies: + - console + - effect + - prelude +workspace: + package_set: + registry: 50.3.2 + extra_packages: {} diff --git a/test-fixtures/monorepo/pedantic-cross-package-imports/src/Main.purs b/test-fixtures/monorepo/pedantic-cross-package-imports/src/Main.purs new file mode 100644 index 000000000..5c18dca55 --- /dev/null +++ b/test-fixtures/monorepo/pedantic-cross-package-imports/src/Main.purs @@ -0,0 +1,10 @@ +module Main where + +import Prelude + +import Effect (Effect) +import Effect.Console (log) + +main :: Effect Unit +main = do + log "🍝" diff --git a/test-fixtures/monorepo/pedantic-cross-package-imports/sub/spago.yaml b/test-fixtures/monorepo/pedantic-cross-package-imports/sub/spago.yaml new file mode 100644 index 000000000..4fe441c79 --- /dev/null +++ b/test-fixtures/monorepo/pedantic-cross-package-imports/sub/spago.yaml @@ -0,0 +1,5 @@ +package: + name: subpackage + dependencies: + - effect + - prelude diff --git a/test-fixtures/monorepo/pedantic-cross-package-imports/sub/src/Main.purs b/test-fixtures/monorepo/pedantic-cross-package-imports/sub/src/Main.purs new file mode 100644 index 000000000..360d473c0 --- /dev/null +++ b/test-fixtures/monorepo/pedantic-cross-package-imports/sub/src/Main.purs @@ -0,0 +1,8 @@ +module Sub.Module where + +import Prelude +import Main as M +import Effect (Effect) + +main :: Effect Unit +main = M.main diff --git a/test/Spago/Build/Monorepo.purs b/test/Spago/Build/Monorepo.purs index 1309bcd28..ba06b7de9 100644 --- a/test/Spago/Build/Monorepo.purs +++ b/test/Spago/Build/Monorepo.purs @@ -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: "." } @@ -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")