Support global and local vendor completions

[jcgruenhage]

Description

This adds both a global/system vendor completions dir, and a local one. The global one is primarily meant for the use with system package managers, such as the ones used by Linux distributions. The local one is specific to the user, and can be used for automatically including completions installed locally by the user.

The global/system vendor completions dir can be overridden at both compile time and run time, the local user one only at run time, as I don't see why a distribution would want to override that.

Fixes #11337

User-Facing Changes

Not sure how much users should see of this at all. This is primarily so that completions magically work when packages ship with them. If anyone has any ideas of how to document this properly for users, please, contribute that bit as well.

Tests + Formatting

I haven't added any new tests here, and I don't want to either. I've got quite limited time at the moment and spent way too much of it on this already, so if someone else could take over this bit, I'd be happy about that as well.

I have run toolkit check pr on my changes though, and that was happy.

[fdncred]

We should probably limit the size of this env var name. Environment space isn't infinite in Windows.

[jcgruenhage]

Any concrete suggestions? I agree that it's a bit long, but not long enough to actually make a difference when it comes to limits like that.

[jcgruenhage]

Github marked this review as outdated, but the conversation itself here is not resolved, the var name is still as it was before.

[fdncred]

My simplification would be NU_COMPLETION_DIR since we have precedence with NU vs NUSHELL

[jcgruenhage]

That wouldn't allow for differentiating between the global dir and the user dir. What about NU_COMPLETIONS_DIR for the user one, and NU_VENDOR_COMPLETIONS_DIR for the one managed by the system package manager?

[fdncred]

What is the point of having a VENDOR one and one without VENDOR? Why do we care, this is just a place to put completions. We don't have that convention in nushell for any other config files. I'm not sure why we'd start with this?

[amtoine]

why not a NU_COMPLETION_DIRS: list<path>? similar to NU_LIB_DIRS for modules

[fdncred]

I'd be ok with that too @amtoine. Then we just add a couple of default folders and make the default_config.nu match.

[amtoine]

yup, exactly 😋

from my experience, $env.NU_LIB_DIRS and $env.NU_PLUGIN_DIRS are pretty useful and practical objects and notions 👌

[jcgruenhage]

I can see how that might be useful, but that also means that if a distribution changes the place where they place their completions and a user has overridden that var while including that folder, they will end up breaking it. I'd therefore be inclined to keep those separated.

If someone wants to modify this so that additionally to the system one a user one a list instead of a single directory, I'd prefer if that was done by someone else in a follow-up PR, to keep this one simpler.

[fdncred]

Thanks. This is pretty good. Just a few minor issues mentioned above. We'll also need to see what others think about it.

[amtoine]

i think it would be great to add to default-env.nu a line like

$env.NUSHELL_GLOBAL_VENDOR_COMPLETIONS_DIR = ...

to show the default 😋

[jcgruenhage]

With the winapi usage and such on Windows, I'm not quite sure whether this works on Windows anymore now, so we'd definitely someone to test this. If no one else is available to do so, I might be able to test this tonight at the hackspace, but if someone else already has a nushell dev environment available on a windows machine, I'd appreciate it if I didn't have to test it myself.

[jcgruenhage]

Tested on windows too, works fine there as well (after a few little changes, which I've just pushed).

it appears requested changes have been implemented

[jcgruenhage]

Anything still needed here?

[fdncred]

I thought it was ready but remembered these items.

1. I don't believe we want to set env vars like this except in tests.

std::env::var("NU_VENDOR_COMPLETIONS_DIR")

I believe we want to use the built-in nushell functions. I think that's stack.add_env_var() but @kubouch would probably know best.
2. I'm still not a fan of having two separate env vars NU_VENDOR_COMPLETIONS_DIR and NU_COMPLETIONS_DIR.
3. It should probably also be plural and be utilized in a similar fashion as NU_LIB_DIRS so you can have a list of directories if needed.

We really need more eyes on this.

[kubouch]

Darren's right, std::env should be avoided at all costs, we manage the environment internally inside Nushell and only use std::env when interfacing with external processes.

However, see my comment in #11337. I think we should first think about the design.

[fdncred]

I think Jakub's $env.NU_AUTOLOAD_DIRS idea sounds reasonable. My only question is does it open the end user up to some type of attack? I'm sure things are auto loaded in bash/zsh/fish/pwsh etc too but I'm not sure how they work or defend against malicious code.

[jcgruenhage]

My only question is does it open the end user up to some type of attack? I'm sure things are auto loaded in bash/zsh/fish/pwsh etc too but I'm not sure how they work or defend against malicious code.

That's also addressed by #11337 (comment):

The autoload is there to avoid restarting Nushell when you install new completions (you just call autoload in the REPL), and you can comment it out from the config to avoid autoloading anything if you don't want it (e.g., security concerns).

I don't think that security concerns here are a valid concern: If you're concerned about the security of a piece of software, you shouldn't install it. If you want to try/test it, do so in a sandboxed environment.

One concern I still have is this bit:

Package maintainers would set $env.NU_AUTOLOAD_DIRS to whatever path they use to distribute completions. This assumes that package maintainers can set some system-wide environment variables on package installations, not sure if that's the case.

There should be a default for this, and the distributors should be able to override that during compile time. This default needs to be platform specific. If that needs to be overridden during runtime, it should happen in env.nu for consistencies sake.

With all that said: Is someone interested in implementing that? I think not much from this can be re-used for the suggested design, and I don't see myself having time to implement this myself.

[kubouch]

I'm not a big fan of the compile-time configuration because instead of one Nushell binary, we'd have an array of potentially incompatible Nushell binaries. For example, the Nushell binaries downloaded from our releases / cargo install / compiled by yourself wouldn't have this distro-specific patch and might behave differently, leading to surprises (“Why are my completions suddenly not working?”). If it's kept only as a runtime config, the same Nushell binary works on every system identically.

What is the problem of keeping NU_AUTOLOAD_DIRS as a runtime-only option (perhaps with a default value, but not compile-configurable)? Is setting an environment variable out of question for package maintainers?

[jcgruenhage]

Setting an environment variable during compile time is easier than setting it at runtime. There is no default way to ensure a binary is invoked with a given environment variable. The closest there is would be /etc/profile and /etc/profile.d, but these are shell scripts that nushell doesn't understand. That's a separate issue IMO, because it means that I don't run nu directly: I'm spawning a posix compliant shell, sourcing /etc/profile, which in turn sources /etc/profile.d/*, and afterwards start nushell to ensure my environment is properly populated.

The other point is: There are platform specific defaults that need to be set here, %PROGRAM_DATA%\nushell\autoload on Windows, and /usr/share/nushell/autoload on MacOS/Linux. The latter is easy, but the former is a bit more complex to properly resolve in pure nushell. How would that work with NU_AUTOLOAD_DIRS being defined purely in env.nu/config.nu?

And regarding "If it's kept only as a runtime config, the same Nushell binary works on every system identically.": This assumes that the systems are identical. There are Linux Distributions which take a vastly different approach to the filesystem hierarchy. By not allowing them to override this at a compile time, you're making nushell increasingly hard to package. Not providing a compile time override won't stop distros from changing the default, you'll just make it harder for them to do so.

[jcgruenhage]

Closing after discussions here and in #11337