Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use walkDirRec for nimble test, output to build/tests #850

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use walkDirRec for nimble test, output to build/tests #850

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93f6332
Use walkDirRec, output to build/tests
Sep 12, 2020
519281a
Remove comment
Sep 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 19 additions & 26 deletions src/nimble.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,8 @@ proc installFromDir(dir: string, requestedVer: VersionRange, options: Options,
# if the build fails then the old package will still be installed.
if pkgInfo.bin.len > 0:
let paths = result.deps.map(dep => dep.getRealDir())
let flags = if options.action.typ in {actionInstall, actionPath, actionUninstall, actionDevelop}:
let flags = if options.action.typ in {actionInstall, actionPath,
actionUninstall, actionDevelop}:
options.action.passNimFlags
else:
@[]
Expand Down Expand Up @@ -332,9 +333,9 @@ proc installFromDir(dir: string, requestedVer: VersionRange, options: Options,
var filesInstalled = initHashSet[string]()
iterInstallFiles(realDir, pkgInfo, options,
proc (file: string) =
createDir(changeRoot(realDir, pkgDestDir, file.splitFile.dir))
let dest = changeRoot(realDir, pkgDestDir, file)
filesInstalled.incl copyFileD(file, dest)
createDir(changeRoot(realDir, pkgDestDir, file.splitFile.dir))
let dest = changeRoot(realDir, pkgDestDir, file)
filesInstalled.incl copyFileD(file, dest)
)

# Copy the .nimble file.
Expand All @@ -356,7 +357,8 @@ proc installFromDir(dir: string, requestedVer: VersionRange, options: Options,
else: bin
if fileExists(pkgDestDir / binDest):
display("Warning:", ("Binary '$1' was already installed from source" &
" directory. Will be overwritten.") % bin, Warning,
" directory. Will be overwritten.") % bin,
Warning,
MediumPriority)

# Copy the binary file.
Expand Down Expand Up @@ -437,7 +439,7 @@ proc install(packages: seq[PkgTuple],
let (meth, url, metadata) = getDownloadInfo(pv, options, doPrompt)
let subdir = metadata.getOrDefault("subdir")
let (downloadDir, downloadVersion) =
downloadPkg(url, pv.ver, meth, subdir, options)
downloadPkg(url, pv.ver, meth, subdir, options)
try:
result = installFromDir(downloadDir, pv.ver, options, url)
except BuildFailed:
Expand Down Expand Up @@ -670,7 +672,7 @@ proc dump(options: Options) =
j[key].add %{
"name": % name,
# we serialize both: `ver` may be more convenient for tooling
# (no parsing needed); while `str` is more familiar.
# (no parsing needed); while `str` is more familiar.
"str": % $ver,
"ver": %* ver,
}
Expand Down Expand Up @@ -1011,11 +1013,11 @@ proc develop(options: Options) =

proc test(options: Options) =
## Executes all tests starting with 't' in the ``tests`` directory.
## Subdirectories are not walked.
var pkgInfo = getPkgInfo(getCurrentDir(), options)

var
files = toSeq(walkDir(getCurrentDir() / "tests"))
files = toSeq(walkDirRec(getCurrentDir() / "tests", {pcFile, pcLinkToFile},
relative = true))
tests, failures: int

if files.len < 1:
Expand All @@ -1025,23 +1027,23 @@ proc test(options: Options) =
if not execHook(options, actionCustom, true):
raise newException(NimbleError, "Pre-hook prevented further execution.")

files.sort((a, b) => cmp(a.path, b.path))
files.sort((a, b) => cmp(a, b))
files = files.map((a) => "tests" / a)

for file in files:
let (_, name, ext) = file.path.splitFile()
if ext == ".nim" and name[0] == 't' and file.kind in {pcFile, pcLinkToFile}:
let (_, name, ext) = file.splitFile()
if ext == ".nim" and name[0] == 't':
var optsCopy = options.briefClone()
optsCopy.action = Action(typ: actionCompile)
optsCopy.action.file = file.path
optsCopy.action.file = file
optsCopy.action.backend = pkgInfo.backend
optsCopy.getCompilationFlags() = options.getCompilationFlags()
# treat run flags as compile for default test task
optsCopy.getCompilationFlags().add(options.action.custRunFlags)
optsCopy.getCompilationFlags().add("-r")
optsCopy.getCompilationFlags().add("--out=" & ("build/" /
file.changeFileExt(ExeExt)))
Comment on lines +1044 to +1045
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, don't we have a setting for this nowadays? @genotrance?

If not, maybe it would be better to put this in tests/build/ or tests/bin/?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what setting?

Related: #787

I think we should instead rely on nim r instead of worrying about yet another directory. That's only in devel though so we should wait a bit until it is in stable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My problem with relying on nim r is that it can leave executable files littered in folders, since in the original code you have to do a 'existedBefore' and 'existedAfter' check. If executable files, due to some crash or whatever reason, were not cleaned up properly, you now have these files scattered all over the test folder's subdirectories that will -not- get removed next 'nimble test' run (because existedBefore will now be true, so they won't get trashed).

Traditionally, a build folder is very clear that it's just build output and can be safely deleted.

I guess tests/build could work.

I just think it's important to have this discussion because it would make nimble test plug and play.

My workflow is to mirror my 'tests' folder based on my src folder, more or less.
So if I had a src/foo/foo_database.nim, it would have a corresponding tests/foo/test_foo_database.nim

optsCopy.getCompilationFlags().add("--path:.")
let
binFileName = file.path.changeFileExt(ExeExt)
existsBefore = fileExists(binFileName)

if options.continueTestsOnFailure:
inc tests
Expand All @@ -1052,15 +1054,6 @@ proc test(options: Options) =
else:
execBackend(pkgInfo, optsCopy)

let
existsAfter = fileExists(binFileName)
canRemove = not existsBefore and existsAfter
if canRemove:
try:
removeFile(binFileName)
except OSError as exc:
display("Warning:", "Failed to delete " & binFileName & ": " &
exc.msg, Warning, MediumPriority)

if failures == 0:
display("Success:", "All tests passed", Success, HighPriority)
Expand Down Expand Up @@ -1194,7 +1187,7 @@ proc doAction(options: var Options) =
else:
raiseNimbleError(msg = "Could not find task $1 in $2" %
[options.action.command, nimbleFile],
hint = "Run `nimble --help` and/or `nimble tasks` for" &
hint = "Run `nimble --help` and/or `nimble tasks` for" &
" a list of possible commands.")

when isMainModule:
Expand Down