Python: report installed dependency versions, and pick range bounds by kind (ANE-3089)

Changelog.md

@@ -1,5 +1,11 @@
 # FOSSA CLI Changelog
 
+## Unreleased
+
+- Dependency versions: When a dependency declares a version range rather than a single version, the version reported no longer depends on the order the bounds were written in. `cryptography<60.0.0, >=46.0.3` and `cryptography>=46.0.3, <60.0.0` both now report `46.0.3`; previously the first reported `60.0.0`, a version the range excludes.
+- Python: `requirements.txt` and `setup.py` dependencies are now reported at the version installed in the environment, when `python` and `pip` are available. Previously a dependency declaring a range was reported at one of its bounds, and a dependency declaring no version at all was reported with no version, even though the CLI had already read the installed version from `pip show` to build the dependency graph.
+- Python: Package names are now matched using [PEP 503](https://peps.python.org/pep-0503/#normalized-names) normalization, so `Zope.Interface`, `zope_interface`, and `zope-interface` are recognized as the same package.
+
 ## 3.17.17
 
 - License Scanning: Detect an OFL-1.1 license notice correctly ([#1742](https://github.com/fossas/fossa-cli/pull/1742))

docs/references/strategies/languages/python/setuptools.md

@@ -37,6 +37,15 @@ Dependencies found in requirements.txt have a spec defined by
 markers (e.g. python version, OS, ...). The resulting graph contains packages
 tagged with environment markers.
 
+Where a dependency declares a version range rather than a single version, the
+CLI reports the version installed in the environment it is run in, which it
+reads from `python -m pip list` and `python -m pip show`. This is the same data
+used to find transitive dependencies, so running the CLI inside the project's
+virtual environment matters for versions as well as for edges. If neither
+`python` nor `pip` is available, the CLI falls back to reporting the lowest
+version the range allows: `cryptography>=46.0.3, <60.0.0` is reported as
+`46.0.3`.
+
 ## Analysis: setup.py
 
 ### Installed packages

spectrometer.cabal

@@ -734,6 +734,7 @@ test-suite unit-tests
     Scala.SbtDependencyTreeParsingSpec
     Scala.SbtDependencyTreeSpec
     Sqlite.SqliteSpec
+    Srclib.ConverterSpec
     Srclib.TypesSpec
     Swift.PackageResolvedSpec
     Swift.PackageSwiftSpec

src/Srclib/Converter.hs

@@ -14,7 +14,6 @@ import Prelude
 
 import Algebra.Graph.AdjacencyMap qualified as AM
 import App.Fossa.Analyze.Project (ProjectResult (..))
-import Control.Applicative ((<|>))
 import Data.Aeson qualified as Aeson
 import Data.Set qualified as Set
 import Data.String.Conversion (toText)
@@ -134,18 +133,57 @@ toLocator dep =
     , locatorRevision = verConstraintToRevision =<< dependencyVersion dep
     }
 
+-- | Choose the single revision that best stands in for a version constraint.
+--
+-- A locator carries one revision, but a constraint may describe a whole range,
+-- so something has to be discarded. 'bestRevision' ranks the candidates and
+-- keeps the best one rather than the leftmost, so that @>=1.0, <2.0@ and
+-- @<2.0, >=1.0@ produce the same answer. Picking the leftmost meant the
+-- reported version depended on the order the bounds happened to be written in,
+-- and an upper bound written first won --- reporting @<2.0@ as @2.0@, the one
+-- version the range explicitly excludes.
 verConstraintToRevision :: VerConstraint -> Maybe Text
-verConstraintToRevision = \case
-  CEq ver -> Just ver
+verConstraintToRevision = fmap fst . bestRevision
+
+-- | How faithfully a revision pulled out of a constraint represents that
+-- constraint. The derived 'Ord' instance follows constructor order, so
+-- constructors listed later are better candidates.
+data RevisionQuality
+  = -- | An exclusive upper bound, e.g. @<2.0@. Not a version the range admits,
+    -- and frequently not a version that was ever published.
+    ExclusiveUpper
+  | -- | An exclusive lower bound, e.g. @>1.0@. Also outside the range.
+    ExclusiveLower
+  | -- | An inclusive upper bound, e.g. @<=2.0@: the newest version allowed.
+    InclusiveUpper
+  | -- | An inclusive lower bound, e.g. @>=1.0@: the oldest version allowed.
+    InclusiveLower
+  | -- | The constraint named one version outright.
+    Exact
+  deriving (Eq, Ord)
+
+-- | Extract the best available revision from a constraint, tagged with how good
+-- a stand-in it is. Ties keep the left-hand candidate.
+bestRevision :: VerConstraint -> Maybe (Text, RevisionQuality)
+bestRevision = \case
+  CEq ver -> Just (ver, Exact)
+  -- ~=1.2 means >=1.2, ==1.*, so the version named is an inclusive lower bound.
+  CCompatible ver -> Just (ver, InclusiveLower)
+  CGreaterOrEq ver -> Just (ver, InclusiveLower)
+  CGreater ver -> Just (ver, ExclusiveLower)
+  CLessOrEq ver -> Just (ver, InclusiveUpper)
+  CLess ver -> Just (ver, ExclusiveUpper)
   CURI _ -> Nothing -- we can't represent this in a locator
-  CCompatible ver -> Just ver
-  CAnd a b -> verConstraintToRevision a <|> verConstraintToRevision b
-  COr a b -> verConstraintToRevision a <|> verConstraintToRevision b
-  CLess ver -> Just ver -- ugh
-  CLessOrEq ver -> Just ver -- ugh
-  CGreater ver -> Just ver -- ugh
-  CGreaterOrEq ver -> Just ver -- ugh
   CNot _ -> Nothing -- we can't represent this in a locator
+  CAnd a b -> better (bestRevision a) (bestRevision b)
+  -- For a disjunction the ranking is arbitrary rather than principled: the
+  -- branches are alternative ranges, and nothing in the constraint says which
+  -- one is installed. We reuse it so the result is at least order-independent.
+  COr a b -> better (bestRevision a) (bestRevision b)
+  where
+    better Nothing y = y
+    better x Nothing = x
+    better (Just x) (Just y) = Just $ if snd y > snd x then y else x
 
 depTypeToFetcher :: DepType -> Text
 depTypeToFetcher = \case

src/Strategy/Python/Poetry/Common.hs

@@ -15,7 +15,7 @@ import Data.Map (Map)
 import Data.Map.Strict qualified as Map
 import Data.Maybe (fromMaybe)
 import Data.Set qualified as Set
-import Data.Text (Text, replace, toLower)
+import Data.Text (Text)
 import DepTypes (
   DepEnvironment (EnvDevelopment, EnvOther, EnvProduction, EnvTesting),
   DepType (GitType, PipType, URLType),
@@ -42,7 +42,7 @@ import Strategy.Python.Poetry.PyProject (
   allPoetryNonProductionDeps,
   toDependencyVersion,
  )
-import Strategy.Python.Util (reqToDependency)
+import Strategy.Python.Util (reqToDependency, toCanonicalName)
 
 -- | Gets build backend of pyproject.
 getPoetryBuildBackend :: PyProject -> Maybe Text
@@ -187,26 +187,6 @@ poetrytoDependency depEnvs name deps =
     depLocations = []
     depTags = Map.empty
 
--- | Converts text to canonical python name for dependency.
--- Relevant Docs: https://www.python.org/dev/peps/pep-0426/#id28
--- Poetry Code: https://github.com/python-poetry/poetry/blob/master/poetry/utils/helpers.py#L35
---
--- Poetry performs this operation inconsistently at the time of writing for package name and it's dependencies
--- within the lock file.
---
---  ```toml
---  [package.dependencies]
---  MarkupSafe = ">=2.0"
---  ....
---
--- [[package]]
--- name = "markupsafe"
--- version = "2.0.1"
--- ...
--- ```
-toCanonicalName :: Text -> Text
-toCanonicalName t = toLower $ replace "_" "-" (replace "." "-" t)
-
 -- | Maps poetry lock package to map of package name and associated dependency.
 makePackageToLockDependencyMap :: [PackageName] -> [PoetryLockPackage] -> Map.Map PackageName Dependency
 makePackageToLockDependencyMap prodPkgs pkgs = Map.fromList $ (\x -> (lockCanonicalPackageName x, toDependency x)) <$> (filter supportedPoetryLockDep pkgs)

src/Strategy/Python/Util.hs

@@ -9,6 +9,7 @@ module Strategy.Python.Util (
   reqName,
   requirementParser,
   reqToDependency,
+  toCanonicalName,
   toConstraint,
 ) where
 
@@ -33,6 +34,22 @@ import Text.URI qualified as URI
 import Toml qualified
 import Toml.Schema qualified
 
+-- | Normalize a Python package name per [PEP 503][pep-503]: collapse runs of
+-- @-@, @_@, and @.@ into a single @-@, then lowercase. @Zope.Interface@ and
+-- @zope_interface@ both become @zope-interface@.
+--
+-- Package names reach us from several sources that disagree on punctuation and
+-- case --- a requirements.txt line, a poetry.lock entry, @pip list@ output ---
+-- so they have to be normalized before they can be compared.
+--
+-- [pep-503]: https://peps.python.org/pep-0503/#normalized-names
+toCanonicalName :: Text -> Text
+toCanonicalName =
+  Text.toLower
+    . Text.intercalate "-"
+    . filter (not . Text.null)
+    . Text.split (\c -> c == '-' || c == '_' || c == '.')
+
 pkgToReq :: PythonPackage -> Req
 pkgToReq p =
   NameReq (pkgName p) Nothing (Just [Version OpEq (pkgVersion p)]) Nothing
@@ -66,11 +83,12 @@ buildGraphSetupFile maybePackages pyPackageName pyReqs cfgPackageName cfgReqs =
   where
     addDeps :: [PythonPackage] -> Maybe Text -> [Req] -> GrapherC Req Identity ()
     addDeps packages maybeName reqs = do
+      let resolved = map (withInstalledVersion packages) reqs
       case maybeName of
-        Nothing -> for_ reqs direct
+        Nothing -> for_ resolved direct
         Just packageName ->
-          case (find (\p -> Text.toLower (pkgName p) == Text.toLower (packageName)) packages) of
-            Nothing -> for_ reqs direct
+          case (find (\p -> toCanonicalName (pkgName p) == toCanonicalName packageName) packages) of
+            Nothing -> for_ resolved direct
             Just pkg ->
               for_ (requires pkg) $ \c -> do
                 let r = pkgToReq c
@@ -83,15 +101,42 @@ buildGraph maybePackages reqs = do
     case maybePackages of
       Nothing -> Graphing.fromList reqs
       Just packages -> do
+        -- Resolve the whole list up front so that @direct@ below and the parent
+        -- looked up by @findParent@ are the same value. The grapher keys nodes
+        -- on 'Req', whose 'Eq' instance covers the version, so resolving in one
+        -- place and not the other would produce two nodes for the same package
+        -- --- one carrying the edges, one carrying the declared range.
+        let resolved = map (withInstalledVersion packages) reqs
         run . evalGrapher $ do
-          for_ reqs direct
+          for_ resolved direct
           for_ packages $ \p -> do
-            case findParent (pkgName p) of
+            case findParent resolved (pkgName p) of
               Just parent -> addChildren parent p
               Nothing -> pure ()
   where
-    findParent :: Text -> Maybe Req
-    findParent packageName = find (\r -> Text.toLower (reqName r) == Text.toLower (packageName)) reqs
+    findParent :: [Req] -> Text -> Maybe Req
+    findParent rs packageName = find (\r -> toCanonicalName (reqName r) == toCanonicalName packageName) rs
+
+-- | Replace a requirement's declared version constraint with the version that
+-- pip reports as installed, when the package is present in the environment.
+--
+-- A manifest records the versions a project /accepts/; pip records the version
+-- that is /there/. Only the latter can become a locator revision honestly,
+-- because a locator holds one revision and collapsing a range down to one is
+-- always a guess --- @cryptography>=46.0.3, <60.0.0@ gives us no way to know
+-- which release was installed.
+--
+-- Extras and the environment marker are carried through untouched: they
+-- describe the requirement, not the version it resolved to, and 'reqToDependency'
+-- turns the marker into the dependency's tags.
+withInstalledVersion :: [PythonPackage] -> Req -> Req
+withInstalledVersion packages = \case
+  -- A URL requirement names its source outright; there is no range to replace.
+  r@UrlReq{} -> r
+  r@(NameReq name extras _ marker) ->
+    case find (\p -> toCanonicalName (pkgName p) == toCanonicalName name) packages of
+      Nothing -> r
+      Just pkg -> NameReq name extras (Just [Version OpEq (pkgVersion pkg)]) marker
 
 addChildren :: (Has (Grapher Req) sig m) => Req -> PythonPackage -> m ()
 addChildren parent pkg = do

test/Python/Poetry/CommonSpec.hs

@@ -7,7 +7,7 @@ import Data.Set qualified as Set
 import Data.Text (Text)
 import Data.Text.IO qualified as TIO
 import DepTypes (DepEnvironment (..), DepType (..), Dependency (..), VerConstraint (..))
-import Strategy.Python.Poetry.Common (getPoetryBuildBackend, makePackageToLockDependencyMap, pyProjectDeps, supportedPoetryLockDep, supportedPyProjectDep, toCanonicalName)
+import Strategy.Python.Poetry.Common (getPoetryBuildBackend, makePackageToLockDependencyMap, pyProjectDeps, supportedPoetryLockDep, supportedPyProjectDep)
 import Strategy.Python.Poetry.PoetryLock (
   ObjectVersion (..),
   PackageName (..),
@@ -163,12 +163,6 @@ spec = do
   pep621Contents <- runIO (TIO.readFile "test/Python/Poetry/testdata/pep621/pyproject.toml")
   pep621MixedContents <- runIO (TIO.readFile "test/Python/Poetry/testdata/pep621-mixed/pyproject.toml")
 
-  describe "toCanonicalName" $ do
-    it "should convert text to lowercase" $
-      toCanonicalName "GreatScore" `shouldBe` "greatscore"
-    it "should replace underscore (_) to hyphens (-)" $
-      toCanonicalName "my_oh_so_great_pkg" `shouldBe` "my-oh-so-great-pkg"
-
   describe "getDependencies" $ do
     it "should get all dependencies" $
       pyProjectDeps expectedPyProject `shouldMatchList` expectedDeps

test/Python/ReqTxtSpec.hs

@@ -5,10 +5,13 @@ module Python.ReqTxtSpec (
 ) where
 
 import Control.Monad (void)
+import Data.Foldable (find)
 import Data.Map.Strict qualified as Map
+import Data.Text (Text)
 import DepTypes
 import Effect.Grapher
 import Graphing (Graphing)
+import Graphing qualified
 import Strategy.Python.Pip (PythonPackage (..))
 import Strategy.Python.Util
 import Text.URI.QQ (uri)
@@ -106,6 +109,20 @@ expectedDeps =
       )
   ]
 
+-- | What the graph looks like when pip reports the installed packages: the
+-- declared constraints on pkgOne and pkgTwo give way to the versions actually
+-- present, and their transitive dependencies appear.
+expectedInstalledDeps :: [ExpectedDependency]
+expectedInstalledDeps = map resolve expectedDeps
+  where
+    resolve (ExpectedDependency (dep, children)) =
+      ExpectedDependency (withInstalled dep, children)
+
+    withInstalled dep = case dependencyName dep of
+      "pkgOne" -> dep{dependencyVersion = Just (CEq "1.0.0")}
+      "pkgTwo" -> dep{dependencyVersion = Just (CEq "1")}
+      _ -> dep
+
 traverseDirect :: [ExpectedDependency] -> Graphing Dependency
 traverseDirect deps = run . evalGrapher $ do
   traverse
@@ -142,4 +159,69 @@ spec =
     it "should only report transitive dependencies for packages found in req.txt" $ do
       let result = buildGraph (Just installedPackages) setupPyInput
 
-      result `shouldBe` traverseDirectAndDeep expectedDeps
+      result `shouldBe` traverseDirectAndDeep expectedInstalledDeps
+
+    it "should report the installed version rather than a declared range" $ do
+      -- pkgOne is declared as ">=1.0.0, <2.0.0" but installed at 1.0.0. A
+      -- locator can only carry one revision, and the environment knows which
+      -- one is actually there.
+      let result = buildGraph (Just installedPackages) setupPyInput
+
+      versionOf "pkgOne" result `shouldBe` Just (Just (CEq "1.0.0"))
+
+    it "should report the installed version regardless of bound order" $ do
+      -- The declared range says the same thing either way round, so the
+      -- reported version must not depend on which bound was written first.
+      let lowerFirst = [NameReq "pkgOne" Nothing (Just [Version OpGtEq "1.0.0", Version OpLt "2.0.0"]) Nothing]
+          upperFirst = [NameReq "pkgOne" Nothing (Just [Version OpLt "2.0.0", Version OpGtEq "1.0.0"]) Nothing]
+
+      versionOf "pkgOne" (buildGraph (Just installedPackages) upperFirst)
+        `shouldBe` versionOf "pkgOne" (buildGraph (Just installedPackages) lowerFirst)
+
+    it "should fill in a version for a requirement that declares none" $ do
+      -- A bare package name on a requirements.txt line otherwise produces a
+      -- locator with no revision at all.
+      let result = buildGraph (Just installedPackages) setupPyInput
+
+      versionOf "pkgTwo" result `shouldBe` Just (Just (CEq "1"))
+
+    it "should match installed packages by their canonical name" $ do
+      -- PEP 503: "Zope.Interface", "zope_interface" and "zope-interface" all
+      -- name the same package.
+      let reqs = [NameReq "Zope.Interface" Nothing (Just [Version OpGtEq "5.0"]) Nothing]
+          installed = [PythonPackage "zope-interface" "5.4.0" []]
+
+      versionOf "Zope.Interface" (buildGraph (Just installed) reqs)
+        `shouldBe` Just (Just (CEq "5.4.0"))
+
+    it "should preserve environment markers when substituting a version" $ do
+      -- The marker becomes the dependency's tags, and describes the
+      -- requirement rather than the version it resolved to.
+      let marker = MarkerExpr "sys_platform" (MarkerOperator OpEq) "linux"
+          reqs = [NameReq "pkgOne" Nothing (Just [Version OpGtEq "1.0.0"]) (Just marker)]
+          result = buildGraph (Just installedPackages) reqs
+
+      tagsOf "pkgOne" result `shouldBe` Just (Map.fromList [("sys_platform", ["linux"])])
+
+    it "should leave a URL requirement alone" $ do
+      let reqs = [UrlReq "pkgOne" Nothing [uri|https://example.com|] Nothing]
+
+      versionOf "pkgOne" (buildGraph (Just installedPackages) reqs)
+        `shouldBe` Just (Just (CURI "https://example.com"))
+
+    it "should keep the declared constraint when pip reports nothing" $ do
+      let result = buildGraph Nothing setupPyInput
+
+      versionOf "pkgOne" result `shouldBe` Just (Just (CAnd (CGreaterOrEq "1.0.0") (CLess "2.0.0")))
+
+-- | Look up a dependency by name and return its version, or 'Nothing' if no
+-- dependency by that name is in the graph.
+versionOf :: Text -> Graphing Dependency -> Maybe (Maybe VerConstraint)
+versionOf name = fmap dependencyVersion . findDep name
+
+-- | Look up a dependency by name and return its tags.
+tagsOf :: Text -> Graphing Dependency -> Maybe (Map.Map Text [Text])
+tagsOf name = fmap dependencyTags . findDep name
+
+findDep :: Text -> Graphing Dependency -> Maybe Dependency
+findDep name = find ((== name) . dependencyName) . Graphing.vertexList