syncinstall refactor: new options for package output handling, artifact/workdir reuse, many fixes

[ZoomRmc]

syncinstall.nim refactor

First of all, some disclaimers are in order. This PR is far more intrusive than I'd like, sorry. It's also LLM-assisted, which is also a reason for its scope reaching wider than I usually deem fitting. I'm still getting used to this workflow and my approach is far from ideal. I tried to leave most of the commit history just so it's clear it's not a blind one-shot slop and that all the changes passed through human review, though it's chaotic and leaks some things rejected from the final state.

This PR marked as draft as the master branch is live and pakku doesn't have a dev/staging branch. This doesn't mean it's not ready. I've been using this branch for my personal machines since May without any problems, though more testing would be great. Advice on how to safely move forward is wanted.

Why

This PR refactors syncinstall.nim (plus touches a couple lines in other places). The initial goal was to introduce tighter integration with system's makepkg and fixing some annoyances with the working directory cleanup and artifact handling.

While working on the features it became obvious that the code is written in a peculiar way which overutilizes closures, recursion and is not that easy to follow if you're just diving into the project. So most of the refactor was done just to improve readability for myself. While doing so it became clear that the process also uncovers some minor bugs and inefficiencies so I've doubled down.

What's changed in short

• Replaces recursive-closure build-and-install logic with an explicit, iterative, typed pipeline: resolve -> order -> build/reuse -> prepare artifacts -> install -> cleanup.
• Adds three new user-facing settings for controlling built package output desitnation and workdir cleanup.
• Adds cross-run artifact and workdir reuse, eschewing repeating the whole process from the ground-up. Useful for continuing aborted runs.

New features

This lists the changes with links to relevant procs and compares with the master branch.

New config options:

• PackageOutputDir — buildLoop now writes makepkg output to config.packageOutputDir when set, instead of always using the tmp workspace (internalPkgdest in buildLoop). Master hard-coded stagingDir = config.tmpRootInitial as PKGDEST with no override.
• CleanupAfterInstall = Full | Worktree | None — cleanupSpec() derives worktree/archive/tmp-prefix removal from policy. Master had exactly one cleanup mode, wired 1:1 to install success via a keepBuiltArtifacts: bool that only ever meant "install failed, keep everything."
• New value for PreserveBuilt = Pkgdest — installGroupFromSources gains an effectivePkgdest-based branch (config.preserveBuilt == PreserveBuilt.pkgdest) copying archives to the user's resolved PKGDEST (via common.resolveEffectivePkgdest) before pacman runs.
• KeepBuildDirOnFailure / KeepBuiltPackagesOnFailure — drive cleanupSpecForFailure(buildFailure, ...). Master always deletes build artifacts unconditionally on build failure (cleanupBuiltArtifacts(true) in installGroupFromSources).

Cross-run artifact reuse

• findReusableArtifacts() scans PackageOutputDir and the configured PreserveBuilt destination before building each base (called from buildAllBases) and skips the build entirely if a matching archive is already present. No equivalent existed in master, nothing is ever reused.

Structural refactoring

1. Most procs now return typed objects instead of ad-hoc tuples. Too many to list. The main benefit is now any desynchronisation (especially cross-module, such as possible with the install helpers) will lead to readable compilation errors. In my book it's huge as you now can easily identify what's bounced between procs by object name, not infer from field composition.
2. Some things, like InstallMode, are now enum encoded instead of being stringly-typed.
3. Lots of routines rewritten from recursion to iteration and from closure-heavy to untangled funcs. This is the largest part of PR by LoC and is the most intrusive. However, this untangles logic, improves readability and makes some low-effort assertion-based hardening possible (see below). LLM-generated summary:

• findDependencies (tail-recursive, while true: loop in the new file).
• orderInstallation (recursive accumulator → while remaining.len > 0:).
• The build loop inside installGroupFromSources (master's local recursive buildNext) → buildAllBases, a plain for index in 0 ..< basePackages.len.
• The interactive review loop (master's nested recursive checkNext inside confirmViewAndImportKeys) → top-level checkNext with a for pkgInfos in flatBasePackages: loop.
• keysLoop, editFileLoopAll → while true: / for file in files: loops.
• The outer per-base install loop in handleInstall (master's recursive installNext) → for index in 0 ..< basePackages.len: with early break.
• Predicate/lookup helpers that were nested closures capturing handle, dbs, satisfied, nodepsCount, etc. inside master's findDependencies (checkDependencyCycle, findInSatisfied, findInAdditional, findInDatabaseWithGroups, findInDatabase, findInDatabases) and inside master's handleInstall (checkSatisfied) are now top-level funcs that take those values as explicit parameters.

4. Consolidated privileged-subprocess execution. New exec() overloads (ExecMode = emNormal | emSilent | emRedirect, with a dropPrivs flag and optional cwd) replace the repeated forkWait/forkWaitRedirect + manual fd close/open + dropPrivilegesAndChdir/dropPrivRedirectAndChdir template pairs scattered through editFileLoop, viewDiffLoop, editLoop, keysLoop, buildLoop, readConfExt, buildFromSources, and the git-tagging step (now tagBuiltBases). One specific defect this removes is double-forking in keysLoop: master's PGP import path is forkWait(() => dropPrivilegesAndChdir(...): forkWait(() => execResult(...))) (a fork inside a fork) while the new keysLoop calls exec(...) once per import attempt.

Correctness fixes and optimisations

1. Conflict detection compared a package's conflicts against itself instead of against the other package. Master's filterIgnoresAndConflicts / nonConflicingPkgInfos:

   for c in p.conflicts:
     if c.isProvidedBy(p.rpc.toPackageReference, true): ...

   this checks whether p conflicts with p's own reference — never a meaningful check against b.

   The new filterIgnoresAndConflicts:

   if b.conflicts.anyIt(it.isProvidedBy(pRef, true)) or
      p.conflicts.anyIt(it.isProvidedBy(bRef, true)):

   checks both directions against the other package's reference, which is the actually-intended symmetric conflict test.

2. printAllWarnings rebuilt seq->table installedTable once per package, inside the warning loop. Master:

   for pkgInfo in pkgInfos:
     let installedTable = installed.map(i => (i.name, i)).toTable
     ...

   The new printAllWarnings takes installedTable as a parameter, built once by the caller (handleInstall/resolveBuildTargets). Same result, O(n) fewer table rebuilds.

3. Build artifacts are now verified to exist rather than assumed. Master's formatArchiveFile just string-formats an expected path (name-version-arch.ext) and hands it to the installer without checking the file is actually there. The new findArtifactsInDir does a single walkDir scan matching real files against expected stems and only returns success if every expected package base was actually found on disk, printing a clear error otherwise (buildLoop).
   As part of the same scan, .pkg.tar.*.sig sidecar files are explicitly excluded from matching (the "no further . after the extension" check in artifactStemFromFilename). Master has no equivalent filtering.

4. canonicalRenames can no longer leak from a failed install group.
   handleInstall's per-base loop only merges installResult.canonicalRenames into the running table after checking installResult.code == 0. A mid-loop failure stops contributing renames before break.

5. buildFromSources in master takes commonArgs: seq[Argument] and never reads it. Dropped.

6. Most uses of sequtils templates changed from closure-based map/any/filter to their slightly more efficient -It counterparts or changed to loops, where this removes transient sequence allocation.

Style fixes

• Pure routines changed to func from proc
• Read-only parameters use openArray instead of seq
• IsContainer concept + requireNonEmpty template allows some slight hardening with assertions.
• Changing the Config in src/config.nim from tuple to object enables using Nim's default object values to simplify init.
• Proc and type doc comments added. There were almost none of those!

Invariants checked with assertions

Unfortunately, pakku doesn't have any tests for the module. Testing is another topic entirely, but the least I could do is to sprkinle some assertions throughout the code, so during any further work dumb logic mistakes would visibly trigger them. This wouldn't be easy without untangling the code mentioned above. Just to remind, asserts in Nim are compiled away and are no-ops in release mode, doAssert statements stay.

LLM-generated summary:

• buildAllBases: assert results.len + skippedCount == basePackages.len — every input base is built, reused, or explicitly counted as skipped.
• installGroupFromSources: assert build.results.len + build.skipped <= basePackages.len and assert build.code == 0 or build.results.len + build.skipped < basePackages.len — a failed build run cannot be mistaken for a complete one.
• prepareArtifacts: assert installFiles.len == install.len, assert install.len <= artifacts.len, and assert aname notin renames, "duplicate canonicalization key" — install targets/files stay aligned, can't exceed discovered artifacts, and split-package rename keys can't collide.
• installGroupFromSources: assert installWithReason.len > 0 and assert pacmanUpgradeParams.len > 0 and pacmanDatabaseParams.len > 0 — the install-helper subprocess is never invoked with an empty payload.
• obtainAurPackageInfos: doAssert target.rpcInfo.isSome and several assert target.sync.foundInfos... / assert target.sync.foundInfos[0].pkg.isSome in obtainPacmanBuildTargets.

Compatibility

• Visible behaviour should be unchanged for default config (CleanupAfterInstall = Full, PreserveBuilt = Disabled, PackageOutputDir = "").

Other gains

This should go to the optimisations above, but just another arguent in favour of the refactor. Even though this PR is pretty big and adds several new features, the compiled and stripped release build loses ~ 37KB, mostly from dropping excessive closures, object reuse and less container back-and-forth churn. Haven't measured allocations, but they should be down considerably. Not that it matters much for the end users, but I claim this is evidence this PR moves in the right direction.

[zqqw]

It seems to build OK and my system updated after running pakku -Syu, but despite updating rmlint-git from the AUR it tries to update it every time I run Syu again, so that part doesn't seem to be working quite right yet from a quick first check. For testing purposes I would suggest you should come up with various scenarios and combinations of actions, especially relating to the Pakku command options and config possibilities where you have altered the code. This may mean installing a few small AUR packages and trying out some different options in /etc/pacman.conf and /etc/pakku.conf, possibly downgrading some packages deliberately so they will be updated, that kind of thing. What LLM did you use, it may be helpful to credit the source in that respect?

[ZoomRmc]

but despite updating rmlint-git from the AUR it tries to update it every time I run Syu again, so that part doesn't seem to be working quite right

This is unexpected and embarrassing if in deed so, as it's the core functionality. What's your config, specifically, what's changed from the defaults? If the package is indeed installed it shouldn't ever be listed again. As already mentioned, for me it works correctly.

Also, rmlint-git fails to build for me on a fully up-to-date system, are you sure you got the package built?

What LLM did you use, it may be helpful to credit the source in that respect?

I don't think it matters much, because I'd be responsible anyway. Also, it was a convoluted process involving several models (via different interfaces) to cross-check and/or suggest changes.

It would be nice to discuss this with you directly, what's the best way to reach you? You can ping me at Matrix (see profile).

[zqqw]

This is an open source project so it would be helpful for any future contributors to know what LLM's you used for any significant inputs so they could go back and ask it for more details about what it did if needed for example - if you just searched online and got an AI summary once or twice then that's hardly relevant, but you seemed to infer above there was more than that, and not all AI's have the same capabilities Also for much the same reason, it's best to keep any discussion about the project here in public view, the messages here reach me as directly as any other means.
rmlint-git builds and installs fine here, possibly you are missing some undefined build-dep that I happen to have by chance - it just keeps getting built on every -Syu - perhaps your changes might have accidentally broken the way git package versions are checked for update? /etc/pakku.conf and /etc/makepkg.conf are standard here and /etc/pacman.conf has nothing special beyond what is needed to make it work. The Nim language version is the regular repo package version too.

[ZoomRmc]

now what LLM's you used for any significant inputs so they could go back and ask it for more details about what it did if needed for example

Sorry, I don't really understand your point. Without the context of the actual session there's zero difference in which model you're asking, they would be inferring everything from the code and the prompt anew anyway.

If one needs to understand the code they can ask whatever. PR-related questions should be addressed to me.

To answer your question directly: Deepseek flash and pro, OpenAI Codex were used interchangeably via agents, Claude used via the web interface. Due to the convoluted and staged nature of the workflow you can't really get any meaningful info about any particular model's capabilities from this PR. FWIW, I can tell you they all stumble and need constant steering.

[ZoomRmc]

Ok, I've successfully installed rmlint-git and I can confrm the issue. However, it's unrelated to this PR. It's pre-existing in the master branch and is also consistent with how yay and paru handle mismatched version info for -git packages, when their --gendb option is not used.

It shows how little do I use devel packages so I never noticed this issue before.

Here's the issue filed: #47