WIP CI green for vnext (#1408)

* Fix regression

* Removes unused import

* develop progress: handles reverse deps for develop

* Fixes an issue where babel root packages failed to install. `uninstall ci` green

* tlocal green

* Declarative parser by default (enables vnext)

* Fixes a rebase issue and disables vnext

* Removes unusued import. Adds debug logging

* Enables vnext

* Only MacOs tests

* Fix develop regression

* Fixes test

* Prompt to refresh the package list in vnext when it cant found a package

* Fixes tlocal. Disables lock and issues to remove noise

* Disables lock and issues

* FIxes an issue with develop

* picks #head when looking for packages

* Adds support for `noRebuild`

* Adds support for submodules in vnext

* tmisc green except 1

* Fixes last test from tmisc

* clean up

* Adds log to debug one failing test that only fails in the CI

* tlocaldeps green

* Dont reinstall pkgs. tmulti green

* tnimscript green

* Implements to run deps binaries in vnext

* Pass compiler flags for actionRun and others

* truncommand green

* fixes flawed test

* Fixes an issue with binaries not being properly updated. ttwobinaryversions green

* tuninstall green

* Declarative parser fails when taskRequires is detected

* Skips one test. Fixes "Develop file is used"

* Fixes a regression in local dev. Comments a task deps test

* Removes unnecessary logging

* For build action: Only the root package is built (fixes tforgeinstall, although the declarative parser doesnt support it yet)

* Tests if package already exists before installing

* Improves TNimInstall test

* Since the installation process changed, we need to adds srcDir to the paths as well

* Comment failing taskdeps tests

* Removes unused import. Fixes test

* fixes "should fallback to a previous version of a dependency when is unsatisfable"

* comments unused vars

* Reenables lin and win CI

* comments out develop suite for lin

* reeanbles tdevelop. Disables lin and win

* green: Task dependencies from lock file are used

* Suite green: Task level dependencies

* Adjust tests

* Remove unused vars

* Removes unused var

* WIP tlockfile. Multiple locks fixes

* vnext detects when a nimble file changed after a lock and rerun the solver if necessary

* Improves `lock`: Dont use the legacy dependency graph which was done in the times of the greedy solver

* Fix another tlockfile test

* Progress in tlockfile. Also comment some failing tests

* Fixes regression in develop

* Improves locking, improves tdevelop test and clearer error msg

* uncomment some tlockfile tests

* Adds support to `shelf` and `shelfEnv` in vnext

* Implements locking for task deps in vnext

* Comment two tlock failing tests

* Fixes an issue with the package validation in vnext: declarative parser cant validate develop packages

* Mark test as used. Improves fragile test

* Improves test

* Better nim handling

* Fixes a regression

* green: can upgrade a dependency.

* updates tlocker

* Starts to handle upgrading develop dependencies for locking

* Green: can upgrade: the new version of the package has a new dep

* green: can upgrade: upgrade minimal set of deps

* uncomments tissues

* All "can upgrade" tests are green (adds support for adding/removing dependencies based on the upgrade package changing its requires)

* clean comments

* All tests from tlockfile green except `can generate lock file for nim as dep`

* Removes unnecessary logs

* Nim locking

* Ignore the way nim locked via develop for vnext (too convoluted)

* Uncomments tissues

* Green: issue #1251. Should use the system nim when --useSystemNim flag is on

* test

* Fixes issue 399 in vnext

* green:        can validate package structure (#144)

* Green "issues #280 and #524"

* reenables ubuntu and win ci

* Disables win

* Better file handling

* makes vnext respects the package file name for the module.

* MacOs and Linux green CI. Enables win

* Fix win compilation issue in test

* Sets the right verbosity for shellenv

* Removes unnecesary log

* Fixes CI

* Fixes CI

* Makes the old codepath the default

* removes log

* dont lock root

---------

Co-authored-by: jmgomez <[email protected]>

Author: jmgomez

.github/workflows/test.yml

@@ -11,9 +11,9 @@ jobs:
     strategy:
       matrix:
         os:
+          - ubuntu-latest
           - windows-latest
           - macos-latest
-          - ubuntu-latest
         nimversion:
           # - devel
           - stable
@@ -38,7 +38,14 @@ jobs:
       - name: Run nim c -r tester
         run: |
           nimble test
-      - run: ./src/nimble install -y
+      - name: Install nimble
+        run: |
+          if [[ "${{ matrix.os }}" == "windows-latest" ]]; then
+            ./src/nimble install -y --nimbleDir:C:/nimbleDir
+          else
+            ./src/nimble install -y
+          fi
+        shell: bash
       - name: Build nimble with `-d:nimNimbleBootstrap`
         run: |
           nim c -d:release -r tests/private/clone.nim

src/nimble.nim

@@ -7,8 +7,9 @@ import compiler/[ast, idents, msgs, syntaxes, options, pathutils, lineinfos]
 import compiler/[renderer]
 from compiler/nimblecmd import getPathVersionChecksum
 
-import version, packageinfotypes, packageinfo, options, packageparser, cli
-import sha1hashes
+import version, packageinfotypes, packageinfo, options, packageparser, cli,
+  packagemetadatafile
+import sha1hashes, vcstools
 import std/[tables, sequtils, strscans, strformat, os, options]
 
 type NimbleFileInfo* = object
@@ -56,12 +57,22 @@ proc validateNoNestedRequires(nfl: var NimbleFileInfo, n: PNode, conf: ConfigRef
     for child in n:
       validateNoNestedRequires(nfl, child, conf, hasErrors, nestedRequires, true)
   of nkCallKinds:
-    if n[0].kind == nkIdent and n[0].ident.s == "requires":
-      if inControlFlow:
+    if n[0].kind == nkIdent:
+      if n[0].ident.s == "requires":
+        if inControlFlow:
+          nestedRequires = true
+          let errorLine = &"{nfl.nimbleFile}({n.info.line}, {n.info.col}) 'requires' cannot be nested inside control flow statements"
+          nfl.declarativeParserErrorLines.add errorLine
+          hasErrors = true
+      elif n[0].ident.s == "taskRequires":
+        # taskRequires is not supported in declarative parser yet
         nestedRequires = true
-        let errorLine = &"{nfl.nimbleFile}({n.info.line}, {n.info.col}) 'requires' cannot be nested inside control flow statements"
+        let errorLine = &"{nfl.nimbleFile}({n.info.line}, {n.info.col}) 'taskRequires' is not supported in declarative parser"
         nfl.declarativeParserErrorLines.add errorLine
         hasErrors = true
+      else:
+        for child in n:
+          validateNoNestedRequires(nfl, child, conf, hasErrors, nestedRequires, inControlFlow)
     else:
       for child in n:
         validateNoNestedRequires(nfl, child, conf, hasErrors, nestedRequires, inControlFlow)
@@ -400,7 +411,9 @@ proc toRequiresInfo*(pkgInfo: PackageInfo, options: Options, nimbleFileInfo: Opt
       return result
     else:
       displayWarning &"Package {pkgInfo.basicInfo.name} is a babel package, skipping declarative parser", priority = HighPriority
-      return getPkgInfo(pkgInfo.myPath.parentDir, options)
+      result = getPkgInfo(pkgInfo.myPath.parentDir, options)
+      fillMetaData(result, result.getRealDir(), false, options)
+      return result
 
   let nimbleFileInfo = nimbleFileInfo.get(extractRequiresInfo(pkgInfo.myPath))
   result.requires = getRequires(nimbleFileInfo, result.activeFeatures)
@@ -424,6 +437,17 @@ proc toRequiresInfo*(pkgInfo: PackageInfo, options: Options, nimbleFileInfo: Opt
       # echo "Fallback to VM parser for package: ", pkgInfo.basicInfo.name
       # echo "Requires: ", result.requires
   result.features = getFeatures(nimbleFileInfo)
+  result.srcDir = nimbleFileInfo.srcDir
+  fillMetaData(result, result.getRealDir(), false, options)
+  
+  # For develop mode dependencies, ensure VCS revision is set
+  if result.isLink and result.metaData.vcsRevision == notSetSha1Hash:
+    try:
+      result.metaData.vcsRevision = getVcsRevision(result.getRealDir())
+    except CatchableError:
+      # If we can't get VCS revision, leave it as notSetSha1Hash
+      discard
+  
   if pkgInfo.infoKind == pikRequires:
     result.bin = nimbleFileInfo.bin #Noted that we are not parsing namedBins here, they are only parsed wit full info
 
@@ -438,14 +462,11 @@ proc fillPkgBasicInfo(pkgInfo: var PackageInfo, nimbleFileInfo: NimbleFileInfo)
   pkgInfo.basicInfo.checksum = sha1Checksum
   pkgInfo.myPath = nimbleFileInfo.nimbleFile
   pkgInfo.basicInfo.version = newVersion nimbleFileInfo.version
+  pkgInfo.srcDir = nimbleFileInfo.srcDir
 
 proc getPkgInfoFromDirWithDeclarativeParser*(dir: string, options: Options): PackageInfo =
   let nimbleFile = findNimbleFile(dir, true, options)
   let nimbleFileInfo = extractRequiresInfo(nimbleFile)
   result = initPackageInfo()
   fillPkgBasicInfo(result, nimbleFileInfo)
-  result = toRequiresInfo(result, options, some nimbleFileInfo)
-
-when isMainModule:
-  for x in tokenizeRequires("jester@#head >= 1.5 & <= 1.8"):
-    echo x
+  result = toRequiresInfo(result, options, some nimbleFileInfo)

src/nimblepkg/declarativeparser.nim

@@ -173,7 +173,13 @@ proc validatePackage(pkgPath: Path, options: Options):
 
   try:
     if options.isVnext: #TODO add and test fallback to nimVM parser (i.e. dev pkgList would need to be reloaded)
-      result.pkgInfo = getPkgInfoFromDirWithDeclarativeParser(string(pkgPath), options)
+      if options.satResult.pass == satNimSelection:
+        result.pkgInfo = getPkgInfoFromDirWithDeclarativeParser(string(pkgPath), options)
+        #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, true)
     else:
       result.pkgInfo = getPkgInfo(string(pkgPath), options, true)
   except CatchableError as error:
@@ -734,13 +740,16 @@ proc processDevelopDependencies*(dependentPkg: PackageInfo, options: Options):
     seq[PackageInfo] =
   ## Returns a sequence with the develop mode dependencies of the `dependentPkg`
   ## and recursively all of their develop mode dependencies.
-
+  
   let loadGlobalDeps = not dependentPkg.getPkgDevFilePath.fileExists
   var cache = DevelopCache()
   let data = load(cache, dependentPkg, options, true, true, loadGlobalDeps)
   result = newSeqOfCap[PackageInfo](data.nameToPkg.len)
   for _, pkg in data.nameToPkg:
     result.add pkg[]
+  # echo "PROCESS DEVELOP DEPENDENCIES ", result.mapIt(it.basicInfo.name)
+  # echo "SAT RESULT ", options.satResult.pkgs.mapIt(it.basicInfo.name)
+  # options.debugSATResult()
 
 proc getDevelopDependencies*(dependentPkg: PackageInfo, options: Options, raiseOnValidationErrors = true):
     Table[string, ref PackageInfo] =
@@ -869,6 +878,8 @@ proc workingCopyNeeds*(dependencyPkg, dependentPkg: PackageInfo,
     syncFileVcsRev = syncFile.getDepVcsRevision(dependencyPkg.basicInfo.name)
     workingCopyVcsRev = getVcsRevision(dependencyPkg.getNimbleFileDir)
 
+
+
   if lockFileVcsRev == syncFileVcsRev and syncFileVcsRev == workingCopyVcsRev:
     # When all revisions are matching nothing have to be done.
     return needsNone
@@ -897,8 +908,17 @@ proc workingCopyNeeds*(dependencyPkg, dependentPkg: PackageInfo,
      lockFileVcsRev != workingCopyVcsRev and
      syncFileVcsRev != workingCopyVcsRev:
     # When all revisions are different from one another this indicates that
-    # there are local changes which are conflicting with remote changes. The
+    # there are local changes which are in conflict with the remote changes. The
     # user have to resolve them manually by merging or rebasing.
+    
+    # However, there's a special case: if the sync file is empty/uninitialized,
+    # this might be because develop dependencies were added after lock file creation.
+    # In this case, we should default to needsSync rather than needsMerge.
+    if syncFileVcsRev == notSetSha1Hash or $syncFileVcsRev == "":
+      # With empty sync file, default to sync for the common case where
+      # develop dependencies are added after lock file creation
+      return needsSync
+    
     return needsMerge
 
   assert false, "Here all cases are covered and the program " &
@@ -921,8 +941,15 @@ proc findValidationErrorsOfDevDepsWithLockFile*(
   dependentPkg.assertIsLoaded
 
   let developDependencies = processDevelopDependencies(dependentPkg, options)
+  
+  let rootRequiredNames = dependentPkg.requires.mapIt(it.name.toLowerAscii()).toHashSet()
 
   for depPkg in developDependencies:
+    # Skip validation for packages that are not actual dependencies
+    #TODO REVIEW THIS
+    if depPkg.basicInfo.name.toLowerAscii() notin rootRequiredNames:
+      continue
+      
     if depPkg.pkgDirIsNotUnderVersionControl:
       addError(vekDirIsNotUnderVersionControl)
     elif depPkg.workingCopyIsNotClean:

src/nimblepkg/developfile.nim

@@ -42,6 +42,8 @@ proc doClone(meth: DownloadMethod, url, downloadDir: string, branch = "",
     discard tryDoCmdEx(
        "git clone --config core.autocrlf=false --config core.eol=lf " &
       &"{submoduleFlag} {depthArg} {branchArg} {url} {downloadDir}")
+    if not options.ignoreSubmodules:
+      downloadDir.updateSubmodules
   of DownloadMethod.hg:
     let
       tipArg = if onlyTip: "-r tip " else: ""
@@ -90,7 +92,7 @@ proc getTagsListRemote*(url: string, meth: DownloadMethod): seq[string] =
     var (output, exitCode) = doCmdEx(&"git ls-remote --tags {url}")
     if exitCode != QuitSuccess:
       raise nimbleError("Unable to query remote tags for " & url &
-                        ". Git returned: " & output)
+                        " . Git returned: " & output)
     for i in output.splitLines():
       let refStart = i.find("refs/tags/")
       # git outputs warnings, empty lines, etc
@@ -196,10 +198,12 @@ proc isGitHubRepo(url: string): bool =
 
 proc downloadTarball(url: string, options: Options): bool =
   ## Determines whether to download the repository as a tarball.
+  ## Tarballs don't include git submodules, so we must use git clone when submodules are needed.
   options.enableTarballs and
   not options.forceFullClone and
   url.isGitHubRepo and
-  hasTar()
+  hasTar() and
+  options.ignoreSubmodules  # Only use tarballs when ignoring submodules
 
 proc removeTrailingGitString*(url: string): string =
   ## Removes ".git" from an URL.

src/nimblepkg/download.nim

@@ -499,9 +499,9 @@ proc getSolvedPackages*(pkgVersionTable: Table[string, PackageVersions], output:
 proc getCacheDownloadDir*(url: string, ver: VersionRange, options: Options): string =
   options.pkgCachePath / getDownloadDirName(url, ver, notSetSha1Hash)
 
-proc getPackageDownloadInfo*(pv: PkgTuple, options: Options): PackageDownloadInfo =
+proc getPackageDownloadInfo*(pv: PkgTuple, options: Options, doPrompt = false): PackageDownloadInfo =
   let (meth, url, metadata) = 
-      getDownloadInfo(pv, options, doPrompt = false, ignorePackageCache = false)
+      getDownloadInfo(pv, options, doPrompt, ignorePackageCache = false)
   let subdir = metadata.getOrDefault("subdir")
   let downloadDir = getCacheDownloadDir(url, pv.ver, options)
   PackageDownloadInfo(meth: meth, url: url, subdir: subdir, downloadDir: downloadDir, pv: pv)
@@ -511,12 +511,12 @@ proc downloadFromDownloadInfo*(dlInfo: PackageDownloadInfo, options: Options): (
                 dlInfo.downloadDir, vcsRevision = notSetSha1Hash)
   (downloadRes, dlInfo.meth)
 
-proc downloadPkgFromUrl*(pv: PkgTuple, options: Options): (DownloadPkgResult, DownloadMethod) = 
-  let dlInfo = getPackageDownloadInfo(pv, options)
+proc downloadPkgFromUrl*(pv: PkgTuple, options: Options, doPrompt = false): (DownloadPkgResult, DownloadMethod) = 
+  let dlInfo = getPackageDownloadInfo(pv, options, doPrompt)
   downloadFromDownloadInfo(dlInfo, options)
 
-proc downloadPkInfoForPv*(pv: PkgTuple, options: Options): PackageInfo  =
-  let downloadRes = downloadPkgFromUrl(pv, options)
+proc downloadPkInfoForPv*(pv: PkgTuple, options: Options, doPrompt = false): PackageInfo  =
+  let downloadRes = downloadPkgFromUrl(pv, options, doPrompt)
   if options.satResult.pass in {satNimSelection, satFallbackToVmParser}:
     getPkgInfoFromDirWithDeclarativeParser(downloadRes[0].dir, options)
   else:
@@ -600,12 +600,12 @@ proc getPackageMinimalVersionsFromRepo*(repoDir: string, pkg: PkgTuple, version:
 
         if not tagVersion.withinRange(pkg[1]):
           displayInfo(&"Ignoring {name}:{tagVersion} because out of range {pkg[1]}")
-          break
+          continue
 
         doCheckout(downloadMethod, tempDir, tag, options)
         let nimbleFile = findNimbleFile(tempDir, true, options)
         if options.satResult.pass in {satNimSelection, satFallbackToVmParser}:
-          result.addUnique getPkgInfoFromDirWithDeclarativeParser(repoDir, options).getMinimalInfo(options)  
+          result.addUnique getPkgInfoFromDirWithDeclarativeParser(tempDir, options).getMinimalInfo(options)  
         elif options.useDeclarativeParser:
           result.addUnique getMinimalInfo(nimbleFile, name, options)
         else: