From 42ef358459289b02269641669c2462c92acfedbd Mon Sep 17 00:00:00 2001
From: Dominik Picheta <dominikpicheta@gmail.com>
Date: Wed, 30 Dec 2015 19:43:20 +0000
Subject: [PATCH] Various optimisations to PackageInfo reading and bug fixes.

* PackageInfo objects are now cached because NimScript evaluation is expensive.
* before/after hooks now return `true` by default.
* Bugfix: when hooks weren't found Nimble would still think that a hook told it
  to skip an action.
* PackageInfo now includes info about which hooks are defined to prevent
  unnecessary execution of those hooks.
* Probably more.
---
 nimble.nimble                      |  7 -------
 src/nimble.nim                     | 13 +++++++++----
 src/nimblepkg/nimbletypes.nim      |  4 ++++
 src/nimblepkg/nimscriptapi.nim     |  3 ++-
 src/nimblepkg/nimscriptsupport.nim | 20 +++++++++++++++-----
 src/nimblepkg/options.nim          |  6 ++++--
 src/nimblepkg/packageinfo.nim      |  2 ++
 src/nimblepkg/packageparser.nim    | 12 +++++++++++-
 8 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/nimble.nimble b/nimble.nimble
index a7b6e75..3170eb2 100644
--- a/nimble.nimble
+++ b/nimble.nimble
@@ -12,13 +12,6 @@ srcDir = "src"
 
 requires "nim >= 0.11.2"
 
-before tasks:
-  echo("About to list tasks!")
-  return true
-
-after tasks:
-  echo("Listed tasks!")
-
 task tests, "Run the Nimble tester!":
   withDir "tests":
     exec "nim c -r tester"
\ No newline at end of file
diff --git a/src/nimble.nim b/src/nimble.nim
index e501b05..334e1c2 100644
--- a/src/nimble.nim
+++ b/src/nimble.nim
@@ -860,11 +860,16 @@ proc execHook(options: Options, before: bool): bool =
   ## Returns whether to continue.
   result = true
   let nimbleFile = findNimbleFile(getCurrentDir(), true)
-  # TODO: Optimise this, there are two (three?) calls to readPackageInfo.
-  if nimbleFile.isNimScript(options):
-    let actionName = ($options.action.typ)[6 .. ^1]
+  # PackageInfos are cached so we can read them as many times as we want.
+  let pkgInfo = readPackageInfo(nimbleFile, options)
+  let actionName = ($options.action.typ)[6 .. ^1]
+  let hookExists =
+    if before: actionName.normalize in pkgInfo.preHooks
+    else: actionName.normalize in pkgInfo.postHooks
+  if pkgInfo.isNimScript and hookExists:
     let res = execHook(nimbleFile, actionName, before, options)
-    result = res.retVal
+    if res.success:
+      result = res.retVal
 
 proc doAction(options: Options) =
   if not existsDir(options.getNimbleDir()):
diff --git a/src/nimblepkg/nimbletypes.nim b/src/nimblepkg/nimbletypes.nim
index aa6d35b..ae6ec47 100644
--- a/src/nimblepkg/nimbletypes.nim
+++ b/src/nimblepkg/nimbletypes.nim
@@ -3,6 +3,8 @@
 # Various miscellaneous common types reside here, to avoid problems with
 # recursive imports
 
+import sets
+
 import version
 export version.NimbleError
 
@@ -14,6 +16,8 @@ type
     isNimScript*: bool ## Determines if this pkg info was read from a nims file
     isMinimal*: bool
     isInstalled*: bool ## Determines if the pkg this info belongs to is installed
+    postHooks*: HashSet[string] ## Useful to know so that Nimble doesn't execHook unnecessarily
+    preHooks*: HashSet[string]
     name*: string
     version*: string
     author*: string
diff --git a/src/nimblepkg/nimscriptapi.nim b/src/nimblepkg/nimscriptapi.nim
index af5171d..b264a07 100644
--- a/src/nimblepkg/nimscriptapi.nim
+++ b/src/nimblepkg/nimscriptapi.nim
@@ -27,12 +27,13 @@ proc requires*(deps: varargs[string]) =
 template before*(action: untyped, body: untyped): untyped =
   ## Defines a block of code which is evaluated before ``action`` is executed.
   proc `action Before`*(): bool =
-    result = false
+    result = true
     body
 
 template after*(action: untyped, body: untyped): untyped =
   ## Defines a block of code which is evaluated after ``action`` is executed.
   proc `action After`*(): bool =
+    result = true
     body
 
 template builtin = discard
diff --git a/src/nimblepkg/nimscriptsupport.nim b/src/nimblepkg/nimscriptsupport.nim
index 4622c62..d62a208 100644
--- a/src/nimblepkg/nimscriptsupport.nim
+++ b/src/nimblepkg/nimscriptsupport.nim
@@ -12,13 +12,11 @@ import
 
 from compiler/scriptconfig import setupVM
 from compiler/idents import getIdent
-from compiler/astalgo import strTableGet, `$`
+from compiler/astalgo import strTableGet
 import compiler/options as compiler_options
 
-import compiler/renderer
-
 import nimbletypes, version, options, packageinfo
-import os, strutils, strtabs, times, osproc
+import os, strutils, strtabs, times, osproc, sets
 
 type
   ExecutionResult*[T] = object
@@ -277,7 +275,9 @@ proc readPackageInfoFromNims*(scriptName: string, options: Options,
 
   # Extract all the necessary fields populated by the nimscript file.
   proc getSym(thisModule: PSym, ident: string): PSym =
-    thisModule.tab.strTableGet(getIdent(ident))
+    result = thisModule.tab.strTableGet(getIdent(ident))
+    if result.isNil:
+      raise newException(NimbleError, "Ident not found: " & ident)
 
   template trivialField(field) =
     result.field = getGlobal(getSym(thisModule, astToStr field))
@@ -322,6 +322,15 @@ proc readPackageInfoFromNims*(scriptName: string, options: Options,
   else:
     result.backend = backend.toLower()
 
+  # Grab all the global procs
+  for i in thisModule.tab.data:
+    if not i.isNil():
+      let name = i.name.s.normalize()
+      if name.endsWith("before"):
+        result.preHooks.incl(name[0 .. ^7])
+      if name.endsWith("after"):
+        result.postHooks.incl(name[0 .. ^6])
+
   cleanup()
 
 proc execTask*(scriptName, taskName: string,
@@ -377,6 +386,7 @@ proc execHook*(scriptName, actionName: string, before: bool,
   if prc.isNil:
     # Procedure not defined in the NimScript module.
     result.success = false
+    cleanup()
     return
   let returnVal = vm.globalCtx.execProc(prc, [])
   case returnVal.kind
diff --git a/src/nimblepkg/options.nim b/src/nimblepkg/options.nim
index 3af5817..7970294 100644
--- a/src/nimblepkg/options.nim
+++ b/src/nimblepkg/options.nim
@@ -1,11 +1,11 @@
 # Copyright (C) Dominik Picheta. All rights reserved.
 # BSD License. Look at license.txt for more info.
 
-import json, strutils, os, parseopt, strtabs, uri
+import json, strutils, os, parseopt, strtabs, uri, tables
 from httpclient import Proxy, newProxy
 
 import nimblepkg/config, nimblepkg/version,
-       nimblepkg/tools
+       nimblepkg/tools, nimblepkg/nimbletypes
 
 type
   Options* = object
@@ -15,6 +15,7 @@ type
     action*: Action
     config*: Config
     nimbleData*: JsonNode ## Nimbledata.json
+    pkgInfoCache*: TableRef[string, PackageInfo]
 
   ActionType* = enum
     actionNil, actionUpdate, actionInit, actionDump, actionPublish,
@@ -258,6 +259,7 @@ proc parseFlag*(flag, val: string, result: var Options) =
 
 proc initOptions*(): Options =
   result.action.typ = actionNil
+  result.pkgInfoCache = newTable[string, PackageInfo]()
 
 proc parseMisc(): Options =
   result = initOptions()
diff --git a/src/nimblepkg/packageinfo.nim b/src/nimblepkg/packageinfo.nim
index c718fe7..304398b 100644
--- a/src/nimblepkg/packageinfo.nim
+++ b/src/nimblepkg/packageinfo.nim
@@ -22,6 +22,8 @@ type
 
 proc initPackageInfo*(path: string): PackageInfo =
   result.mypath = path
+  result.preHooks.init()
+  result.postHooks.init()
   # reasonable default:
   result.name = path.splitFile.name
   result.version = ""
diff --git a/src/nimblepkg/packageparser.nim b/src/nimblepkg/packageparser.nim
index d81122d..ed9f4ac 100644
--- a/src/nimblepkg/packageparser.nim
+++ b/src/nimblepkg/packageparser.nim
@@ -1,6 +1,6 @@
 # Copyright (C) Dominik Picheta. All rights reserved.
 # BSD License. Look at license.txt for more info.
-import parsecfg, json, streams, strutils, parseutils, os
+import parsecfg, json, streams, strutils, parseutils, os, tables
 import version, tools, nimbletypes, nimscriptsupport, options, packageinfo
 
 ## Contains procedures for parsing .nimble files. Moved here from ``packageinfo``
@@ -187,6 +187,15 @@ proc readPackageInfo*(nf: NimbleFile, options: Options,
   ##
   ## When ``onlyMinimalInfo`` is true, only the `name` and `version` fields are
   ## populated. The isNimScript field can also be relied on.
+  ##
+  ## This version uses a cache stored in ``options``, so calling it multiple
+  ## times on the same ``nf`` shouldn't require re-evaluation of the Nimble
+  ## file.
+
+  # Check the cache.
+  if options.pkgInfoCache.hasKey(nf):
+    return options.pkgInfoCache[nf]
+
   result = initPackageInfo(nf)
 
   validatePackageName(nf.splitFile.name)
@@ -221,6 +230,7 @@ proc readPackageInfo*(nf: NimbleFile, options: Options,
         raise newException(NimbleError, msg)
 
   validatePackageInfo(result, nf)
+  options.pkgInfoCache[nf] = result
 
 proc getPkgInfo*(dir: string, options: Options): PackageInfo =
   ## Find the .nimble file in ``dir`` and parses it, returning a PackageInfo.