Skip to content
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
2 changes: 1 addition & 1 deletion src/nimble.nim
Original file line number Diff line number Diff line change
Expand Up @@ -2270,7 +2270,7 @@ when isMainModule:
# Actions that don't need a Nim binary should not trigger downloading Nim.
# This avoids e.g. `nimble --version` or `nimble list -i` fetching Nim binaries.
const actionsNotNeedingNim = {actionRefresh, actionSearch, actionList,
actionPath, actionUninstall, actionClean, actionManual,
actionPath, actionClean, actionManual,
actionNil}
let needsNim = not opt.showVersion and not opt.showHelp and
opt.action.typ notin actionsNotNeedingNim
Expand Down
1 change: 1 addition & 0 deletions src/nimblepkg/nimscriptwrapper.nim
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ proc getNimblecache(): string =
proc execNimscript(nimBin: string,
nimbleFile, nimsFile, actionName: string, options: Options, isHook: bool
): tuple[output: string, exitCode: int, stdout: string] =
assert nimBin != ""
let
outFile = getNimbleTempDir() & ".out"
isCustomTask = isCustomTask(actionName, options)
Expand Down
4 changes: 4 additions & 0 deletions src/nimblepkg/options.nim
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ type
noColor*: bool
disableValidation*: bool
continueTestsOnFailure*: bool
skipBin*: bool # Whether to skip compilation of binaries for hybrid dependencies.
## Whether packages' repos should always be downloaded with their history.
forceFullClone*: bool
# Temporary storage of flags that have not been captured by any specific Action.
Expand Down Expand Up @@ -295,6 +296,7 @@ Nimble Options:
--features Activate features. Only used when using the declarative parser.
--ignoreSubmodules Ignore submodules when cloning a repository.
--asyncdownloads Use async for package downloads. (temporary flag)
--skipBin Skip compilation of binaries of hybrid dependencies.
For more information read the GitHub readme:
https://github.com/nim-lang/nimble#readme
"""
Expand Down Expand Up @@ -787,6 +789,8 @@ proc parseFlag*(flag, val: string, result: var Options, kind = cmdLongOption) =
result.useAsyncDownloads = true
of "lenient":
result.lenient = true
of "skipbin":
result.skipBin = true
else: isGlobalFlag = false

var wasFlagHandled = true
Expand Down
37 changes: 28 additions & 9 deletions src/nimblepkg/vnext.nim
Original file line number Diff line number Diff line change
Expand Up @@ -702,16 +702,26 @@ proc executeHook(nimBin: string, dir: string, options: var Options, action: Acti
else:
raise nimbleError("Post-hook prevented further execution.")

proc binsExist(pkgInfo: PackageInfo, options: Options): bool =
let needsBinaries = pkgInfo.bin.len > 0 and not pkgInfo.basicInfo.name.isNim and not options.skipBin
if needsBinaries:
for bin in pkgInfo.bin.keys:
let binPath = pkgInfo.getOutputDir(bin)
if not fileExists(binPath):
return false
result = true

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just return here


proc packageExists(nimBin: string, pkgInfo: PackageInfo, options: Options):
Option[PackageInfo] =
## Checks whether a package `pkgInfo` already exists in the Nimble cache. If a
## package already exists returns the `PackageInfo` of the package in the
## cache otherwise returns `none`. Raises a `NimbleError` in the case the
## package exists in the cache but it is not valid.
## cache otherwise returns `none`. If a package exits but is missing expected binaries also returns `none`.
## Raises a `NimbleError` in the case the package exists in the cache but it is not valid.
##
## Also checks for packages with the same name and checksum but different version
## to avoid storing the same content multiple times with different version labels.
let pkgDestDir = pkgInfo.getPkgDest(options)

if fileExists(pkgDestDir / packageMetaDataFileName):
var oldPkgInfo = initPackageInfo()
try:
Expand All @@ -720,6 +730,10 @@ proc packageExists(nimBin: string, pkgInfo: PackageInfo, options: Options):
raise nimbleError(&"The package inside \"{pkgDestDir}\" is invalid.",
details = error)
fillMetaData(oldPkgInfo, pkgDestDir, true, options)

if not binsExist(pkgInfo, options):
return none(PackageInfo)

return some(oldPkgInfo)

# Check if a package with the same name and checksum exists with a different version.
Expand All @@ -740,6 +754,10 @@ proc packageExists(nimBin: string, pkgInfo: PackageInfo, options: Options):
except CatchableError:
continue # Skip invalid packages
fillMetaData(oldPkgInfo, path, true, options)

if not binsExist(pkgInfo, options):
return none(PackageInfo)

return some(oldPkgInfo)

return none[PackageInfo]()
Expand Down Expand Up @@ -808,13 +826,14 @@ proc installFromDirDownloadInfo(nimBin: string, downloadDir: string, url: string
# Don't copy artifacts if project local deps mode and "installing" the top level package.
if not (options.localdeps and options.isInstallingTopLevel(dir)):
var filesInstalled: HashSet[string]
let hasBinaries = pkgInfo.bin.len > 0 and not pkgInfo.basicInfo.name.isNim
let shouldBuildBinaries = pkgInfo.bin.len > 0 and not pkgInfo.basicInfo.name.isNim and
(not options.skipBin or options.isInstallingTopLevel(dir))
let hasPreInstallHook = pkgInfo.hasBeforeInstallHook and not pkgInfo.basicInfo.name.isNim

# Install pipeline: workDir → before-install hook → build → copy to pkgDestDir → after-install hook
# Optimization: skip buildtemp when we know it's safe (no binaries, no before-install hook, no submodules)
let hasSubmodules = not options.ignoreSubmodules and fileExists(downloadDir / ".gitmodules")
let canSkipBuildTemp = not hasBinaries and not hasPreInstallHook and not hasSubmodules
let canSkipBuildTemp = not shouldBuildBinaries and not hasPreInstallHook and not hasSubmodules

var workDir, buildTempDir: string
var workPkgInfo: PackageInfo
Expand All @@ -825,7 +844,7 @@ proc installFromDirDownloadInfo(nimBin: string, downloadDir: string, url: string
workPkgInfo = pkgInfo
else:
display("Info:", "Using buildtemp for " & pkgInfo.basicInfo.name &
" (binaries: " & $hasBinaries & ", before-install hook: " & $hasPreInstallHook &
" (binaries: " & $shouldBuildBinaries & ", before-install hook: " & $hasPreInstallHook &
", submodules: " & $hasSubmodules & ")",
priority = LowPriority)
buildTempDir = options.getPkgBuildTempDir(
Expand Down Expand Up @@ -882,7 +901,7 @@ proc installFromDirDownloadInfo(nimBin: string, downloadDir: string, url: string
executeHook(nimBin, workDir, options, actionInstall, before = true)

# Build binaries (only if there are any)
if hasBinaries:
if shouldBuildBinaries:
let paths = getPathsAllPkgs(options, nimBin)
let flags = if options.action.typ in {actionInstall, actionPath, actionUninstall, actionDevelop}:
options.action.passNimFlags
Expand Down Expand Up @@ -921,7 +940,7 @@ proc installFromDirDownloadInfo(nimBin: string, downloadDir: string, url: string
filesInstalled.incl copyFileD(workPkgInfo.myPath, nimbleFileDest)

# Copy built binaries (only if there are any)
if hasBinaries:
if shouldBuildBinaries:
for bin, src in workPkgInfo.bin:
let binDest = if dirExists(pkgDestDir / bin): bin & ".out" else: bin
let srcBin = workPkgInfo.getOutputDir(bin)
Expand All @@ -940,7 +959,7 @@ proc installFromDirDownloadInfo(nimBin: string, downloadDir: string, url: string
executeHook(nimBin, pkgDestDir, options, actionInstall, before = false)

# Create bin symlinks (only if there are binaries)
if hasBinaries:
if shouldBuildBinaries:
createBinSymlink(pkgInfo, options)

finally:
Expand Down Expand Up @@ -1432,7 +1451,7 @@ proc installPkgs*(satResult: var SATResult, options: var Options, nimBin: string
if isRoot and options.action.typ in rootBuildActions:
buildPkg(nimBin, pkgToBuild, isRoot, options)
satResult.buildPkgs.add(pkgToBuild)
elif pkgToBuild.isLink:
elif pkgToBuild.isLink and not options.skipBin:
# Build develop mode packages
buildPkg(nimBin, pkgToBuild, false, options)
satResult.buildPkgs.add(pkgToBuild)
Expand Down
14 changes: 14 additions & 0 deletions tests/pkgWithHybridDep/pkgWithHybridDep.nimble
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Package

version = "0.1.0"
author = "test"
description = "A new awesome nimble package"
license = "MIT"
srcDir = "src"
bin = @["pkgWithHybridDep"]


# Dependencies

requires "nim >= 2.2.4"
requires "https://github.com/nim-lang/nimble?subdir=tests/develop/hybrid"
5 changes: 5 additions & 0 deletions tests/pkgWithHybridDep/src/pkgWithHybridDep.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# This is just an example to get you started. A typical binary package
# uses this file as the main entry point of the application.

when isMainModule:
echo("Hello, World!")
1 change: 1 addition & 0 deletions tests/tester.nim
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import tfilepathrequires
import tglobalinstall
import tasynctools
import tbuildinstall
import tskipbin
# # nonim tests are very slow and (often) break the CI.

# # import tnonim
39 changes: 39 additions & 0 deletions tests/tskipbin.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Tests that with --skipBin hybrid packages are not compiled

{.used.}

import unittest, os, strutils
import testscommon
from nimble import nimblePathsFileName, nimbleConfigFileName
from nimblepkg/common import cd

func exe(name: string): string =
when defined(windows): name & ".exe" else: name

template checkSkip(cmd: string, toSkip = false) =
var args = @[cmd]
if toSkip: args.add "--skipBin"
let res = execNimbleYes(args)
verify res
let hybridPkgDir = getPackageDir(pkgsDir, "hybrid")
check hybridPkgDir.len > 0
check "Building hybrid/hybrid" in res.output != toSkip
check fileExists(hybridPkgDir / "hybrid".exe) != toSkip
check fileExists(installDir / "bin" / "hybrid") != toSkip
# --skipBin should not effect the root package
if cmd == "install":
check fileExists(installDir / "bin" / "pkgWithHybridDep")

template runTest(name: string, body: untyped) =
test name:
cd "pkgWithHybridDep":
cleanDir installDir
cleanFiles nimblePathsFileName, nimbleConfigFileName
body

suite "--skipBin":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please dont use templates to expand the tests, its fine to extract repeated code but this doesnt follow the symmetry of the whole codebase. Try to be more meaningful in the suite name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought at least one template would be necessary to abstract the repeated step (checkSkip in this case) when using std/unittest since otherwise the check false isn't propagated to the test block level and then the test itself shows as [OK] even with failures.

for command in ["setup", "install"]:
runTest command & " with --skipBin & without --skipBin":
checkSkip(command, true)
checkSkip(command, false)