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

rustdevbtw · 2024-05-11T17:52:46Z

closes #997
closes #991

This PR adds support for the --unsafe install flag to PKGX, which, as proposed by @jhheider (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
if [ "$PKGX_UNINSTALL" != 1 ]; then
-  exec pkgx +nodejs.org -- node "$@"
+  ARGS="$@"
+  pkgx_resolve() {
+    mkdir -p "${XDG_CACHE_DIR:-$HOME/.cache}/pkgx/envs"
+    pkgx +nodejs.org 1>"${XDG_CACHE_DIR:-$HOME/.cache}/pkgx/envs/nodejs.org.env"
+    run
+  }
+  run() {
+    if [[ -e "${XDG_CACHE_DIR:-$HOME/.cache}/pkgx/envs/nodejs.org.env" && -e "${PKGX_HOME:-$HOME/.pkgx}/nodejs.org/v*/bin/node" ]]; then
+      set -a
+      source "${XDG_CACHE_DIR:-$HOME/.cache}/pkgx/envs/nodejs.org.env"
+      exec "${PKGX_HOME:-$HOME/.pkgx}/nodejs.org/v*/bin/node" "$ARGS"
+    else
+      pkgx_resolve
+    fi
+  }
+  run
else
  cd "$(dirname "$0")"
  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:

pkgx uninstall now also removes the env stub of the package
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)

Usage

To do an unsafe installation, there are two options:

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

In the presence of either, 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:

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

Edit: it has been fixed

If the envs need updating at a later point, that has to be done separately.
If a certain dependency is removed, it doesn't properly check for that, and would error.
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]

Signed-off-by: Rajdeep Malakar <rajdeepm.dev@gmail.com>

rustdevbtw · 2024-05-11T18:56:16Z

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.

…ts nested package caches from failing)

rustdevbtw · 2024-05-12T16:01:43Z

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.

src/modes/install.ts

jhheider · 2024-05-14T17:33:23Z

src/modes/install.ts

-            const found = value.match(/^\s*exec pkgx \+([^ ]+)/)?.[1]
+            const found = value.match(/^\s*pkgx \+([^ ]+)/)?.[1]


this might be dangerous, since it would catch a script with #!/usr/bin/env pkgx +python ...

Is there some other way of detecting both normal and unsafe packages? We can use special regex to match maybe both exec pkgx OR some other unsafe-specific (non-pkgx) match?

in this instance. at least, you could search for ${pkgstr}.env. that should be unique to your shim.

So, stat ~/.local/cache/pkgx/envs/${pkgstr}.env, and if it exists, and that has pkgx in it, consider it to be installed via PKGX?

What if instead, unsafe installations contain a specific line (like PKGX_UNSAFE=1) and check that? Unlike safe (normal) installations, we have full control over how the scripts are gonna be, and there's no backwards compatibility requirement.

i meant that the script should contain the string ${pkgstr}.env, since they source it.

Yeah, that could be done. Probably with regex as well.

src/modes/uninstall.ts

jhheider · 2024-05-14T17:34:44Z

src/parse-args.ts

@@ -89,6 +91,9 @@ export default function(input: string[]): Args {
      case 'update':
        flags.update = true
        break
+      case 'unsafe':
+        flags.unsafe = true


you could simplify ENV handling by moving it here; i don't know if we do that otherwise (as with, say, verbosity or debug).

No other option uses ENV yet. So, it would have been overkill, probably. Maybe I can follow-up with a PR adding ENV options for other flags (PKGX_VERBOSITY etc.)?

worth starting a discussion about, definitely. experience teaches us that there are sometimes unwritten design principles at play; though, in general, providing for either flag or env control allows a wider variety of use-cases.

Okay, so we should prioritise flags, right? So, if a flag is unset, only then it'll check the env. If that's fine, I'll do a PR for that after this one.

i think either being set means we do it.

What about PKGX_UNSAFE_INSTAL=0? It means false, so we shouldn't consider it to be set. And if it's this, we should check for the other.

I would think if either --unsafe or PKGX_UNSAFE_INSTALL != 0, then we do an unsafe install. PKGX_UNSAFE_INSTALL=0 should basically do nothing.

You mean, the env should take priority?

Then, it needs to be like:

function is_unsafe(unsafe: boolean): boolean { const env = Deno.env.get("PKGX_UNSAFE_INSTALL"); if (env) { // If present, it takes priority return parseInt(env) ? true : false; } else { // Otherwise, check flag return unsafe; } }

rustdevbtw · 2024-05-14T19:32:54Z

@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 · 2024-05-14T19:44:14Z

@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 · 2024-05-14T19:45:10Z

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 · 2024-05-14T19:46:40Z

@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 · 2024-05-14T19:47:48Z

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.

src/modes/install.ts

jhheider · 2024-05-15T14:38:06Z

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 · 2024-05-15T14:39:52Z

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 · 2024-05-15T14:42:36Z

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 · 2024-05-15T14:43:58Z

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 · 2024-05-15T14:45:46Z

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 <#1008 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ATXDOIZKEBPBKQ6ZCQFUVFDZCNYEJAVCNFSM6AAAAABHSDC4DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJSG42DCNRYHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

jhheider · 2024-05-15T14:48:11Z

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 · 2024-05-15T16:52:02Z

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:

ENV doesn't exist, but Flag is used == unsafe.
ENV doesn't exist, and Flag isn't used == normal.
ENV exists and is 1, but Flag isn't used == unsafe.
ENV exists and is 0, but Flag isn't used == normal.
ENV exists and is 1, and Flag is used == unsafe.
ENV exists and is 0, and Flag is used == unsafe.

rustdevbtw · 2024-05-15T19:32:42Z

@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 · 2024-05-16T16:39:41Z

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 · 2024-05-17T19:44:30Z

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 · 2024-05-17T21:52:18Z

looks like good work

rustdevbtw added 3 commits May 11, 2024 23:03

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

7235159

Signed-off-by: Rajdeep Malakar <rajdeepm.dev@gmail.com>

forgot to remove debug

ae5fa3a

--unsafe takes precedence, not the env

132605a

Signed-off-by: Rajdeep Malakar <rajdeepm.dev@gmail.com>

dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 11, 2024

rustdevbtw added 2 commits May 11, 2024 23:24

let's just warn if deleting cache fails

31d63d6

Signed-off-by: Rajdeep Malakar <rajdeepm.dev@gmail.com>

use pre-parsed unsafe flag

cae08c4

Signed-off-by: Rajdeep Malakar <rajdeepm.dev@gmail.com>

do mkdir -p on the expected dir instead of just the cache one (preven…

09b1e76

…ts nested package caches from failing)

jhheider reviewed May 14, 2024

View reviewed changes

rustdevbtw added 2 commits May 15, 2024 00:56

improve it

b8dd655

use Config and Path hooks

b2c483c

fix script invocations

8dc97eb

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels May 14, 2024

jhheider added 2 commits May 14, 2024 16:09

linter making more work

963bce6

a few more

a5ab5db

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels May 14, 2024

jhheider reviewed May 14, 2024

View reviewed changes

src/modes/install.ts Outdated Show resolved Hide resolved

jhheider and others added 3 commits May 14, 2024 16:13

last one

b39b3c3

Use .slice() instead of .pop()

d3f2779

Use .slice() instead of .pop(), in uninstall too

5366c08

rustdevbtw added 2 commits May 15, 2024 23:45

handle PKGX_UNSAFE_INSTALL=0 env

322d14d

fix it

22f7a97

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rustdevbtw commented May 11, 2024 •

edited

rustdevbtw commented May 11, 2024

rustdevbtw commented May 12, 2024 •

edited

jhheider May 14, 2024

rustdevbtw May 14, 2024

jhheider May 14, 2024

rustdevbtw May 14, 2024

rustdevbtw May 14, 2024

jhheider May 14, 2024

rustdevbtw May 15, 2024

jhheider May 14, 2024

rustdevbtw May 14, 2024

jhheider May 14, 2024

rustdevbtw May 14, 2024

jhheider May 14, 2024

rustdevbtw May 15, 2024

jhheider May 15, 2024

rustdevbtw May 15, 2024 •

edited

rustdevbtw commented May 14, 2024

jhheider commented May 14, 2024

jhheider commented May 14, 2024

rustdevbtw commented May 14, 2024

rustdevbtw commented May 14, 2024

jhheider commented May 15, 2024

rustdevbtw commented May 15, 2024

rustdevbtw commented May 15, 2024

jhheider commented May 15, 2024

rustdevbtw commented May 15, 2024 via email

jhheider commented May 15, 2024 •

edited

rustdevbtw commented May 15, 2024 •

edited

rustdevbtw commented May 15, 2024

jhheider commented May 16, 2024

rustdevbtw commented May 17, 2024

jhheider commented May 17, 2024

		const found = value.match(/^\s*exec pkgx \+([^ ]+)/)?.[1]
		const found = value.match(/^\s*pkgx \+([^ ]+)/)?.[1]

feat(install): add support for --unsafe install flag #1008

Are you sure you want to change the base?

feat(install): add support for --unsafe install flag #1008

Conversation

rustdevbtw commented May 11, 2024 • edited

The stub

Additional changes

Usage

Caveats

rustdevbtw commented May 11, 2024

rustdevbtw commented May 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustdevbtw May 15, 2024 • edited

Choose a reason for hiding this comment

rustdevbtw commented May 14, 2024

jhheider commented May 14, 2024

jhheider commented May 14, 2024

rustdevbtw commented May 14, 2024

rustdevbtw commented May 14, 2024

jhheider commented May 15, 2024

rustdevbtw commented May 15, 2024

rustdevbtw commented May 15, 2024

jhheider commented May 15, 2024

rustdevbtw commented May 15, 2024 via email

jhheider commented May 15, 2024 • edited

rustdevbtw commented May 15, 2024 • edited

rustdevbtw commented May 15, 2024

jhheider commented May 16, 2024

rustdevbtw commented May 17, 2024

jhheider commented May 17, 2024

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

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

rustdevbtw commented May 11, 2024 •

edited

rustdevbtw commented May 12, 2024 •

edited

rustdevbtw May 15, 2024 •

edited

jhheider commented May 15, 2024 •

edited

rustdevbtw commented May 15, 2024 •

edited