diff --git a/Changelog.md b/Changelog.md index 3f18ee1b6..cbca24ed9 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,13 @@ # FOSSA CLI Changelog +## 3.16.5 + +- PNPM: Fix pnpm v9 lockfile transitive devDependency classification. Dependencies of devDependencies were incorrectly reported as production dependencies in pnpm v9 projects. ([#1668](https://github.com/fossas/fossa-cli/pull/1668)) + +## 3.16.4 + +- Bugfix: revert caching changes as they caused a problem with missing libs on macos ([#1675](https://github.com/fossas/fossa-cli/pull/1675)) + ## 3.16.3 - Elixir: Use `MIX_ENV=prod` for accurate production dependency resolution, with fallback to `--only prod` for projects lacking `config/prod.exs` ([#1662](https://github.com/fossas/fossa-cli/pull/1662)) diff --git a/src/Strategy/Node/Pnpm/PnpmLock.hs b/src/Strategy/Node/Pnpm/PnpmLock.hs index ebde629ad..0d43fd184 100644 --- a/src/Strategy/Node/Pnpm/PnpmLock.hs +++ b/src/Strategy/Node/Pnpm/PnpmLock.hs @@ -10,6 +10,7 @@ where import Control.Applicative ((<|>)) import Control.Effect.Diagnostics (Diagnostics, Has, context) +import Control.Monad (when) import Data.Aeson (FromJSON (..), withObject) import Data.Aeson.Extra (TextLike (..)) import Data.Aeson.KeyMap (toHashMapText) @@ -30,17 +31,23 @@ import DepTypes ( DepType (GitType, NodeJSType, URLType, UserType), Dependency (Dependency, dependencyType), VerConstraint (CEq), + hydrateDepEnvs, + insertEnvironment, ) -import Effect.Grapher (deep, direct, edge, evalGrapher, run) +import Effect.Grapher (deep, direct, edge, label, run, withLabeling) import Effect.Logger ( Logger, logWarn, pretty, ) import Effect.ReadFS (ReadFS, readContentsYaml) -import Graphing (Graphing, shrink) +import Graphing (Graphing) +import Graphing qualified import Path (Abs, File, Path) +newtype PnpmLabel = PnpmEnv DepEnvironment + deriving (Eq, Ord) + -- | Pnpm Lockfile -- -- Pnpm lockfile (v5) (in yaml) has the following shape (irrelevant fields omitted): @@ -300,16 +307,22 @@ analyze file = context "Analyzing Npm Lockfile (v3)" $ do context "Building dependency graph" $ pure $ buildGraph pnpmLockFile +-- Build the dependency graph, labeling direct deps with their environment +-- (prod/dev). hydrateDepEnvs then propagates those environments to all +-- transitive successors. buildGraph :: PnpmLockfile -> Graphing Dependency -buildGraph lockFile = withoutLocalPackages $ - run . evalGrapher $ do +buildGraph lockFile = withoutLocalPackages . hydrateDepEnvs $ + run . withLabeling applyLabels $ do for_ (toList lockFile.importers) $ \(_, projectImporters) -> do - let allDirectDependencies = - toList (directDependencies projectImporters) - <> toList (directDevDependencies projectImporters) + for_ (Map.toList $ directDependencies projectImporters) $ \(depName, ProjectMapDepMetadata depVersion) -> + for_ (toResolvedDependency depName depVersion) $ \dep -> do + direct dep + when isV9 $ label dep (PnpmEnv EnvProduction) - for_ allDirectDependencies $ \(depName, (ProjectMapDepMetadata depVersion)) -> - maybe (pure ()) direct $ toResolvedDependency (isPnpm9Dev depName) depName depVersion + for_ (Map.toList $ directDevDependencies projectImporters) $ \(depName, ProjectMapDepMetadata depVersion) -> + for_ (toResolvedDependency depName depVersion) $ \dep -> do + direct dep + when isV9 $ label dep (PnpmEnv EnvDevelopment) -- Add edges and deep dependencies by iterating over all packages. -- @@ -338,14 +351,14 @@ buildGraph lockFile = withoutLocalPackages $ let (depName, depVersion) = case getPkgNameVersion pkgKey of Nothing -> (pkgKey, Nothing) Just (name, version) -> (name, Just version) - let parentDep = toDependency (isPnpm9Dev depName) depName depVersion pkgMeta + let parentDep = toDependency depName depVersion pkgMeta -- It is ok, if this dependency was already graphed as direct -- @direct 1 <> deep 1 = direct 1@ deep parentDep for_ deepDependencies $ \(deepName, deepVersion) -> do - maybe (pure ()) (edge parentDep) (toResolvedDependency (isPnpm9Dev deepName) deepName deepVersion) + maybe (pure ()) (edge parentDep) (toResolvedDependency deepName deepVersion) where getPkgNameVersion :: Text -> Maybe (Text, Text) getPkgNameVersion = case lockFileVersion lockFile of @@ -418,8 +431,8 @@ buildGraph lockFile = withoutLocalPackages $ -- e.g. -- file:../local-package -- - toResolvedDependency :: Bool -> Text -> Text -> Maybe Dependency - toResolvedDependency devOverride depName depVersion = do + toResolvedDependency :: Text -> Text -> Maybe Dependency + toResolvedDependency depName depVersion = do -- Some versions of the lockfile remove the peer dep suffix. -- Others do not which is why it tries both. let strippedVersion = withoutPeerDepSuffix depVersion @@ -431,25 +444,8 @@ buildGraph lockFile = withoutLocalPackages $ <|> fmap (depVersion,) (Map.lookup (mkPkgKey depName depVersion) (packages lockFile)) case (maybeNonRegistrySrcPackage, maybeRegistrySrcPackage) of (Nothing, Nothing) -> Nothing - (Just nonRegistryPkg, _) -> Just $ toDependency devOverride depName Nothing nonRegistryPkg - (Nothing, Just (version, registryPkg)) -> Just $ toDependency devOverride depName (Just version) registryPkg - - -- PNPM lockfile 9 doesn't store information about dev deps directly on package data. - -- Use a project map to determine if a package should be marked dev. - isPnpm9ProjectDev :: Text -> ProjectMap -> Bool - isPnpm9ProjectDev name ProjectMap{directDevDependencies} = Map.member name directDevDependencies - - isPnpm9ProjectDep :: Text -> ProjectMap -> Bool - isPnpm9ProjectDep name ProjectMap{directDependencies} = Map.member name directDependencies - - -- Returns true if a dependency is either absent or a dev dependency in each workspace. If it is a - -- non-dev dependency in any workspace, then this function returns false. Also returns false if the - -- lockfile is not PNPM v9 - isPnpm9Dev :: Text -> Bool - isPnpm9Dev depName = - (lockFile.lockFileVersion == PnpmLockV9) - && (any (isPnpm9ProjectDev depName) lockFile.importers) - && not (any (isPnpm9ProjectDep depName) lockFile.importers) + (Just nonRegistryPkg, _) -> Just $ toDependency depName Nothing nonRegistryPkg + (Nothing, Just (version, registryPkg)) -> Just $ toDependency depName (Just version) registryPkg -- Makes representative key if the package was -- resolved via registry resolver. @@ -466,17 +462,17 @@ buildGraph lockFile = withoutLocalPackages $ PnpmLockV678 _ -> "/" <> name <> "@" <> version PnpmLockV9 -> name <> "@" <> version - toDependency :: Bool -> Text -> Maybe Text -> PackageData -> Dependency - toDependency devOverride name maybeVersion (PackageData isDev _ (RegistryResolve _) _ _) = - toDep NodeJSType name (withoutPeerDepSuffix . withoutSymConstraint <$> maybeVersion) (isDev || devOverride) - toDependency devOverride _ _ (PackageData isDev _ (GitResolve (GitResolution url rev)) _ _) = - toDep GitType url (Just rev) (isDev || devOverride) - toDependency devOverride _ _ (PackageData isDev _ (TarballResolve (TarballResolution url)) _ _) = - toDep URLType url Nothing (isDev || devOverride) - toDependency devOverride _ _ (PackageData isDev (Just name) (DirectoryResolve _) _ _) = - toDep UserType name Nothing (isDev || devOverride) - toDependency devOverride name _ (PackageData isDev Nothing (DirectoryResolve _) _ _) = - toDep UserType name Nothing (isDev || devOverride) + toDependency :: Text -> Maybe Text -> PackageData -> Dependency + toDependency name maybeVersion (PackageData isDev _ (RegistryResolve _) _ _) = + toDep NodeJSType name (withoutPeerDepSuffix . withoutSymConstraint <$> maybeVersion) isDev + toDependency _ _ (PackageData isDev _ (GitResolve (GitResolution url rev)) _ _) = + toDep GitType url (Just rev) isDev + toDependency _ _ (PackageData isDev _ (TarballResolve (TarballResolution url)) _ _) = + toDep URLType url Nothing isDev + toDependency _ _ (PackageData isDev (Just name) (DirectoryResolve _) _ _) = + toDep UserType name Nothing isDev + toDependency name _ (PackageData isDev Nothing (DirectoryResolve _) _ _) = + toDep UserType name Nothing isDev -- Sometimes package versions include symlinked paths -- of sibling dependencies used for resolution. @@ -489,8 +485,23 @@ buildGraph lockFile = withoutLocalPackages $ toDep :: DepType -> Text -> Maybe Text -> Bool -> Dependency toDep depType name version isDev = Dependency depType name (CEq <$> version) mempty (toEnv isDev) mempty + -- For v9, environments start empty and are set later via labels + -- during graph construction, then propagated by hydrateDepEnvs. + -- For older versions, isDev from PackageData is reliable and + -- environments are set inline. toEnv :: Bool -> Set.Set DepEnvironment - toEnv isNotRequired = Set.singleton $ if isNotRequired then EnvDevelopment else EnvProduction + toEnv isNotRequired = + if isV9 + then mempty + else Set.singleton (if isNotRequired then EnvDevelopment else EnvProduction) + + isV9 :: Bool + isV9 = lockFile.lockFileVersion == PnpmLockV9 + + applyLabels :: Dependency -> Set.Set PnpmLabel -> Dependency + applyLabels = foldr applyLabel + where + applyLabel (PnpmEnv env) = insertEnvironment env withoutLocalPackages :: Graphing Dependency -> Graphing Dependency withoutLocalPackages = Graphing.shrink (\dep -> dependencyType dep /= UserType) diff --git a/test/Pnpm/PnpmLockSpec.hs b/test/Pnpm/PnpmLockSpec.hs index aa7516228..523a81856 100644 --- a/test/Pnpm/PnpmLockSpec.hs +++ b/test/Pnpm/PnpmLockSpec.hs @@ -16,6 +16,7 @@ import DepTypes ( VerConstraint (CEq), ) import GraphUtil ( + expectDep, expectDirect, expectEdge, ) @@ -31,6 +32,19 @@ mkProdDep nameAtVersion = mkDep nameAtVersion (Just EnvProduction) mkDevDep :: Text -> Dependency mkDevDep nameAtVersion = mkDep nameAtVersion (Just EnvDevelopment) +mkBothEnvDep :: Text -> Dependency +mkBothEnvDep nameAtVersion = do + let nameAndVersionSplit = Text.splitOn "@" nameAtVersion + name = head nameAndVersionSplit + version = last nameAndVersionSplit + Dependency + NodeJSType + name + (CEq <$> Just version) + mempty + (Set.fromList [EnvProduction, EnvDevelopment]) + mempty + mkDep :: Text -> Maybe DepEnvironment -> Dependency mkDep nameAtVersion env = do let nameAndVersionSplit = Text.splitOn "@" nameAtVersion @@ -110,9 +124,16 @@ spec = do -- With the advent of lockfile v9, pnpm now has its own pnpm-workspace.yaml file. let pnpmLockV9Workspace = currentDir $(mkRelFile "test/Pnpm/testdata/pnpm-9-workspace-project/pnpm-lock.yaml") + let pnpmLockV9SharedDep = currentDir $(mkRelFile "test/Pnpm/testdata/pnpm-9-shared-dep/pnpm-lock.yaml") + let pnpmLockV9LocalDep = currentDir $(mkRelFile "test/Pnpm/testdata/pnpm-9-local-dep/pnpm-lock.yaml") + let pnpmLockV9MultiVersion = currentDir $(mkRelFile "test/Pnpm/testdata/pnpm-9-multi-version/pnpm-lock.yaml") + describe "works with v9 format" $ do checkGraph pnpmLockV9 pnpmLockV9GraphSpec describe "workspace" $ checkGraph pnpmLockV9Workspace pnpmLockV9GraphSpec + describe "shared deps" $ checkGraph pnpmLockV9SharedDep pnpmLockV9SharedDepSpec + describe "local dep env propagation" $ checkGraph pnpmLockV9LocalDep pnpmLockV9LocalDepSpec + describe "multi-version env labeling" $ checkGraph pnpmLockV9MultiVersion pnpmLockV9MultiVersionSpec pnpmLockGraphSpec :: Graphing Dependency -> Spec pnpmLockGraphSpec graph = do @@ -407,5 +428,62 @@ pnpmLockV9GraphSpec graph = do -- ├── sax 1.4.4 -- └── xmlbuilder 11.0.1 hasEdge (mkProdDep "uri-js@4.4.1") (mkProdDep "punycode@2.3.1") - hasEdge (mkDevDep "xml2js@0.6.2") (mkProdDep "sax@1.4.4") - hasEdge (mkDevDep "xml2js@0.6.2") (mkProdDep "xmlbuilder@11.0.1") + hasEdge (mkDevDep "xml2js@0.6.2") (mkDevDep "sax@1.4.4") + hasEdge (mkDevDep "xml2js@0.6.2") (mkDevDep "xmlbuilder@11.0.1") + +pnpmLockV9SharedDepSpec :: Graphing Dependency -> Spec +pnpmLockV9SharedDepSpec graph = do + let hasEdge :: Dependency -> Dependency -> Expectation + hasEdge = expectEdge graph + + describe "buildGraph with shared deps" $ do + it "should mark direct dependencies of project as direct" $ do + expectDirect + [ mkProdDep "uri-js@4.4.1" + , mkDevDep "xml2js@0.6.2" + ] + graph + + it "should build edges with correct environments" $ do + -- sax is reachable from both prod (uri-js) and dev (xml2js) -- gets both + hasEdge (mkProdDep "uri-js@4.4.1") (mkBothEnvDep "sax@1.4.4") + hasEdge (mkDevDep "xml2js@0.6.2") (mkBothEnvDep "sax@1.4.4") + -- punycode is prod-only (via uri-js) + hasEdge (mkProdDep "uri-js@4.4.1") (mkProdDep "punycode@2.3.1") + -- xmlbuilder is dev-only (via xml2js) + hasEdge (mkDevDep "xml2js@0.6.2") (mkDevDep "xmlbuilder@11.0.1") + +-- Bug 2 regression: transitive deps of a local (file:) package must inherit +-- the environment from the importer that depends on it, even though the +-- local package node itself is removed from the final graph. +pnpmLockV9LocalDepSpec :: Graphing Dependency -> Spec +pnpmLockV9LocalDepSpec graph = do + let hasEdge :: Dependency -> Dependency -> Expectation + hasEdge = expectEdge graph + + describe "buildGraph with local dep" $ do + it "should not include the local package in the final graph" $ do + -- local-pkg is UserType and should be removed by withoutLocalPackages + expectDirect + [ mkProdDep "express@4.18.2" + ] + graph + + it "should propagate prod environment through local package to transitive deps" $ do + -- express is a transitive dep of local-pkg (prod) — must be prod + expectDep (mkProdDep "express@4.18.2") graph + -- body-parser is a transitive dep of express — must also be prod + expectDep (mkProdDep "body-parser@1.20.1") graph + hasEdge (mkProdDep "express@4.18.2") (mkProdDep "body-parser@1.20.1") + +-- Bug 3 regression: when two workspace packages depend on different versions +-- of the same package (one prod, one dev), each version must receive only its +-- own environment label — not both. +pnpmLockV9MultiVersionSpec :: Graphing Dependency -> Spec +pnpmLockV9MultiVersionSpec graph = do + describe "buildGraph with multi-version deps" $ do + it "should label each version with only its own environment" $ do + -- sax@1.2.1 is prod-only (from app-a) + expectDep (mkProdDep "sax@1.2.1") graph + -- sax@1.4.4 is dev-only (from app-b) + expectDep (mkDevDep "sax@1.4.4") graph diff --git a/test/Pnpm/testdata/pnpm-9-local-dep/pnpm-lock.yaml b/test/Pnpm/testdata/pnpm-9-local-dep/pnpm-lock.yaml new file mode 100644 index 000000000..ce40e7d4b --- /dev/null +++ b/test/Pnpm/testdata/pnpm-9-local-dep/pnpm-lock.yaml @@ -0,0 +1,37 @@ +lockfileVersion: '9.0' + +settings: + autoInstallPeers: true + excludeLinksFromLockfile: false + +# Bug 2 regression: a local (file:) package is a direct production dep. +# Its transitive deps (express, body-parser) must inherit EnvProduction +# even though the local package node is removed from the final graph. +importers: + .: + dependencies: + local-pkg: + specifier: file:packages/local-pkg + version: file:packages/local-pkg + +packages: + file:packages/local-pkg: + resolution: {directory: packages/local-pkg, type: directory} + name: local-pkg + + express@4.18.2: + resolution: {integrity: sha512-xxxxxxxxxx==} + + body-parser@1.20.1: + resolution: {integrity: sha512-xxxxxxxxxx==} + +snapshots: + file:packages/local-pkg: + dependencies: + express: 4.18.2 + + express@4.18.2: + dependencies: + body-parser: 1.20.1 + + body-parser@1.20.1: {} diff --git a/test/Pnpm/testdata/pnpm-9-multi-version/pnpm-lock.yaml b/test/Pnpm/testdata/pnpm-9-multi-version/pnpm-lock.yaml new file mode 100644 index 000000000..e5ca8b7ef --- /dev/null +++ b/test/Pnpm/testdata/pnpm-9-multi-version/pnpm-lock.yaml @@ -0,0 +1,35 @@ +lockfileVersion: '9.0' + +settings: + autoInstallPeers: true + excludeLinksFromLockfile: false + +# Bug 3 regression: two workspace packages depend on different versions of +# the same package — one as prod, one as dev. Each version must receive +# only its own environment label, not both. +importers: + .: {} + + packages/app-a: + dependencies: + sax: + specifier: ^1.2.1 + version: 1.2.1 + + packages/app-b: + devDependencies: + sax: + specifier: ^1.4.4 + version: 1.4.4 + +packages: + sax@1.2.1: + resolution: {integrity: sha512-xxxxxxxxxx==} + + sax@1.4.4: + resolution: {integrity: sha512-yyyyyyyyyy==} + +snapshots: + sax@1.2.1: {} + + sax@1.4.4: {} diff --git a/test/Pnpm/testdata/pnpm-9-shared-dep/pnpm-lock.yaml b/test/Pnpm/testdata/pnpm-9-shared-dep/pnpm-lock.yaml new file mode 100644 index 000000000..4bcd354cb --- /dev/null +++ b/test/Pnpm/testdata/pnpm-9-shared-dep/pnpm-lock.yaml @@ -0,0 +1,50 @@ +lockfileVersion: '9.0' + +settings: + autoInstallPeers: true + excludeLinksFromLockfile: false + +importers: + .: + dependencies: + uri-js: + specifier: ^4.4.1 + version: 4.4.1 + devDependencies: + xml2js: + specifier: ^0.6.2 + version: 0.6.2 + +packages: + punycode@2.3.1: + resolution: {integrity: sha512-vYt7UD1U9Wg6138shLtLOvdAu+8DsC/ilFtEVHcH+wydcSpNE20AfSOduf6MkRFahL5FY7X1oU7nKVZFtfq8Fg==} + engines: {node: '>=6'} + + sax@1.4.4: + resolution: {integrity: sha512-1n3r/tGXO6b6VXMdFT54SHzT9ytu9yr7TaELowdYpMqY/Ao7EnlQGmAQ1+RatX7Tkkdm6hONI2owqNx2aZj5Sw==} + + uri-js@4.4.1: + resolution: {integrity: sha512-7rKUyy33Q1yc98pQ1DAmLtwX109F7TIfWlW1Ydo8Wl1ii1SeHieeh0HHfPeL2fMXK6z0s8ecKs9frCuLJvndBg==} + + xml2js@0.6.2: + resolution: {integrity: sha512-T4rieHaC1EXcES0Kxxj4JWgaUQHDk+qwHcYOCFHfiwKz7tOVPLq7Hjq9dM1WCMhylqMEfP7hMcOIChvotiZegA==} + + xmlbuilder@11.0.1: + resolution: {integrity: sha512-fDlsI/kFEx7gLvbecc0/ohLG50fugQp8ryHzMTuW9vSa1GJ0XYWKnhsUx7oie3G98+r56aTQIUB4kht42R3JvA==} + +snapshots: + punycode@2.3.1: {} + + sax@1.4.4: {} + + uri-js@4.4.1: + dependencies: + punycode: 2.3.1 + sax: 1.4.4 + + xml2js@0.6.2: + dependencies: + sax: 1.4.4 + xmlbuilder: 11.0.1 + + xmlbuilder@11.0.1: {}