refactor: remove satNimSelection two-pass dead code, rename to satSolving (#1651)

Author: jmgomez

src/nimble.nim

@@ -2026,7 +2026,7 @@ proc runVNext*(options: var Options, nimBin: string) {.instrument.} =
     # For non-project-dir develops, solve from each cloned package as root
     if not options.thereIsNimbleFile and developedPkgs.len > 0:
       for rootPkg in developedPkgs:
-        options.satResult = initSATResult(satNimSelection)
+        options.satResult = initSATResult(satSolving)
         solvePkgs(rootPkg, options, nimBin)
         developFromSolution(rootPkg.basicInfo.name, options, nimBin)
       return
@@ -2041,7 +2041,7 @@ proc runVNext*(options: var Options, nimBin: string) {.instrument.} =
       options.action.packages.len == 0 and options.thereIsNimbleFile
   if isGlobalInstallRoot:
     # nimble install -g: install current project globally
-    options.satResult = initSATResult(satNimSelection)
+    options.satResult = initSATResult(satSolving)
     rootPackage = getPkgInfoFromDirWithDeclarativeParser(getCurrentDir(), options, nimBin = nimBin)
     solvePkgs(rootPackage, options, nimBin)
     let rootSolvedPkg = SolvedPackage(
@@ -2057,7 +2057,7 @@ proc runVNext*(options: var Options, nimBin: string) {.instrument.} =
   elif options.thereIsNimbleFile and not isGlobalInstall and
        not (findNimbleFile(getCurrentDir(), error = false, options, warn = false).splitFile.name.isNim and
             options.action.typ == actionInstall and options.action.packages.len > 0):
-    options.satResult = initSATResult(satNimSelection)
+    options.satResult = initSATResult(satSolving)
     options.isFilePathDiscovering = true
     #we need to skip validation for root
     rootPackage = getPkgInfoFromDirWithDeclarativeParser(getCurrentDir(), options, nimBin = nimBin)
@@ -2068,7 +2068,7 @@ proc runVNext*(options: var Options, nimBin: string) {.instrument.} =
   elif options.action.typ == actionInstall:
     #Global install
     for pkg in options.action.packages:
-      options.satResult = initSATResult(satNimSelection)
+      options.satResult = initSATResult(satSolving)
       # Download package info to pkgcache WITHOUT submodules - submodules are
       # populated in buildtemp during actual install to avoid mutating shared cache (issue #1592)
       # Force git clone (not tarball) so .git and .gitmodules are preserved for buildtemp

src/nimblepkg/declarativeparser.nim

@@ -684,8 +684,7 @@ proc toRequiresInfo*(pkgInfo: PackageInfo, options: Options, nimBin: string, nim
     result.infoKind = pikRequires
 
   if nimbleFileInfo.nestedRequires and options.action.typ != actionCheck: #When checking we want to fail on porpuse
-    if options.satResult.pass == satNimSelection:
-      assert nimBin != "", "Cant fallback to the vm parser as there is no nim bin."
+    assert nimBin != "", "Cant fallback to the vm parser as there is no nim bin."
 
     if options.verbosity <= LowPriority:
       for line in nimbleFileInfo.declarativeParserErrorLines:

src/nimblepkg/developfile.nim

@@ -9,7 +9,7 @@ import sets, sequtils, os, strformat, tables, hashes, strutils, math,
 
 import typetraits except distinctBase
 import compat/json
-import common, cli, packageinfotypes, packageinfo, packageparser, options,
+import common, cli, packageinfotypes, packageinfo, options,
        version, paths, displaymessages, sha1hashes,
        tools, vcstools, syncfile, lockfile, declarativeparser
 
@@ -172,13 +172,7 @@ proc validatePackage(pkgPath: Path, options: Options, nimBin: string):
   ##                 not a valid package directory.
 
   try:
-    if options.satResult.pass == satNimSelection:
-      result.pkgInfo = getPkgInfoFromDirWithDeclarativeParser(string(pkgPath), options, nimBin)
-      #TODO find a way to validate the package, for now just mark the declarativeParser as failed
-      #as we dont have Nim selected yet.
-      options.satResult.declarativeParseFailed = true
-    else:
-      result.pkgInfo = getPkgInfo(string(pkgPath), options, nimBin, true)
+    result.pkgInfo = getPkgInfoFromDirWithDeclarativeParser(string(pkgPath), options, nimBin)
   except CatchableError as error:
     result.error = error
 

src/nimblepkg/download.nim

@@ -7,7 +7,7 @@ import parseutils, os, strutils, tables, uri, strformat,
 import compat/[json, osproc]
 from algorithm import SortOrder, sorted
 
-import packageinfotypes, packageparser, version, tools, common, options, cli,
+import packageinfotypes, version, tools, common, options, cli,
        sha1hashes, vcstools, displaymessages, packageinfo, config, declarativeparser, packagemetadatafile
 
 const userAgent = "nimble/" & nimbleVersion
@@ -919,10 +919,7 @@ proc downloadPkg*(url: string, verRange: VersionRange,
   var pkgInfo: PackageInfo
   ## Makes sure that the downloaded package's version satisfies the requested
   ## version range.
-  pkginfo = if options.satResult.pass == satNimSelection: #TODO later when in vnext we should just use this code path and fallback inside the toRequires if we can
-    getPkgInfoFromDirWithDeclarativeParser(result.dir, options, nimBin)
-  else:
-    getPkgInfo(result.dir, options, nimBin)
+  pkginfo = getPkgInfoFromDirWithDeclarativeParser(result.dir, options, nimBin)
   if pkginfo.basicInfo.version notin verRange:
     raise nimbleError(
       "Downloaded package's version does not satisfy requested version " &
@@ -999,10 +996,7 @@ proc downloadPkgAsync*(url: string, verRange: VersionRange,
   var pkgInfo: PackageInfo
   ## Makes sure that the downloaded package's version satisfies the requested
   ## version range.
-  pkginfo = if options.satResult.pass == satNimSelection: #TODO later when in vnext we should just use this code path and fallback inside the toRequires if we can
-    getPkgInfoFromDirWithDeclarativeParser(result.dir, options, nimBin)
-  else:
-    getPkgInfo(result.dir, options, nimBin)
+  pkginfo = getPkgInfoFromDirWithDeclarativeParser(result.dir, options, nimBin)
   if pkginfo.basicInfo.version notin verRange:
     raise nimbleError(
       "Downloaded package's version does not satisfy requested version " &

src/nimblepkg/nimblesat.nim

@@ -90,10 +90,6 @@ proc getMinimalInfo*(pkg: PackageInfo, options: Options): PackageMinimalInfo =
   result.name = if pkg.basicInfo.name.isNim: "nim" else: pkg.basicInfo.name
   result.version = pkg.basicInfo.version
   result.requires = pkg.requires.map(convertNimAliasToNim)
-  if options.satResult.pass != satNimSelection and
-     (options.action.typ in {actionLock, actionDeps} or options.hasNimInLockFile()):
-    # Keep nim requirements with special versions (e.g., #devel, #commit-sha)
-    result.requires = result.requires.filterIt(not it.isNim or it.ver.kind == verSpecial)
   result.url = pkg.metadata.url
 
 proc getMinimalInfo*(nimbleFile: string, options: Options, nimBin: string): PackageMinimalInfo =
@@ -105,10 +101,6 @@ proc getMinimalInfo*(nimbleFile: string, options: Options, nimBin: string): Pack
   result.version = pkg.basicInfo.version
   result.requires = pkg.requires.map(convertNimAliasToNim)
   result.url = pkg.metadata.url
-  if options.satResult.pass != satNimSelection and
-     (options.action.typ in {actionLock, actionDeps} or options.hasNimInLockFile()):
-    # Keep nim requirements with special versions (e.g., #devel, #commit-sha)
-    result.requires = result.requires.filterIt(not it.isNim or it.ver.kind == verSpecial)
 
 proc getMinimalInfoFromContent*(content: string, name: string, version: Version,
                                  url: string, options: Options): Option[PackageMinimalInfo] =
@@ -127,11 +119,6 @@ proc getMinimalInfoFromContent*(content: string, name: string, version: Version,
   var activeFeatures = initTable[PkgTuple, seq[string]]()
   pkgInfo.requires = info.getRequires(activeFeatures).map(convertNimAliasToNim)
 
-  if options.satResult.pass != satNimSelection and
-     (options.action.typ in {actionLock, actionDeps} or options.hasNimInLockFile()):
-    # Keep nim requirements with special versions (e.g., #devel, #commit-sha)
-    pkgInfo.requires = pkgInfo.requires.filterIt(not it.isNim or it.ver.kind == verSpecial)
-
   return some(pkgInfo)
 
 proc hasVersion*(packageVersions: PackageVersions, pv: PkgTuple): bool =
@@ -702,10 +689,7 @@ proc downloadPkgFromUrl*(pv: PkgTuple, options: Options, doPrompt = false, nimBi
 
 proc downloadPkInfoForPv*(pv: PkgTuple, options: Options, doPrompt = false, nimBin: string): PackageInfo  =
   let downloadRes = downloadPkgFromUrl(pv, options, doPrompt, nimBin)
-  if options.satResult.pass in {satNimSelection}:
-    result = getPkgInfoFromDirWithDeclarativeParser(downloadRes[0].dir, options, nimBin)
-  else:
-    result = getPkgInfo(downloadRes[0].dir, options, nimBin, forValidation = false, onlyMinimalInfo = false)
+  result = getPkgInfoFromDirWithDeclarativeParser(downloadRes[0].dir, options, nimBin)
 
 proc getAllNimReleases(options: Options, nimVersion: Option[Version]): seq[PackageMinimalInfo] =
   let releases = getOfficialReleases(options)
@@ -857,11 +841,7 @@ proc getPackageMinimalVersionsFromRepo*(repoDir: string, pkg: PkgTuple, version:
           copyDir(repoDir, tempDir)
           tempDirCreated = true
         discard doCheckout(downloadMethod, tempDir, tag, versionDiscoveryOptions)
-        let nimbleFile = findNimbleFile(tempDir, true, options, warn = false)
-        if options.satResult.pass in {satNimSelection}:
-          result.addUnique getPkgInfoFromDirWithDeclarativeParser(tempDir, options, nimBin).getMinimalInfo(options)
-        else:
-          result.addUnique getMinimalInfo(nimbleFile, options, nimBin)
+        result.addUnique getPkgInfoFromDirWithDeclarativeParser(tempDir, options, nimBin).getMinimalInfo(options)
         #here we copy the directory to its own folder so we have it cached for future usage
         let downloadInfo = getPackageDownloadInfo((name, tagVersion.toVersionRange()), options)
         if not dirExists(downloadInfo.downloadDir):
@@ -874,18 +854,11 @@ proc getPackageMinimalVersionsFromRepo*(repoDir: string, pkg: PkgTuple, version:
 
   # Add HEAD version last (tagged releases take precedence if same version exists)
   try:
-    if options.satResult.pass in {satNimSelection}:
-      result.addUnique getPkgInfoFromDirWithDeclarativeParser(repoDir, options, nimBin).getMinimalInfo(options)
-    else:
-      result.addUnique getPkgInfo(repoDir, options, nimBin).getMinimalInfo(options)
+    result.addUnique getPkgInfoFromDirWithDeclarativeParser(repoDir, options, nimBin).getMinimalInfo(options)
   except CatchableError as e:
     displayWarning(&"Error getting package info for {name}: {e.msg}", HighPriority)
 
-  if not (options.satResult.pass == satNimSelection and options.satResult.declarativeParseFailed):
-    #Dont save tagged versions if the declarative parser failed as this could cache the incorrect versions.
-    #its suboptimal in the sense that next packages after failure wont be saved in the first past but there is a guarantee that there is a second pass in the case
-    #the declarative parser fails so they will be saved then.
-    saveTaggedVersions(name, result, options)
+  saveTaggedVersions(name, result, options)
 
   # Clean up tempDir if it was created
   if tempDirCreated:
@@ -954,11 +927,7 @@ proc getPackageMinimalVersionsFromRepoAsync*(repoDir: string, pkg: PkgTuple, ver
         # Fall back to checkout + VM parser if declarative parsing failed
         if not parsed:
           discard await doCheckoutAsync(downloadMethod, tempDir, tag, versionDiscoveryOptions)
-          let nimbleFile = findNimbleFile(tempDir, true, options, warn = false)
-          if options.satResult.pass in {satNimSelection}:
-            result.addUnique getPkgInfoFromDirWithDeclarativeParser(tempDir, options, nimBin).getMinimalInfo(options)
-          else:
-            result.addUnique getMinimalInfo(nimbleFile, options, nimBin)
+          result.addUnique getPkgInfoFromDirWithDeclarativeParser(tempDir, options, nimBin).getMinimalInfo(options)
           #here we copy the directory to its own folder so we have it cached for future usage
           let downloadInfo = getPackageDownloadInfo((name, tagVersion.toVersionRange()), options)
           if not dirExists(downloadInfo.downloadDir):
@@ -971,21 +940,14 @@ proc getPackageMinimalVersionsFromRepoAsync*(repoDir: string, pkg: PkgTuple, ver
 
     # Add HEAD version last (tagged releases take precedence if same version exists)
     try:
-      if options.satResult.pass in {satNimSelection}:
-        result.addUnique getPkgInfoFromDirWithDeclarativeParser(repoDir, options, nimBin).getMinimalInfo(options)
-      else:
-        result.addUnique getPkgInfo(repoDir, options, nimBin).getMinimalInfo(options)
+      result.addUnique getPkgInfoFromDirWithDeclarativeParser(repoDir, options, nimBin).getMinimalInfo(options)
     except CatchableError as e:
       displayWarning(&"Error getting package info for {name}: {e.msg}", HighPriority)
 
-    if not (options.satResult.pass == satNimSelection and options.satResult.declarativeParseFailed):
-      #Dont save tagged versions if the declarative parser failed as this could cache the incorrect versions.
-      #its suboptimal in the sense that next packages after failure wont be saved in the first past but there is a guarantee that there is a second pass in the case
-      #the declarative parser fails so they will be saved then.
-      try:
-        saveTaggedVersions(name, result, options)
-      except CatchableError as e:
-        displayWarning(&"Error saving tagged versions for {name}: {e.msg}", LowPriority)
+    try:
+      saveTaggedVersions(name, result, options)
+    except CatchableError as e:
+      displayWarning(&"Error saving tagged versions for {name}: {e.msg}", LowPriority)
   finally:
     try:
       removeDir(tempDir)
@@ -1162,10 +1124,7 @@ proc downloadPkgFromUrlAsync*(pv: PkgTuple, options: Options, doPrompt = false,
 proc downloadPkInfoForPvAsync*(pv: PkgTuple, options: Options, doPrompt = false, nimBin: string): Future[PackageInfo] {.async.} =
   ## Async version of downloadPkInfoForPv that downloads and gets package info.
   let downloadRes = await downloadPkgFromUrlAsync(pv, options, doPrompt, nimBin)
-  if options.satResult.pass in {satNimSelection}:
-    return getPkgInfoFromDirWithDeclarativeParser(downloadRes[0].dir, options, nimBin)
-  else:
-    return getPkgInfo(downloadRes[0].dir, options, nimBin, forValidation = false, onlyMinimalInfo = false)
+  return getPkgInfoFromDirWithDeclarativeParser(downloadRes[0].dir, options, nimBin)
 
 var downloadCache {.threadvar.}: Table[string, Future[seq[PackageMinimalInfo]]]
 

src/nimblepkg/packageinfotypes.nim

@@ -110,8 +110,8 @@ type
 
   SATPass* = enum
     satNone
-    satLockFile #From a lock file. SAT is not ran.
-    satNimSelection #Declarative parser preferred. Fallback to VM parser if needed via bootstrapped nim
+    satLockFile  # From a lock file. SAT is not run.
+    satSolving   # SAT solver active, declarative parser with atomic VM fallback
     satDone
 
   NimResolved* = object
@@ -143,7 +143,6 @@ type
     pass*: SATPass
     installedPkgs*: seq[PackageInfo] #Packages installed in the current pass
     buildPkgs*: seq[PackageInfo] #Packages that were built in the current pass
-    declarativeParseFailed*: bool
     nimResolved*: NimResolved
     bootstrapNim*: BootstrapNim #The nim that we are going to use if we dont have a nim resolved yet and the declarative parser failed. Notice this is required to Atomic Parser fallback (not implemented)
     normalizedRequirements*: Table[string, string] #normalized -> old. Some packages are not published as nimble packages, we keep the url for installation.
@@ -178,7 +177,7 @@ proc getGloballyActiveFeatures*(): seq[string] =
 
 proc initSATResult*(pass: SATPass): SATResult =
   SATResult(pkgsToInstall: @[], solvedPkgs: @[], output: "", pkgs: initHashSet[PackageInfo](),
-    pass: pass, installedPkgs: @[], declarativeParseFailed: false,
+    pass: pass, installedPkgs: @[],
     normalizedRequirements: initTable[string, string](),
     gitErrors: @[],
     lockFileVcsRevisions: initTable[string, Sha1Hash]()

src/nimblepkg/vnext.nim

@@ -38,7 +38,7 @@ proc debugSATResult*(options: Options, calledFrom: string) =
   else:
     echo "No Nim selected"
   echo color, "Bootstrap Nim: ", reset, "isSet: ", satResult.bootstrapNim.nimResolved.pkg.isSome, " version: ", satResult.bootstrapNim.nimResolved.version
-  echo color, "Declarative parser failed: ", reset, satResult.declarativeParseFailed
+
   let pkgsWithErrors = satResult.pkgs.toSeq.filterIt(it.declarativeParserErrors.len > 0)
   if pkgsWithErrors.len > 0:
     echo color, "Declarative parser errors: ", reset
@@ -639,9 +639,6 @@ proc resolveAndConfigureNim*(rootPackage: PackageInfo, pkgList: seq[PackageInfo]
   # setBootstrapNim(systemNimPkg, pkgListDecl, options)
   #TODO NEXT PR
   #At this point, if we failed before to parse the pkglist. We need to reparse with the bootsrapped nim as we may have missed some deps.
-  # if options.satResult.declarativeParseFailed:
-  #   echo "FAILED TO PARSE THE PKGLIST, REPARSING WITH BOOTSTRAPPED NIM"
-  #   debugSatResult(options, "resolveAndConfigureNim")
 
 
   var resolvedNim = resolveNim(rootPackage, pkgListDecl, systemNimPkg, options)