feat(install): add support for `--unsafe` install flag

[rustdevbtw]

closes #997 closes #991

This PR adds support for the --unsafe install flag to PKGX, which, as proposed by @jhheider and @felipecrs (thanks!), creates additional env stubs for a package on the first run, making subsequent runs make use of the already installed binaries, thus making it faster[1].

The stub

As for the stub, it makes it a bit different, for instance, here's a "safe" node wrapper (installed normally):

#!/bin/sh
if [ "$PKGX_UNINSTALL" != 1 ]; then
  exec pkgx +nodejs.org -- node "$@"
else
  cd "$(dirname "$0")"
  rm node && echo "uninstalled: nodejs.org" >&2
fi

And here's the "unsafe" (as in experimental) node wrapper:

 #!/bin/sh
+#MANAGED BY PKGX
 if [ "$PKGX_UNINSTALL" != 1 ]; then
-  exec pkgx +nodejs.org -- node "$@"
+  ARGS="$*"
+  ENV_FILE="${XDG_CACHE_DIR:-$HOME/.cache}/pkgx/envs/nodejs.org.env"
+  PKGX_DIR="${PKGX_DIR:-$HOME/.pkgx}"
+
+  pkgx_resolve() {
+    mkdir -p "$(dirname "$ENV_FILE")"
+    pkgx +nodejs.org 1>"$ENV_FILE"
+    run
+  }
+  run() {
+    if test -e "$ENV_FILE" && test -e "$PKGX_DIR/nodejs.org/v*/bin/node"; then
+      set -a
+      # shellcheck source=$ENV_FILE
+      . "$ENV_FILE"
+      set +a
+      exec "$PKGX_DIR/nodejs.org/v*/bin/node" "$ARGS"
+    else
+      pkgx_resolve
+    fi
+  }
+  run
 else
-  cd "$(dirname "$0")"
+  cd "$(dirname "$0")" || exit
   rm node && echo "uninstalled: nodejs.org" >&2
 fi

Edit: this version of the stub is outdated, and doesn't match the actual one implemented. Please refer to the code instead.

Additional changes

Additionally, this PR also makes the following behavioural changes to PKGX:

1. pkgx uninstall now also removes the env stub of the package
2. pkgx install now checks for both exec pkgx (old behaviour) and #MANAGED BY PKGX (comment) to determine whether that file was created by PKGX.

Usage

To do an unsafe installation, there are two options:

1. Use the --unsafe flag (e.g pkgx install --unsafe node)
2. Use the PKGX_UNSAFE_INSTALL environmental variable.

In the presence of both, the --unsafe flag takes precedence.

Caveats

This approach doesn't come without any caveat. It's necessary to document them upon having such a functionality. As per my knowledge, these are the known issues:

1. If the cache dir doesn't exist (after installing the package), it fails.

   Edit: it has been fixed

2. If the envs need updating at a later point, that has to be done separately.
3. If a certain dependency is removed, it doesn't properly check for that, and would error.
4. These stubs aren't 100% managed by PKGX (unless you remove the env, when they'll be updated by PKGX)

Most (if not all) of them error correctly and offer (potentially) helpful error messages.

[1]

[rustdevbtw]

Here is another benchmark, comparing node.unsafe (installed through --unsafe), node.safe (normally installed), pkgx node, and the system-installed (in this case, installed from Arch repos) node:

I believe (though I could be wrong) that the 1.1x diff is because of the shell overhead, and overhead of checking files.

[rustdevbtw]

Changed the mkdir -p (on the first run, or when the cache doesn't exist) to do that on $(dirname ~/.cache/pkgx/envs/gnu.org/emacs.env) so it creates the gnu.org dir also (the install also takes care of this, but if the cache is missing, it'll cause error, as mentioned in 1st caveat). It handles the first caveat.

[rustdevbtw]

@jhheider It seems to be okay, but I'm not in PC rn (it's 1:02AM here lol). Can you try it now please, if possible?

[jhheider]

@jhheider It seems to be okay, but I'm not in PC rn (it's 1:02AM here lol). Can you try it now please, if possible?

bugs in the script, but i can add a commit to fix them.

[jhheider]

but the speed looks good:

pkgx hyperfine "node --version"
Benchmark 1: node --version
  Time (mean ± σ):      26.4 ms ±   2.1 ms    [User: 18.8 ms, System: 3.5 ms]
  Range (min … max):    25.0 ms …  46.3 ms    103 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

[rustdevbtw]

@jhheider It seems to be okay, but I'm not in PC rn (it's 1:02AM here lol). Can you try it now please, if possible?

bugs in the script, but i can add a commit to fix them.

Thanks. Sucks to be without laptop/PC, lol. Anyways, thanks again.

[rustdevbtw]

but the speed looks good:

pkgx hyperfine "node --version"
Benchmark 1: node --version
  Time (mean ± σ):      26.4 ms ±   2.1 ms    [User: 18.8 ms, System: 3.5 ms]
  Range (min … max):    25.0 ms …  46.3 ms    103 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Is that right after doing the install, or after doing an initial run? It caches only at the first run, so subsequent runs should be faster than the first one. And the first one might interfere with the rest.

[jhheider]

You mean, the env should take priority?

No, I mean if either is set, it should be on. Which is the way you have it, I believe. Setting the envvar to zero shouldn't do anything.

[rustdevbtw]

You mean, the env should take priority?

No, I mean if either is set, it should be on. Which is the way you have it, I believe. Setting the envvar to zero shouldn't do anything.

By shouldn't do anything, do you mean it should ignore that? So, if that's set, regardless of its value, it should do an unsafe install?

[rustdevbtw]

You mean, the env should take priority?

No, I mean if either is set, it should be on. Which is the way you have it, I believe. Setting the envvar to zero shouldn't do anything.

By shouldn't do anything, do you mean it should ignore that? So, if that's set, regardless of its value, it should do an unsafe install?

If yes, it gets simpler:

function is_unsafe(unsafe: boolean): boolean {
    return Deno.env.get("PKGX_UNSAFE_INSTALL") || unsafe;
}

[jhheider]

So, if that's set, regardless of its value, it should do an unsafe install?

No. If it's non-zero, or the --unsafe flag is passed, it should do an unsafe install. If the envvar is zero, it has no effect one way or the other.

[rustdevbtw]

The default behaviour (without any env or flag) is to install normally. So, if the env is set to 0, it should do that? Sorry for sounding like an idiot.

On Wed, 15 May, 2024, 8:14 pm Jacob Heider, @.***> wrote:

So, if that's set, regardless of its value, it should do an unsafe install?

No. If it's non-zero, or the --unsafe flag is passed, it should do an unsafe install. If the envvar is zero, it has no effect one way or the other.

— Reply to this email directly, view it on GitHub https://github.com/pkgxdev/pkgx/pull/1008#issuecomment-2112741688, or unsubscribe https://github.com/notifications/unsubscribe-auth/ATXDOIZKEBPBKQ6ZCQFUVFDZCNYEJAVCNFSM6AAAAABHSDC4DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJSG42DCNRYHA . You are receiving this because you authored the thread.Message ID: @.***>

[jhheider]

The default behaviour (without any env or flag) is to install normally. So, if the env is set to 0, it should do that?

Exactly. Setting it 0 is the same as not setting it. So, it doesn't affect behavior.

If it's set to 0 and the flag is passed it's unsafe. If it's 0 and no flag, then safe. Set to 0 is a no-op.

[rustdevbtw]

The default behaviour (without any env or flag) is to install normally. So, if the env is set to 0, it should do that?

Exactly. Setting it 0 is the same as not setting it. So, it doesn't affect behavior.

If it's set to 0 and the flag is passed it's unsafe. If it's 0 and no flag, then safe. Set to 0 is a no-op.

So, this:

function is_unsafe(unsafe: boolean): boolean {
    const env = parseInt(Deno.env.get("PKGX_UNSAFE_INSTALL") || "0") ? true : false;
    return env || unsafe;
}

With this, there are six possible combinations:

1. ENV doesn't exist, but Flag is used == unsafe.
2. ENV doesn't exist, and Flag isn't used == normal.
3. ENV exists and is 1, but Flag isn't used == unsafe.
4. ENV exists and is 0, but Flag isn't used == normal.
5. ENV exists and is 1, and Flag is used == unsafe.
6. ENV exists and is 0, and Flag is used == unsafe.

[rustdevbtw]

@jhheider implemented it, is it okay? If so, can you please test that the env works as expected? (It probably does, but still a test is a good idea)

[jhheider]

right, i don't think that changed anything. your original implementation checked the truth value of the env || the flag. so, it should still be working correctly.

[rustdevbtw]

So @jhheider anything else that's left? (Maybe check if it's by PKGX part? We can improve that)

Also @mxcl can you please share thoughts?

[jhheider]

looks like good work

[felipecrs]

2. pkgx install now checks for pkgx (instead of exec pkgx) in a script to consider it as installed by PKGX (because unsafe installations don't use exec pkgx)

I've got to say this approach is just as not future-proof as the previous. Perhaps we should fix it for good by adding some known comment to the stubs? Like:

#!/bin/sh
# managed by pkgx

[felipecrs]

as proposed by @jhheider (thanks!)

Guys, I don't want to be silly, but I had proposed this on Apr 11. @jhheider is there a chance you missed my comment and later came up with the same proposal?

Don't get me wrong, all I want is pkgx to be improved. I'm asking for pure curiosity.

[rustdevbtw]

2. pkgx install now checks for pkgx (instead of exec pkgx) in a script to consider it as installed by PKGX (because unsafe installations don't use exec pkgx)

I've got to say this approach is just as not future-proof as the previous. Perhaps we should fix it for good by adding some known comment to the stubs? Like:

#!/bin/sh
# managed by pkgx

That is possible, but it'll break compatibility with the stubs installed by older versions. PKGX wouldn't detect them as installed by PKGX if we completely move to this approach. As for unsafe install, I've had the idea to see and check for .env files (generated for unsafe installations only). That's not as reliable, though, because it would fail if the cache was somehow deleted, or not yet generated.

[felipecrs]

That is possible, but it'll break compatibility with the stubs installed by older versions. PKGX wouldn't detect them as installed by PKGX if we completely move to this approach.

Perhaps we can keep searching for pkgx while still in pkgx 1.x, and maybe drop support for it in pkgx 2.x. Anyway, this can be done in another PR, no need to have it done here.

[jhheider]

as proposed by @jhheider (thanks!)

Guys, I don't want to be silly, but I had proposed this on Apr 11. @jhheider is there a chance you missed my comment and later came up with the same proposal?

Don't get me wrong, all I want is pkgx to be improved. I'm asking for pure curiosity.

Ha. I don't specifically recall your comment, but, yes, we were talking in discord about what and how to cache. I certainly don't deserve the credit for the idea, but we came to the same conclusion as you.

[rustdevbtw]

maybe i read that proposal before coming up with this PR, and forgot about it before doing it. it is certainly inspired by your proposal. so, i think you deserve the credit!

On Sat, Jun 8, 2024 at 1:42 AM Felipe Santos @.***> wrote:

as proposed by @jhheider (thanks!)

Guys, I don't want to be silly, but I had proposed this on Apr 11. @jhheider is there a chance you missed my comment and later came up with the same proposal?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[rustdevbtw]

That is possible, but it'll break compatibility with the stubs installed by older versions. PKGX wouldn't detect them as installed by PKGX if we completely move to this approach.

Perhaps we can keep searching for pkgx while still in pkgx 1.x, and maybe drop support for it in pkgx 2.x. Anyway, this can be done in another PR, no need to have it done here.

That sounds like a good option. What do you think @jhheider?

We can also add that to unsafe install now (which is in scope of this PR), because backwards compatibility doesn't exist for it.

[jhheider]

sure, sounds like a good idea.

[rustdevbtw]

does #MANAGED BY PKGX sound good for that comment? (it's unique enough to not cause conflicts)

[jhheider]

go for it!

[rustdevbtw]

go for it!

done!

[rustdevbtw]

Also, updated the example to reflect the current diff (as of now)

[coveralls]

Pull Request Test Coverage Report for Build 10740267243

Details

• 2 of 5 (40.0%) changed or added relevant lines in 1 file are covered.
• 10 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-1.5%) to 91.389%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parse-args.ts	2	5	40.0%
<!--	Total:	2	5	40.0%	-->

Files with Coverage Reduction	New Missed Lines	%
src/utils/execve.ts	10	81.72%
<!--	Total:	10		-->

Totals
Change from base Build 10740131780:	-1.5%
Covered Lines:	1475
Relevant Lines:	1603

---

💛 - Coveralls

[mxcl]

Nice feature. Additive so easy merge.