Empty directories remain after `jpm uninstall`

[sogaiu]

For the following two repositories / projects, performing jpm uninstall here after an installation, leaves behind empty directories in the package cache under syspath:

• https://github.com/ianthehenry/jimmy
• https://github.com/ml-2/stx

Sample session:

$ git clone https://github.com/ianthehenry/jimmy && cd jimmy
Cloning into 'jimmy'...
remote: Enumerating objects: 125, done.
remote: Counting objects: 100% (125/125), done.
remote: Compressing objects: 100% (76/76), done.
remote: Total 125 (delta 63), reused 103 (delta 41), pack-reused 0
Receiving objects: 100% (125/125), 32.63 KiB | 32.63 MiB/s, done.
Resolving deltas: 100% (63/63), done.

$ jpm install
generating /home/user/.local/lib/janet/.manifests/jimmy.jdn...
Installed as 'jimmy'.
copying build/jimmy/native.so to /home/user/.local/lib/janet/jimmy...
copying build/jimmy/native.meta.janet to /home/user/.local/lib/janet/jimmy...
copying build/jimmy/native.a to /home/user/.local/lib/janet/jimmy...
copying src/set.janet to /home/user/.local/lib/janet/jimmy...
copying src/map.janet to /home/user/.local/lib/janet/jimmy...
copying src/vec.janet to /home/user/.local/lib/janet/jimmy...
copying src/util.janet to /home/user/.local/lib/janet/jimmy...
copying src/init.janet to /home/user/.local/lib/janet/jimmy...

$ ls -a ~/.local/lib/janet/jimmy/
.   init.janet  native.a           native.so  util.janet
..  map.janet   native.meta.janet  set.janet  vec.janet

$ jpm uninstall
removing /home/user/.local/lib/janet/jimmy/native.so
removing /home/user/.local/lib/janet/jimmy/native.meta.janet
removing /home/user/.local/lib/janet/jimmy/native.a
removing /home/user/.local/lib/janet/jimmy/set.janet
removing /home/user/.local/lib/janet/jimmy/map.janet
removing /home/user/.local/lib/janet/jimmy/vec.janet
removing /home/user/.local/lib/janet/jimmy/util.janet
removing /home/user/.local/lib/janet/jimmy/init.janet
removing manifest /home/user/.local/lib/janet/.manifests/jimmy.jdn
Uninstalled.

$ ls -a ~/.local/lib/janet/jimmy/
.  ..

Perhaps it has something to do with the use of :prefix in declare-source?

Lines from project.janet from jimmy:

(declare-source
  :source [
    "src/set.janet"
    "src/map.janet"
    "src/vec.janet"
    "src/util.janet"
    "src/init.janet"
  ]
  :prefix "jimmy")

Lines from project.janet from stx:

(declare-source
 :source ["src/init.janet"]
 :prefix "stx")

Janet: 1.30.0-2ac36a05
JPM: 65e218e
OS: Ubuntu Linux 22.0.x

[sogaiu]

At least in stx's case, there does not appear to be any mention of the stx directory itself in the manifest:

{:tag "eb6bedd643ad47caa9d3c0630c1679ca49484fdc"
 :paths @["/home/user/.local/lib/janet/stx/native.so"
          "/home/user/.local/lib/janet/stx/native.meta.janet"
          "/home/user/.local/lib/janet/stx/native.a"
          "/home/user/.local/lib/janet/stx/init.janet"]
  :type :git
  :url "https://github.com/ml-2/stx"
  :version "0.0.0" 
  :dependencies @[]}

Currently, it seems that uninstall leads to this code (slightly edited):

(defn uninstall
  "Uninstall bundle named name"
  [name]
  (def manifest (find-manifest name))
  (when-with [f (file/open manifest)]
    (def man (parse (:read f :all)))
    (each path (get man :paths [])
      (def path1 (string (dyn :dest-dir "") path))
      (print "removing " path1)
      (rm path1))
    (print "removing manifest " manifest)
    (:close f) # <censored (^^;>
    (rm manifest)
    (print "Uninstalled.")))

So, IIUC, things that are not explicitly mentioned in the manifest are not removed [1].

Does that seem right?

---

[1] Thanks to pyrmont for the pointer.

[pyrmont]

In my opinion there are three ways to deal with this problem.

1. Keep the status quo. Leave things as they are and only delete files that are in the manifest.

2. Record directory creation. If the manifest recorded the directories it created, it could also remove them.

3. Auto-delete empty directories. After deleting each path, check if the parent is empty and if it is, delete it, too.

My suggestion would be to adopt Option 3. Option 2 could lead to JPM deleting user data that is saved into a directory. Option 3 will only delete directories that are empty.

[sogaiu]

I think a slight variation on option 2 could be doable too, i.e. only remove recorded directories if they become empty...error otherwise.

Or, check whether there is anything in the relevant directories other than what's recorded in the manifest before removing anything? This way one doesn't end up with a partial installation perhaps.

I'm not coming up with a scenario of there being post-installation-user-created data under syspath, but that might just be my limited imagination (^^;

Not necessarily saying that I think an option 2-ish thing is better, just observing here.

[sogaiu]

This issue seems to now have a real-world consequence.

For repositories that might be trying to support both jpm as well as bundle/*-based approaches, the following sequence can cause an installation failure:

1. install via jpm

~/src/janet-ref$ jpm install
generating /home/user/.local/lib/janet/.manifests/janet-ref.jdn...
Installed as 'janet-ref'.
copying lib to /home/user/.local/lib/janet/janet-ref...
copying init.janet to /home/user/.local/lib/janet/janet-ref...
installing bin/jref to /home/user/.local/bin/jref

2. uninstall via jpm

~/src/janet-ref$ jpm uninstall
removing /home/user/.local/lib/janet/janet-ref/lib
removing /home/user/.local/lib/janet/janet-ref/init.janet
removing /home/user/.local/bin/jref
removing manifest /home/user/.local/lib/janet/.manifests/janet-ref.jdn
Uninstalled.

3. verify directory remains behind

$ ls -ald ~/.local/lib/janet/janet-ref
drwxrwxr-x 2 user user 4096 Sep 21 18:31 /home/user/.local/lib/janet/janet-ref

4. install via janet --install .

~/src/janet-ref$ janet --install .
running hook install for bundle janet-ref
add /home/user/.local/lib/janet/bin/jref
installation error, uninstalling
error: collision at /home/user/.local/lib/janet/janet-ref, directory already exists
  in errorf [boot.janet] on line 208, column 3
  in bundle/add-directory [boot.janet] on line 4453, column 7
  in install [/home/user/.local/lib/janet/bundle/janet-ref/init.janet] (tail call) on line 31, column 3
  in do-hook [boot.janet] on line 4226, column 5
  in edefer [boot.janet] on line 4374, column 7
  in bundle/install [boot.janet] on line 4355, column 5
  in <anonymous> [boot.janet] (tail call) on line 4705, column 18
  in cli-main [boot.janet] on line 4742, column 13

IIUC, the last step fails because the unremoved janet-ref directory from the previous installation contributes to a collision.

I think the steps above can be tried locally via this branch.

[sogaiu]

Possibly it's not a great approach, but the following patch to boot.janet seems to resolve the issue here:

diff --git a/src/boot/boot.janet b/src/boot/boot.janet
index 731f2359..b2f7e4b3 100644
--- a/src/boot/boot.janet
+++ b/src/boot/boot.janet
@@ -4449,8 +4449,10 @@
     (def files (get-files manifest))
     (def s (sep))
     (def absdest (string (dyn *syspath*) s dest))
-    (unless (os/mkdir absdest)
-      (errorf "collision at %s, directory already exists" absdest))
+    (os/mkdir absdest)
+    (assertf (and (= :directory (os/stat absdest :mode)))
+                  (empty? (os/dir absdest))
+             "collision at %s" absdest)
     (def absdest (os/realpath absdest))
     (array/push files absdest)
     (when chmod-mode

The idea here is to attempt creating a directory named by absdest, ignoring the return value of os/mkdir. Subsequently, we only proceed further if the expected directory exists and is empty.

Not sure if there are adverse consequences to this...

[sogaiu]

Something close to 3 as mentioned here might be ok.

Auto-delete empty directories. After deleting each path, check if the parent is empty and if it is, delete it, too.

• An additional check might be to verify that the directory under consideration is a child / descendent of syspath and not syspath itself.

• If unexpectedly there is a non-empty directory, perhaps there should be some warning that problems might lie ahead if the directory and its content remain in place after uninstallation.

Some code that seems relevant for uninstall appears to be [1]:

jpm/jpm/commands.janet

Lines 211 to 215 in d93b7c2

	(defn- uninstall-cmd
	[& what]
	(if (empty? what)
	(local-rule "uninstall")
	(each wha what (uninstall wha))))

jpm/jpm/declare.janet

Lines 42 to 55 in d93b7c2

	(defn uninstall
	"Uninstall bundle named name"
	[name]
	(def manifest (find-manifest name))
	(when-with [f (file/open manifest)]
	(def man (parse (:read f :all)))
	(each path (get man :paths [])
	(def path1 (string (dyn :dest-dir "") path))
	(print "removing " path1)
	(rm path1))
	(print "removing manifest " manifest)
	(:close f) # I hate windows
	(rm manifest)
	(print "Uninstalled.")))

rm's definition is:

jpm/jpm/shutil.janet

Lines 60 to 70 in d93b7c2

	(defn rm
	"Remove a directory and all sub directories."
	[path]
	(case (os/lstat path :mode)
	:directory (do
	(each subpath (os/dir path)
	(rm (string path "/" subpath)))
	(os/rmdir path))
	nil nil # do nothing if file does not exist
	# Default, try to remove
	(os/rm path)))

---

[1] A user-specified override of uninstall in project.janet seems like it could lead to availability issues...

[sogaiu]

Still might also be worth doing 2 from here as well:

Record directory creation. If the manifest recorded the directories it created, it could also remove them.

declare-source doesn't appear to record the prefix directory in the manifest:

jpm/jpm/declare.janet

Lines 177 to 188 in d93b7c2

	(defn declare-source
	"Create Janet modules. This does not actually build the module(s),
	but registers them for packaging and installation. :source should be an
	array of files and directores to copy into JANET_MODPATH or JANET_PATH.
	:prefix can optionally be given to modify the destination path to be
	(string JANET_PATH prefix source)."
	[&keys {:source sources :prefix prefix}]
	(def path (string (dyn:modpath) (if prefix "/") prefix))
	(if (bytes? sources)
	(install-rule sources path)
	(each s sources
	(install-rule s path))))

install-rule assumes a file or directory exists already, which doesn't hold for the case in question.

jpm/jpm/declare.janet

Lines 21 to 31 in d93b7c2

	(defn install-rule
	"Add install and uninstall rule for moving files from src into destdir."
	[src destdir]
	(unless (check-release) (break))
	(def name (last (peg/match path-splitter src)))
	(def path (string destdir "/" name))
	(array/push (dyn :installed-files) path)
	(def dir (string (dyn :dest-dir "") destdir))
	(task "install" []
	(os/mkdir dir)
	(copy src dir)))

I tried various invocations of install-rule but didn't get it working as desired.

---

An initial sketch at recording the :prefix directory in the manifest:

diff --git a/jpm/declare.janet b/jpm/declare.janet
index 24cde49..9009aed 100644
--- a/jpm/declare.janet
+++ b/jpm/declare.janet
@@ -185,7 +185,11 @@
   (if (bytes? sources)
     (install-rule sources path)
     (each s sources
-      (install-rule s path))))
+      (install-rule s path)))
+  (when prefix
+    (array/push (dyn :installed-files) path)
+    (task "install" []
+          (os/mkdir prefix))))

When prefix is truthy:

• make a note to record the prefix directory in the manifest via (dyn :installed-files), and
• arrange for the prefix directory to be created via task

Thanks to @pyrmont for spotting the order problem that was here originally 😅

---

For comparison, declare-source in spork's declare-cc.janet looks like this:

(defn declare-source
  "Create Janet modules. This does not actually build the module(s),
  but registers them for packaging and installation. :source should be an
  array of files and directores to copy into JANET_MODPATH or JANET_PATH.
  :prefix can optionally be given to modify the destination path to be
  (string JANET_PATH prefix source)."
  [&named source prefix]
  (defn dest [s] (def bn (path/basename s)) (if prefix (path/join prefix bn) bn))
  (def sources (if (bytes? source) [source] source))
  (when prefix
    (rule :pre-install []
          (def manifest (assert (dyn *install-manifest*)))
          (bundle/add-directory manifest prefix)))
  (each s source
    (install-rule s (dest s))))

---

Additional notes:

• Have not thought of a straight-forward way to somehow communicate information about the result of calling os/mkdir to the writing of the manifest. If os/mkdir were to fail (e.g. there is already a directory), should the manifest contain the directory in question?
• The changes above seem to work against the idea of having more than one project that shares the same :prefix (e.g. imagine some kind of plugin-like arrangement). Not sure if that's a real issue, but FWIW.

[sogaiu]

I did a bit of searching to get an idea of how much :prefix is being used with declare-source [1] [2].

---

Summary of Investigation

• There appear to be at least 49 projects that use this feature out of somewhat over 580 projects.

• No :prefix appears to be reused outside of a particular project...nor is any :prefix a "subpath" of any other across projects [2].

• One somewhat unique case is toolbox which has more than one declare-source + :prefix use where the multiple :prefixes are the same. This seems to be intentional. One consequence of this is that the :paths array value in the corresponding manifest ends up containing some duplicate values. Perhaps that's not a problem though.

• A somewhat "worrying" comment was spotted here:

  # Given its generic name, this prefix is happily "shared" with other projects
  :prefix "pure"
  

  It seems that at least one person had the idea that a particular :prefix might get shared across projects.

---

Reflections on the Current State of Things

Given that bundle/add-directory is currently implemented like this:

    (unless (os/mkdir absdest)
      (errorf "collision at %s, directory already exists" absdest))

may be the idea of :prefix-sharing across projects is not one that is currently intended. Perhaps jpm should reflect this as well.

An alternative approach might be for bundle/add-directory to relax its constraint and instead attempt to prevent overwriting of existing files by different projects under a common :prefix.

In any case, it seems to me that a consistent approach would be preferrable to the current mixed state of affairs.

---

Detailed Output of Search Invocation

The invocation run across local clones to identify some use was:

$ find . -name project.janet | xargs rg -A 5 \\\(declare-source | rg :prefix | rg -v toolbox/deps | sort

(The last bit filters out some bits that looked "vendored".)

./git.envs.net/iacore/janet-signal/project.janet-  :prefix "signal"
./github.com/agent-kilo/janetland/project.janet-                :prefix ((dyn :project) :name))
./github.com/CFiggers/jgraph/project.janet-  :prefix "jgraph"
./github.com/CFiggers/journo/project.janet-  :prefix "journo"
./github.com/chamaeleon/janet-plplot/project.janet-  :prefix "plplot"
./github.com/chris-chambers/effuse/project.janet-  :prefix "effuse"
./github.com/cideM/janet-hsluv/project.janet-                :prefix "hsluv/")
./github.com/dghaehre/gp-router/project.janet-  :prefix "gp-router"
./github.com/disruptek/jay/project.janet-  :prefix "jay"
./github.com/disruptek/slay-ir/project.janet-  :prefix "slay-ir"
./github.com/dlisboa/janet-slog/project.janet-  :prefix "slog"
./github.com/ianthehenry/banquet/project.janet-  :prefix "banquet"
./github.com/ianthehenry/bytemap/project.janet-  :prefix "bytemap"
./github.com/ianthehenry/cmd/project.janet-  :prefix "cmd"
./github.com/ianthehenry/cmp/project.janet-  :prefix "cmp"
./github.com/ianthehenry/heap/project.janet-  :prefix "heap"
./github.com/ianthehenry/judge/project.janet-  :prefix "judge"
./github.com/ianthehenry/pat/project.janet-  :prefix "pat"
./github.com/ianthehenry/square/project.janet-  :prefix "square"
./github.com/janet-lang/jpm/project.janet-  :prefix "jpm"
./github.com/JMinyard1335/Janet-rvars/project.janet-  :prefix "rvars"
./github.com/jsks/janet-sdbus/project.janet-  :prefix "sdbus"
./github.com/Luexa/jvs/project.janet-  :prefix "/jvs")
./github.com/meinside/httprequest-janet/project.janet-  :prefix "httprequest"
./github.com/meinside/telegram-bot-janet/project.janet-  :prefix "telegram-bot-janet"
./github.com/ml-2/stx/project.janet- :prefix "stx")
./github.com/pyrmont/arnie/project.janet-  :prefix "arnie")
./github.com/pyrmont/churlish/project.janet-  :prefix "churlish")
./github.com/pyrmont/documentarian/project.janet-  :prefix "documentarian")
./github.com/pyrmont/jeep/fixtures/doc/project.janet-  :prefix "example"
./github.com/pyrmont/medea/project.janet-  :prefix "medea")
./github.com/pyrmont/musty/project.janet-  :prefix "musty")
./github.com/pyrmont/remarkable/project.janet-  :prefix "remarkable")
./github.com/pyrmont/testament/project.janet-  :prefix "testament")
./github.com/pyrmont/tomlin/project.janet-  :prefix "tomlin")
./github.com/rokf/janet-datastar/project.janet-  :prefix "datastar"
./github.com/rokf/janet-jwt/project.janet-  :prefix "jwt"
./github.com/rokf/janet-resp/project.janet-  :prefix "resp"
./github.com/rokf/janet-rqlite/project.janet-  :prefix "rqlite"
./github.com/rokf/janet-tile38/project.janet-  :prefix "tile38"
./github.com/sbocq/janet-duckdb/project.janet-  :prefix "janet-duckdb"
./github.com/s-lambert/janet-game/project.janet-    :prefix "janet-game"
./github.com/Techcable/janet-json-pure/project.janet-  :prefix "pure"
./github.com/tionis/toolbox/project.janet-    :prefix "toolbox"
./github.com/tionis/toolbox/project.janet-    :prefix "toolbox"
./github.com/tionis/toolbox/project.janet-    :prefix "toolbox"
./git.sr.ht/~bakpakin/moogtool/project.janet-  :prefix "moogtool"
./git.sr.ht/~pepe/jff/project.janet-  :prefix "jff"
./notabug.org/debris/lllm-janet/project.janet- :prefix "lllm"

---

Footnotes

[1] The results might also help in seeing how the feature is being used (e.g. is anyone actually doing the "plug-in"-like thing described in the comment above).

[2] So far I've only come across the following docs regarding :prefix for declare-source (i.e. the docstring for declare-source):

Create Janet modules. This does not actually build the module(s),
but registers them for packaging and installation. :source should be an
array of files and directores to copy into JANET_MODPATH or JANET_PATH.
:prefix can optionally be given to modify the destination path to be
(string JANET_PATH prefix source).

This doesn't appear to make any promises about what happens if multiple projects use the same :prefix or in the case of two or more projects that have :prefixes where one is a "subpath" of the other (e.g. one project uses "spork" and another uses "spork/my-extensions"). I think I'd be happy with a "don't do that" or "use at your own peril" stance.