Fold nimble upgrade into nimble lock --refresh

Author: jmgomez

src/nimble.nim

@@ -1490,7 +1490,7 @@ proc lock(options: var Options, nimBin: Option[string]) =
   # upgrading the libraries never moves the compiler. Captured here, BEFORE the
   # lock is rewritten below; written back in the dependency loop. 
   var prevNimDep = none(LockFileDep)
-  if options.action.typ == actionUpgrade and options.action.packages.len == 0 and lockExists:
+  if options.isUpgrade and options.action.packages.len == 0 and lockExists:
     for name, dep in currentLockFile.getLockedDependencies.lockedDepsFor(options):
       if name.isNim:
         prevNimDep = some(dep)
@@ -2450,6 +2450,11 @@ proc doAction(options: var Options, nimBinParam: Option[string]) {.instrument.}
   of actionRun:
     runAction(options, nimBin)
   of actionUpgrade:
+    displayWarning("`nimble upgrade` is deprecated and will be removed in a " &
+                   "future release. Use `nimble lock --refresh` instead.",
+                   HighPriority)
+    # `upgrade` is an alias of `lock --refresh`; `forceFetch` is set at parse
+    # time (see parseCommand) so the solver fetches the remotes, not the cache.
     lock(options, nimBin)
   of actionCompile, actionDoc:
     var pkgInfo = getPkgInfo(getCurrentDir(), options, nimBin = nimBin)

src/nimblepkg/nimblesat.nim

@@ -1191,7 +1191,7 @@ proc debugSATResult*(options: Options, calledFrom: string) =
   echo color, "Root requires: ", reset, satResult.rootPackage.requires.mapIt(it.name & " " & $it.ver)
   echo color, "Solved packages: ", reset, satResult.solvedPkgs.mapIt(it.pkgName & " " & $it.version & " " & $it.deps.mapIt(it.pkgName))
   echo color, "Solution as Packages Info: ", reset, satResult.pkgs.mapIt(it.basicInfo.name & " " & $it.basicInfo.version)
-  if options.action.typ == actionUpgrade:
+  if options.isUpgrade:
     echo color, "Upgrade versions: ", reset, options.action.packages.mapIt(it.name & " " & $it.ver)
     echo color, "RESULT REVISIONS ", reset, satResult.pkgs.mapIt(it.basicInfo.name & " " & $it.metaData.vcsRevision)
     echo color, "PKG LIST REVISIONS ", reset, satResult.pkgList.mapIt(it.basicInfo.name & " " & $it.metaData.vcsRevision)
@@ -1267,7 +1267,7 @@ proc solveLockFileDeps*(satResult: var SATResult, pkgList: seq[PackageInfo], opt
   # in the shouldSolve check. When upgrading, changed requirements for the
   # upgraded packages are expected and should not trigger a full re-solve.
   var upgradePkgNames: seq[string]
-  if options.action.typ == actionUpgrade:
+  if options.isUpgrade:
     for pkg in options.action.packages:
       upgradePkgNames.add(pkg.name.resolveAlias(options).toLowerAscii())
   for current in currentRequires:
@@ -1294,7 +1294,7 @@ proc solveLockFileDeps*(satResult: var SATResult, pkgList: seq[PackageInfo], opt
   # named packages and keeps the rest locked, which is the wrong model here, so force
   # the full fresh solve. (The install-preferring local solve in solvePackages is
   # already skipped for actionUpgrade, so this resolves to newest.)
-  if options.action.typ == actionUpgrade and options.action.packages.len == 0:
+  if options.isUpgrade and options.action.packages.len == 0:
     shouldSolve = true
 
   var pkgListDecl: seq[PackageInfo]
@@ -1324,7 +1324,7 @@ proc solveLockFileDeps*(satResult: var SATResult, pkgList: seq[PackageInfo], opt
       displayError(satResult.output)
       raise resolutionFailureError(
         "Couldn't find a solution for the packages.")
-  elif options.action.typ == actionUpgrade:
+  elif options.isUpgrade:
     #[
     Retrocompatibility (goes against SAT in some edge cases)
     When upgrading dep1: Only dep1 should change, dep2 should stay at it is
@@ -1335,24 +1335,36 @@ proc solveLockFileDeps*(satResult: var SATResult, pkgList: seq[PackageInfo], opt
       # Populate requirements from lock file dependencies to preserve them
       let requirements = dep.dependencies.mapIt((name: it, ver: VersionRange(kind: verAny)))
       let solvedPkg = SolvedPackage(pkgName: name, version: dep.version, requirements: requirements)
-      if options.action.typ == actionUpgrade:
+      if options.isUpgrade:
         if solvedPkg.pkgName in satResult.solvedPkgs.mapIt(it.pkgName):
           satResult.solvedPkgs = satResult.solvedPkgs.filterIt(it.pkgName != name)
           satResult.pkgs = satResult.pkgs.toSeq.filterIt(it.basicInfo.name != name).toHashSet()
-        var addedUpgradePkg = false
+        var
+          addedUpgradePkg = false
+          versionComesFromSolve = false
         for upgradePkg in options.action.packages:
           if upgradePkg.name == name:
             if upgradePkg.ver.kind == verSpecial:
               satResult.pkgsToInstall.add((name, upgradePkg.ver.spe))
-            # For verAny (no version specified), the temp SAT solve below
-            # will determine the correct version to install.
+            else:
+              # For verAny (no version specified), the temp SAT solve below
+              # will determine the correct version to install.
+              versionComesFromSolve = true
             addedUpgradePkg = true
         if not addedUpgradePkg:
           for pkg in pkgListDecl.toHashSet():
             if pkg.basicInfo.name == name and pkg.basicInfo.version == dep.version and pkg.metaData.vcsRevision == dep.vcsRevision:
               satResult.pkgs.incl(pkg)
               break
-        satResult.solvedPkgs.add(solvedPkg)
+        # Carry the locked version over, EXCEPT when the temp solve below is the
+        # thing that picks the version. Re-adding it in that case pins the old
+        # one: the solve computes the new version, but the merge that follows
+        # skips any name already in solvedPkgs, so the stale entry wins and the
+        # named package silently never moves. A package pinned to an explicit
+        # special version still needs its entry - that path installs from
+        # pkgsToInstall above and the solve has nothing to contribute.
+        if not versionComesFromSolve:
+          satResult.solvedPkgs.add(solvedPkg)
       var pkgListDecl = pkgListDecl
       for upgradePkg in options.action.packages:
         for pkg in pkgList:

src/nimblepkg/options.nim

@@ -250,8 +250,15 @@ Commands:
                [--ini, --json]    Selects the output format (the default is --ini). Only applicable to package information.
                [--collect]        Collects all the packages in the dependency tree of the given package and shows the result in the console.
                [--solve]          Solves the dependency tree of the given package and shows the result in the console.
-  lock                            Generates or updates a package lock file.
-  upgrade      [pkgname, ...]     Upgrades a list of packages in the lock file.
+  lock         [pkgname, ...]     Generates or updates a package lock file.
+                                  Every pin is kept unless the package's
+                                  requirements changed. Naming packages relocks
+                                  those and leaves the rest of the file pinned.
+       [--refresh]                Resolve against the package repositories
+                                  instead of the cached version information, so
+                                  newly published versions are picked up. With
+                                  no package named, relocks everything.
+  upgrade      [pkgname, ...]     Deprecated alias of `lock --refresh`.
   deps                            Outputs dependencies for current package.
                [--tree]           Outputs dependency tree.
                [--inverted]       Outputs inverted (reversed) dependency tree.
@@ -484,6 +491,24 @@ proc nim*(options: Options): string =
 proc getNimbleDir*(options: Options): string =
   return options.nimbleDir
 
+proc isUpgrade*(options: Options): bool =
+  ## Whether this run should relock rather than keep what the lock file pins.
+  ##
+  ## Two things ask `lock` to move a pin
+  ##
+  ## * naming packages - `nimble lock foo` relocks `foo` and leaves the rest of
+  ##   the lock file alone.
+  ## * `--refresh` - go look at the remotes first, instead of resolving against
+  ##   the tagged versions cache. With no packages named that means relocking
+  ##   everything to the newest available.
+  ##
+  ## So `nimble lock foo --refresh` fetches and then relocks just `foo`, and
+  ## plain `nimble lock` keeps every pin. `nimble upgrade` is the deprecated
+  ## spelling of `lock --refresh`.
+  options.action.typ == actionUpgrade or
+  (options.action.typ == actionLock and
+   (options.forceFetch or options.action.packages.len > 0))
+
 proc getPkgsDir*(options: Options): string =
   options.getNimbleDir() / nimblePackagesDirName
 
@@ -645,6 +670,13 @@ proc setNimbleDir*(options: var Options) =
 proc parseCommand*(key: string, result: var Options) =
   result.action = Action(typ: parseActionType(key))
   initAction(result, key)
+  # `nimble upgrade` is a deprecated alias of `nimble lock --refresh`. Set
+  # forceFetch here - at parse time, before the SAT solver runs - so version
+  # discovery goes to the remotes (not the tagged-versions cache) for the whole
+  # run. Setting it later, in the action dispatch, would be too late: the
+  # solver would already have resolved dependencies from the cache.
+  if result.action.typ == actionUpgrade:
+    result.forceFetch = true
 
 
 proc getRequiredNimVersion*(pkgInfo: PackageInfo): VersionRange =
@@ -666,7 +698,8 @@ proc parseArgument*(key: string, result: var Options) =
   case result.action.typ
   of actionNil:
     assert false
-  of actionInstall, actionPath, actionDevelop, actionUninstall, actionUpgrade, actionAdd:
+  of actionInstall, actionPath, actionDevelop, actionUninstall, actionUpgrade,
+     actionLock, actionAdd:
     # Parse pkg@verRange or git@github.com:nim-lang/nimble.git
     # First trim whitespace from the key
     let trimmedKey = key.strip()

tests/tnimblerefresh.nim

@@ -9,6 +9,7 @@ import testscommon
 from nimblepkg/common import cd, cdNewDir
 from nimblepkg/tools import tryDoCmdEx
 from nimblepkg/packageinfotypes import DownloadMethod
+from nimblepkg/options import defaultLockFileName
 
 suite "nimble refresh":
   test "can refresh with default urls":
@@ -142,34 +143,41 @@ license       = "MIT"
     tryDoCmdEx("git add .")
     tryDoCmdEx("git commit -am " & msg.quoteShell)
 
-  proc writeDepVersion(version: string) =
-    ## Writes dep1.nimble at `version` in the cwd and tags it.
-    writeFile("dep1.nimble", nimbleFileTemplate % version)
+  proc writeDepVersion(version: string, name = "dep1") =
+    ## Writes <name>.nimble at `version` in the cwd and tags it.
+    writeFile(&"{name}.nimble", nimbleFileTemplate % version)
     commitAll(version)
     tryDoCmdEx(&"git tag v{version}")
 
-  proc initDepOrigin(versions: seq[string]) =
-    cdNewDir depOriginPath:
+  proc initDepOrigin(versions: seq[string], name = "dep1") =
+    cdNewDir originsDirPath / name:
       initRepo()
       for v in versions:
-        writeDepVersion(v)
+        writeDepVersion(v, name)
 
-  proc addDepVersion(version: string) =
-    cd depOriginPath:
-      writeDepVersion(version)
+  proc addDepVersion(version: string, name = "dep1") =
+    cd originsDirPath / name:
+      writeDepVersion(version, name)
 
-  proc initMainPkg(requirement: string) =
+  proc initMainPkg(requirement: string, underVcs = false) =
     createDir mainPkgPath
     cd mainPkgPath:
       writeFile("main.nimble",
         (nimbleFileTemplate % "0.1.0") & &"requires \"{requirement}\"\n")
+      if underVcs:
+        # Rewriting an existing lock file requires the project dir to be under
+        # version control (creating one does not).
+        initRepo()
+        commitAll("main")
 
-  proc writePkgListFile() =
+  proc writePkgListFile(names = @["dep1"]) =
     createDir tempDir
-    let record = PackagesListFileRecord(
-      name: "dep1", url: depOriginPath, `method`: DownloadMethod.git,
-      tags: @["test"], description: "A test package.", license: "MIT")
-    writeFile(pkgListFilePath, (%(@[record])).pretty)
+    var records: seq[PackagesListFileRecord]
+    for name in names:
+      records.add PackagesListFileRecord(
+        name: name, url: originsDirPath / name, `method`: DownloadMethod.git,
+        tags: @["test"], description: "A test package.", license: "MIT")
+    writeFile(pkgListFilePath, (%records).pretty)
 
   template withCleanDirs(body: untyped) =
     removeDir tempDir
@@ -182,15 +190,46 @@ license       = "MIT"
   template withDepProject(requirement: string, body: untyped) =
     ## dep1 origin at 0.1.0, a main package requiring it, and a warm cache
     ## (`nimble install` resolved dep1 once, so tagged_versions.json knows 0.1.0).
+    withDepProject(requirement, false, body)
+
+  template withDepProject(requirement: string, underVcs, body: untyped) =
+    ## As the 2-arg overload, but with the main package's directory under git
+    ## control so the lock file can be rewritten in place.
     withCleanDirs:
       writePkgListFile()
       usePackageListFile pkgListFilePath:
         initDepOrigin(@["0.1.0"])
-        initMainPkg(requirement)
+        initMainPkg(requirement, underVcs)
+        cd mainPkgPath:
+          check execNimbleYes("install").exitCode == QuitSuccess
+        body
+
+  template withTwoDepProject(body: untyped) =
+    ## dep1 and dep2 origins at 0.1.0 and a main package requiring both, under
+    ## git so its lock file can be rewritten. Two deps are what make "only the
+    ## named package moved" observable.
+    withCleanDirs:
+      writePkgListFile(@["dep1", "dep2"])
+      usePackageListFile pkgListFilePath:
+        initDepOrigin(@["0.1.0"], "dep1")
+        initDepOrigin(@["0.1.0"], "dep2")
+        createDir mainPkgPath
         cd mainPkgPath:
+          writeFile("main.nimble", (nimbleFileTemplate % "0.1.0") &
+            "requires \"dep1 >= 0.1.0\"\nrequires \"dep2 >= 0.1.0\"\n")
+          initRepo()
+          commitAll("main")
           check execNimbleYes("install").exitCode == QuitSuccess
         body
 
+  proc lockedVersion(pkg: string): string =
+    ## The version `pkg` is pinned to in the main package's lock file.
+    let lock = parseJson(readFile(mainPkgPath / defaultLockFileName))
+    for name, dep in lock["packages"].pairs:
+      if name.cmpIgnoreCase(pkg) == 0:
+        return dep["version"].getStr
+    return ""
+
   test "refresh makes a newly published tag visible":
     withDepProject("dep1 >= 0.1.0"):
       addDepVersion("0.2.0")   # published after the cache was warmed
@@ -300,3 +339,70 @@ license       = "MIT"
       cd depClonePath:
         let tag = tryDoCmdEx("git describe --tags").strip
         check tag == "v0.1.0"
+
+  test "lock --refresh relocks to a newly published version":
+    withDepProject("dep1 >= 0.1.0", true):
+      cd mainPkgPath:
+        # Lock first so the next publish has an existing pin to keep or move.
+        check execNimbleYes("lock").exitCode == QuitSuccess
+        check (defaultLockFileName.readFile).contains("0.1.0")
+      addDepVersion("0.2.0")   # published after the lock was written
+      cd mainPkgPath:
+        # A plain `lock` keeps its pins; the newly published version is not used.
+        check execNimbleYes("lock").exitCode == QuitSuccess
+        check not (defaultLockFileName.readFile).contains("0.2.0")
+        check (defaultLockFileName.readFile).contains("0.1.0")
+
+        # `lock --refresh` ignores the pins and relocks to the newest.
+        check execNimbleYes("lock", "--refresh").exitCode == QuitSuccess
+        check (defaultLockFileName.readFile).contains("0.2.0")
+        check not (defaultLockFileName.readFile).contains("0.1.0")
+
+  test "upgrade still works and says it is deprecated":
+    withDepProject("dep1 >= 0.1.0", true):
+      cd mainPkgPath:
+        # Lock first so `upgrade` relocks an existing pin rather than creating one.
+        check execNimbleYes("lock").exitCode == QuitSuccess
+        check (defaultLockFileName.readFile).contains("0.1.0")
+      addDepVersion("0.2.0")   # published after the lock was written
+      cd mainPkgPath:
+        let (output, exitCode) = execNimbleYes("upgrade")
+        check exitCode == QuitSuccess
+        # `upgrade` is an alias of `lock --refresh`, so it fetches the remotes
+        # and finds 0.2.0 the same way `lock --refresh` does.
+        check output.contains("`nimble upgrade` is deprecated")
+        check output.contains("nimble lock --refresh")
+        check (defaultLockFileName.readFile).contains("0.2.0")
+        check not (defaultLockFileName.readFile).contains("0.1.0")
+
+  test "lock --refresh pkg relocks only that package":
+    withTwoDepProject:
+      cd mainPkgPath:
+        check execNimbleYes("lock").exitCode == QuitSuccess
+        check lockedVersion("dep1") == "0.1.0"
+        check lockedVersion("dep2") == "0.1.0"
+      addDepVersion("0.2.0", "dep1")
+      addDepVersion("0.2.0", "dep2")
+      cd mainPkgPath:
+        check execNimbleYes("lock", "--refresh", "dep1").exitCode == QuitSuccess
+        # Naming a package scopes the relock to it; dep2 keeps its pin even
+        # though 0.2.0 is available for it too.
+        check lockedVersion("dep1") == "0.2.0"
+        check lockedVersion("dep2") == "0.1.0"
+
+  test "lock pkg relocks from the cache without fetching":
+    withTwoDepProject:
+      cd mainPkgPath:
+        check execNimbleYes("lock").exitCode == QuitSuccess
+      addDepVersion("0.2.0", "dep1")
+      cd mainPkgPath:
+        # Without --refresh nothing is fetched, so the newly published 0.2.0 is
+        # not visible yet and the pin stays put.
+        check execNimbleYes("lock", "dep1").exitCode == QuitSuccess
+        check lockedVersion("dep1") == "0.1.0"
+
+        # Once discovery has seen it, naming the package moves it - and only it.
+        check execNimbleYes("refresh").exitCode == QuitSuccess
+        check execNimbleYes("lock", "dep1").exitCode == QuitSuccess
+        check lockedVersion("dep1") == "0.2.0"
+        check lockedVersion("dep2") == "0.1.0"