fix: Small changes affecting packaging and cleanup by Syedowais312 · Pull Request #2641 · unikraft/kraftkit

Syedowais312 · 2026-02-08T21:23:44Z

Summary

Clean up leftover runtime artifacts when the KraftKit package is
removed or purged.

Changes

Install a cleanup helper via nfpm
Invoke cleanup logic from the postrm hook
Ensure no leftover artifacts remain after package removal

Testing

Built snapshot packages using GoReleaser
Installed .deb, verified artifacts
Removed and purged package, verified cleanup

closes: #2550

Syedowais312 · 2026-02-08T21:33:41Z

Hi @nderjung @craciunoiuc,
whenever you get a moment, could you please take a look at this PR?

craciunoiuc

Some comments

nderjung

Please see our contribution guidelines for commit hygiene.

craciunoiuc

some more comments my side

craciunoiuc · 2026-02-17T08:33:23Z

+CONFIG="${HOME}/.config/kraftkit/config.yaml"
+
+# Defaults
+CACHE_DIR="${HOME}/.cache/kraftkit"
+DATA_DIR="${HOME}/.local/share/kraftkit"


Clean up for other users also

Do you want cleanup to apply only to the invoking user, or also root’s home when KraftKit was used via sudo? I want to avoid removing unrelated users’ data

Copilot

Pull request overview

Adds package-removal cleanup for KraftKit by shipping a helper script via nfpm and wiring it into the Debian postrm hook, aiming to remove user runtime artifacts on uninstall/purge.

Changes:

Add scripts/cleanup-artifacts.sh and install it as /usr/libexec/kraftkit/cleanup-artifacts in nfpm packages
Add packaging/deb/postrm and register it as the nfpm postremove script
Update GoReleaser configs (stable + staging) to include the helper + postremove hook (and adjust build tags/ignore matrix)

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
scripts/cleanup-artifacts.sh	Cleanup helper that attempts to derive artifact directories from config and delete them
packaging/deb/postrm	Debian post-remove maintainer script invoking cleanup and deleting config on purge
.goreleaser-staging.yaml	nfpm packaging updates to ship helper and run postremove hook
.goreleaser-stable.yaml	nfpm packaging updates to ship helper and run postremove hook

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

craciunoiuc · 2026-02-23T12:37:50Z

Please do a pass and click Resolve conversation for any comments that have been fixed.

Syedowais312 · 2026-03-05T09:17:19Z

Please do a pass and click Resolve conversation for any comments that have been fixed.

Done. I have reviewed the comments and resolved the conversations where the issues have been addressed. The suggested changes that made sense have been applied...
Please let me know if anything else needs adjustment.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T13:49:04Z

+CONFIG="${HOME}/.config/kraftkit/config.yaml"
+
+# Default runtime directories created by KraftKit.
+CACHE_DIR="${HOME}/.cache/kraftkit"
+DATA_DIR="${HOME}/.local/share/kraftkit"
+
+
+# If a config exists and yq is available, read custom paths.
+# Fallback to defaults to ensure cleanup always succeeds.
+if [ -f "$CONFIG" ] && command -v yq >/dev/null 2>&1; then
+  CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"
+  DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"
+fi
+
+
+# Safety guards:
+#  - non empty
+#  - not root directory
+#  - ignore missing paths
+[ -n "$CACHE_DIR" ] && [ "$CACHE_DIR" != "/" ] && rm -rf -- "$CACHE_DIR" || true
+[ -n "$DATA_DIR" ] && [ "$DATA_DIR" != "/" ] && rm -rf -- "$DATA_DIR" || true


This helper relies on $HOME to locate/remove artifacts. When invoked from package maintainer scripts it typically runs as root (HOME=/root), so it will miss artifacts for real users and/or delete root’s cache instead. Consider iterating over actual user home dirs (e.g., from /etc/passwd) and removing per-user paths relative to each home, or accept an explicit target home/user and have the maintainer script call it appropriately.

Suggested change

CONFIG="${HOME}/.config/kraftkit/config.yaml"

# Default runtime directories created by KraftKit.

CACHE_DIR="${HOME}/.cache/kraftkit"

DATA_DIR="${HOME}/.local/share/kraftkit"

# If a config exists and yq is available, read custom paths.

# Fallback to defaults to ensure cleanup always succeeds.

if [ -f "$CONFIG" ] && command -v yq >/dev/null 2>&1; then

CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"

DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"

fi

# Safety guards:

# - non empty

# - not root directory

# - ignore missing paths

[ -n "$CACHE_DIR" ] && [ "$CACHE_DIR" != "/" ] && rm -rf -- "$CACHE_DIR" || true

[ -n "$DATA_DIR" ] && [ "$DATA_DIR" != "/" ] && rm -rf -- "$DATA_DIR" || true

cleanup_user_home() {

user_home=$1

# Skip if home is empty or not a directory.

[ -n "$user_home" ] && [ -d "$user_home" ] || return 0

CONFIG="${user_home}/.config/kraftkit/config.yaml"

# Default runtime directories created by KraftKit for this user.

CACHE_DIR="${user_home}/.cache/kraftkit"

DATA_DIR="${user_home}/.local/share/kraftkit"

# If a config exists and yq is available, read custom paths.

# Fallback to defaults to ensure cleanup always succeeds.

if [ -f "$CONFIG" ] && command -v yq >/dev/null 2>&1; then

CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"

DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"

fi

# Safety guards:

# - non empty

# - not root directory

# - ignore missing paths

[ -n "$CACHE_DIR" ] && [ "$CACHE_DIR" != "/" ] && rm -rf -- "$CACHE_DIR" || true

[ -n "$DATA_DIR" ] && [ "$DATA_DIR" != "/" ] && rm -rf -- "$DATA_DIR" || true

}

# When run as root (e.g., from maintainer scripts), iterate over real users'

# home directories. Otherwise, clean only the current user's home.

uid=$(id -u 2>/dev/null || echo 1)

if [ "$uid" -eq 0 ]; then

# Root: iterate over users from /etc/passwd (UID >= 1000, exclude nobody 65534).

if [ -r /etc/passwd ]; then

while IFS=: read -r _ _ pw_uid _ _ pw_home _; do

# Ensure UID is numeric.

case "$pw_uid" in

''|*[!0-9]*) continue ;;

esac

if [ "$pw_uid" -ge 1000 ] && [ "$pw_uid" -ne 65534 ]; then

[ -n "$pw_home" ] && cleanup_user_home "$pw_home"

fi

done < /etc/passwd

fi

else

# Non-root: operate on the current user's home.

cleanup_user_home "${HOME:-}"

fi

Copilot · 2026-04-02T13:49:05Z

+  CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"
+  DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"


The config keys read here ('.paths.cache' and '.paths.data') don’t exist in KraftKit’s config schema (config/config.go defines paths.manifests, paths.sources and runtime_dir). As a result custom locations won’t be respected. Update the expressions to match the actual keys you need to clean (and note that the config directory itself can be controlled via env and may not be under $HOME/.config).

Suggested change

CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"

DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"

CACHE_DIR="$(yq '.runtime_dir // "'"$CACHE_DIR"'"' "$CONFIG")"

DATA_DIR="$(yq '.paths.sources // "'"$DATA_DIR"'"' "$CONFIG")"

Copilot · 2026-04-02T13:49:05Z

+  CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"
+  DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"


Because this script uses 'set -e', a failing 'yq' invocation (unsupported yq flavor/version, parse error, etc.) will abort the script, contradicting the “never fails package uninstall” goal. Make yq reads best-effort (capture failures and fall back to defaults), and consider using yq raw output mode to avoid quoted/scalar formatting issues.

Suggested change

CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"

DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"

NEW_CACHE_DIR="$(yq -r '.paths.cache // ""' "$CONFIG" 2>/dev/null || printf '')"

[ -n "$NEW_CACHE_DIR" ] && CACHE_DIR="$NEW_CACHE_DIR"

NEW_DATA_DIR="$(yq -r '.paths.data // ""' "$CONFIG" 2>/dev/null || printf '')"

[ -n "$NEW_DATA_DIR" ] && DATA_DIR="$NEW_DATA_DIR"

Copilot · 2026-04-02T13:49:05Z

+# Safety guards:
+#  - non empty
+#  - not root directory
+#  - ignore missing paths
+[ -n "$CACHE_DIR" ] && [ "$CACHE_DIR" != "/" ] && rm -rf -- "$CACHE_DIR" || true
+[ -n "$DATA_DIR" ] && [ "$DATA_DIR" != "/" ] && rm -rf -- "$DATA_DIR" || true


The current safety guard only prevents empty and '/' paths. If the config points at '.'/'..', '/tmp', or another broad directory, this will recursively delete unintended data. Add stricter validation (e.g., require absolute paths under the expected base dir, reject '.', '..', and paths not ending in '/kraftkit', and/or resolve/verify with realpath before rm -rf).

Suggested change

# Safety guards:

# - non empty

# - not root directory

# - ignore missing paths

[ -n "$CACHE_DIR" ] && [ "$CACHE_DIR" != "/" ] && rm -rf -- "$CACHE_DIR" || true

[ -n "$DATA_DIR" ] && [ "$DATA_DIR" != "/" ] && rm -rf -- "$DATA_DIR" || true

# Safely remove a directory if it looks like an intended KraftKit path.

# Safety guards:

# - non empty

# - not root directory, '.', or '..'

# - absolute path

# - resolved (if realpath available)

# - under $HOME

# - path ends with '/kraftkit'

safe_rm() {

dir="$1"

# Ignore empty.

[ -z "$dir" ] && return 0

# Reject obviously dangerous values.

case "$dir" in

/|.|..|"") return 0 ;;

esac

# Require absolute paths.

case "$dir" in

/*) ;;

*) return 0 ;;

esac

# Resolve path if realpath is available.

if command -v realpath >/dev/null 2>&1; then

resolved="$(realpath -- "$dir" 2>/dev/null || true)"

[ -z "$resolved" ] && return 0

else

resolved="$dir"

fi

# Final sanity checks on the resolved path.

case "$resolved" in

/|.|..|"") return 0 ;;

esac

# Require the path to be under the user's home directory.

case "$resolved" in

"$HOME"/*) ;;

*) return 0 ;;

esac

# Require the directory name to be 'kraftkit' (to avoid wiping broader dirs).

case "$resolved" in

*/kraftkit) ;;

*) return 0 ;;

esac

rm -rf -- "$resolved"

}

# Ignore missing paths; safe_rm performs all other checks.

safe_rm "$CACHE_DIR" || true

safe_rm "$DATA_DIR" || true

Copilot · 2026-04-02T13:49:06Z

+if [ -x "$CLEANUP" ]; then
+  "$CLEANUP" || true
+fi


This prerm runs cleanup unconditionally. On Debian, prerm is also invoked during upgrades, so this would delete runtime/cache data on upgrade as well as on remove. Additionally, deleting user data during a normal “remove” conflicts with the stated policy in postrm. Gate the cleanup by $1 (e.g., only run on remove/purge, and avoid destructive cleanup on upgrade unless strictly necessary).

Suggested change

if [ -x "$CLEANUP" ]; then

"$CLEANUP" || true

fi

case "$1" in

remove|purge)

if [ -x "$CLEANUP" ]; then

"$CLEANUP" || true

fi

;;

*)

# On upgrade/deconfigure and other actions, avoid destructive cleanup.

;;

esac

Copilot · 2026-04-02T13:49:06Z

@@ -0,0 +1,18 @@
+#!/usr/bin/env bash


Using '#!/usr/bin/env bash' in maintainer scripts adds an unnecessary dependency on bash (and on /usr/bin/env) for uninstall paths. Debian policy commonly recommends /bin/sh for maintainer scripts unless bash-specific features are required; this script is POSIX-compatible and could use /bin/sh.

Suggested change

#!/usr/bin/env bash

#!/bin/sh

Copilot · 2026-04-02T13:49:06Z

      #@ end
    tags:
+      - containers_image_storage_stub
+      - containers_image_openpgp      


This build tag entry appears to include trailing whitespace. If the YAML parser preserves it, Go will receive a tag like 'containers_image_openpgp ' which won’t match any //go:build constraints and can cause confusing build behavior. Remove the trailing spaces to ensure the tag is exactly 'containers_image_openpgp'.

Suggested change

- containers_image_openpgp

- containers_image_openpgp

Signed-off-by: syedowais312 <syedowais312sf@gmail.com>

Install the cleanup-artifacts helper in the package and register the preremove and postremove scripts. Enable container image storage and openpgp plugins. Signed-off-by: syedowais312 <syedowais312sf@gmail.com>

Syedowais312 · 2026-04-02T20:32:15Z

I went through the copilot suggestions and applied the ones that made sense.
Let me know if anything else needs tweaking.

Syedowais312 requested review from craciunoiuc and nderjung as code owners February 8, 2026 21:23

github-project-automation Bot moved this to 🧊 Icebox in KraftKit Roadmap Feb 8, 2026

github-project-automation Bot added this to KraftKit Roadmap Feb 8, 2026

Syedowais312 commented Feb 8, 2026

View reviewed changes

Comment thread .goreleaser-staging.yaml

Syedowais312 force-pushed the cleanup-package-removal branch from fb76d9d to 5714893 Compare February 8, 2026 21:31

craciunoiuc requested changes Feb 10, 2026

View reviewed changes

Syedowais312 requested review from craciunoiuc February 10, 2026 17:17

nderjung reviewed Feb 11, 2026

View reviewed changes

Comment thread .goreleaser-stable.yaml Outdated

nderjung reviewed Feb 11, 2026

View reviewed changes

Comment thread .goreleaser-stable.yaml Outdated

nderjung reviewed Feb 11, 2026

View reviewed changes

Comment thread .goreleaser-stable.yaml Outdated

Syedowais312 requested a review from nderjung February 11, 2026 11:00

nderjung requested changes Feb 13, 2026

View reviewed changes

Comment thread .goreleaser-staging.yaml

Comment thread .goreleaser-staging.yaml Outdated

Comment thread .goreleaser-stable.yaml Outdated

Comment thread packaging/deb/postrm Outdated

nderjung changed the title ~~fix(packaging): Clean up artifacts on package removal~~ fix: Small changes affecting packaging and cleanup Feb 13, 2026

Syedowais312 force-pushed the cleanup-package-removal branch 5 times, most recently from 824da23 to 2b2e350 Compare February 17, 2026 08:20

craciunoiuc requested a review from Copilot February 17, 2026 08:31

Copilot started reviewing on behalf of craciunoiuc February 17, 2026 08:31 View session

Syedowais312 force-pushed the cleanup-package-removal branch from 2b2e350 to f0c3303 Compare February 17, 2026 08:34

craciunoiuc requested changes Feb 17, 2026

View reviewed changes

Copilot AI reviewed Feb 17, 2026

View reviewed changes

Syedowais312 force-pushed the cleanup-package-removal branch from f0c3303 to 8d1acbc Compare February 19, 2026 14:39

Syedowais312 requested a review from nderjung February 19, 2026 14:41

Syedowais312 force-pushed the cleanup-package-removal branch from 8d1acbc to 1a59a58 Compare February 23, 2026 14:08

craciunoiuc requested a review from Copilot April 2, 2026 13:43

Copilot started reviewing on behalf of craciunoiuc April 2, 2026 13:44 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

Syedowais312 added 3 commits April 3, 2026 01:52

fix(packaging): Cleanup artifacts on remove and purge

4e7c7ee

build(goreleaser): Add 386 architecture targets

9228588

Signed-off-by: syedowais312 <syedowais312sf@gmail.com>

build(goreleaser): Install cleanup helper and enable plugins

a1c09e4

Install the cleanup-artifacts helper in the package and register the preremove and postremove scripts. Enable container image storage and openpgp plugins. Signed-off-by: syedowais312 <syedowais312sf@gmail.com>

Syedowais312 force-pushed the cleanup-package-removal branch from 1a59a58 to a1c09e4 Compare April 2, 2026 20:26

		CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"
		DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"

-  CACHE_DIR="$(yq '.paths.cache // "'"$CACHE_DIR"'"' "$CONFIG")"
-  DATA_DIR="$(yq '.paths.data // "'"$DATA_DIR"'"' "$CONFIG")"
+  NEW_CACHE_DIR="$(yq -r '.paths.cache // ""' "$CONFIG" 2>/dev/null || printf '')"
+  [ -n "$NEW_CACHE_DIR" ] && CACHE_DIR="$NEW_CACHE_DIR"
+  NEW_DATA_DIR="$(yq -r '.paths.data // ""' "$CONFIG" 2>/dev/null || printf '')"
+  [ -n "$NEW_DATA_DIR" ] && DATA_DIR="$NEW_DATA_DIR"

-# Safety guards:
-#  - non empty
-#  - not root directory
-#  - ignore missing paths
-[ -n "$CACHE_DIR" ] && [ "$CACHE_DIR" != "/" ] && rm -rf -- "$CACHE_DIR" || true
-[ -n "$DATA_DIR" ] && [ "$DATA_DIR" != "/" ] && rm -rf -- "$DATA_DIR" || true
+# Safely remove a directory if it looks like an intended KraftKit path.
+# Safety guards:
+#  - non empty
+#  - not root directory, '.', or '..'
+#  - absolute path
+#  - resolved (if realpath available)
+#  - under $HOME
+#  - path ends with '/kraftkit'
+safe_rm() {
+  dir="$1"
+  # Ignore empty.
+  [ -z "$dir" ] && return 0
+  # Reject obviously dangerous values.
+  case "$dir" in
+    /|.|..|"") return 0 ;;
+  esac
+  # Require absolute paths.
+  case "$dir" in
+    /*) ;;
+    *) return 0 ;;
+  esac
+  # Resolve path if realpath is available.
+  if command -v realpath >/dev/null 2>&1; then
+    resolved="$(realpath -- "$dir" 2>/dev/null || true)"
+    [ -z "$resolved" ] && return 0
+  else
+    resolved="$dir"
+  fi
+  # Final sanity checks on the resolved path.
+  case "$resolved" in
+    /|.|..|"") return 0 ;;
+  esac
+  # Require the path to be under the user's home directory.
+  case "$resolved" in
+    "$HOME"/*) ;;
+    *) return 0 ;;
+  esac
+  # Require the directory name to be 'kraftkit' (to avoid wiping broader dirs).
+  case "$resolved" in
+    */kraftkit) ;;
+    *) return 0 ;;
+  esac
+  rm -rf -- "$resolved"
+}
+# Ignore missing paths; safe_rm performs all other checks.
+safe_rm "$CACHE_DIR" || true
+safe_rm "$DATA_DIR" || true

Conversation

Syedowais312 commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Uh oh!

Uh oh!

Syedowais312 commented Feb 8, 2026

Uh oh!

craciunoiuc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nderjung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craciunoiuc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

craciunoiuc Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Syedowais312 Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craciunoiuc commented Feb 23, 2026

Uh oh!

Syedowais312 commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Syedowais312 commented Feb 8, 2026 •

edited

Loading