Set environment variables needed for bash, zsh completion in opam env

[WardBrian]

Would close #6427

This updates opam env to set XDG_DATA_DIRS (from the XDG base directory specification) to include the current switch's share root, and FPATH to include %{share}%/zsh/site-functions when the shell is zsh

The former works with bash-completion, which looks in the XDG_DATA_DIRS entries for bash-completion/completions sub-directories, while the later sets up zsh's built-in completion mechanisms to look in this location.

Questions for reviewers --

1. I currently only modify FPATH on zsh, but I think it would be safe to do it generically (the only other shell that seems to assign meaning to the FPATH variable is ksh, which opam doesn't support)
2. Assuming we want to keep FPATH zsh-specific, is it worth threading through a shell variable all the way from the CLI to where we compute these updates, or is it fine to keep the guess_shell_compat call?

[kit-ty-kate]

cc @hannesm @reynir do you have any thoughts about the security considerations of this feature by any chance?

[hannesm]

@kit-ty-kate thanks for asking, no I don't have an idea about this environment variable (and whether to prepend or append to it).

[reynir]

I don't know much about it. For bash-completion I suspect it could add completion scripts from installed opam packages, and I think those can run arbitrary commands. Then again I guess packages can already install arbitrary binaries shadowing common binaries such as ls. I'm not sure what the security model is or how that fits into that.

[WardBrian]

Even if you're sandboxing the build commands (so you can't be pwned by arbitrary badness hidden behind a make command), I still think it is currently the case that you can't install an opam package unless you trust it. It doesn't even have to shadow something like ls; presumably you're going to run the code from the package eventually, or else you wouldn't be installing it.

[dbuenzli]

Then again I guess packages can already install arbitrary binaries shadowing common binaries such as ls

Not to mention that arbitrary ocaml packages can actually set or extend theses variables themselves…

[WardBrian]

Not to mention that arbitrary ocaml packages can actually set or extend theses variables themselves…

Yes. If this is decided out of scope for the opam tool, possibly the next best option would be a conf-completion package in the opam repository which contains just these two variables as setenv fields. Then tools could depend on that to get their users environments correctly configured for autocomplete.

That feels like a bit of a hack compared to doing it in code here.

[WardBrian]

The failing windows reftests are fairly inscrutable to me, and I'm extra confused as to why only some configurations are failing in that matrix

[kit-ty-kate]

The failing windows reftests are fairly inscrutable to me, and I'm extra confused as to why only some configurations are failing in that matrix

all Native configurations of windows fail. The only "windows" configurations that work fine are the ones using the cygwin backend of ocaml, which mains that it simply works like unix does.

The error is:

diff --git a/_build/default/tests/reftests/env-idempotent.test b/_build/default/tests/reftests/env-idempotent.out
index cb5a13d..5d23c8f 100644
--- a/_build/default/tests/reftests/env-idempotent.test
+++ b/_build/default/tests/reftests/env-idempotent.out
@@ -34,4 +34,3 @@ User configuration:
 ### rm -rf root/fake2
 ### bash bash-lc.sh "'$OPAMBIN' exec --root root --switch=fake -- env" | grep -v MANPATH | grep /root/fake2/ | '=.*' -> ''
 PATH
-XDG_DATA_DIRS
File "tests/reftests/env-idempotent.test", line 1, characters 0-0:
"C:\Program Files\Git\cmd\git.exe" --no-pager diff --no-index --color=always -u --ignore-cr-at-eol _build/default/tests/reftests/env-idempotent.test _build/default/tests/reftests/env-idempotent.out
Command exited with code 1.
diff --git a/_build/default/tests/reftests/env.win32.test b/_build/default/tests/reftests/env.win32.out
index 7dee3d0..511d6ed 100644
--- a/_build/default/tests/reftests/env.win32.test
+++ b/_build/default/tests/reftests/env.win32.out
@@ -811,7 +811,17 @@ Processing  2/3: [all-formulae: sh env | grep RAF_ENV | sort]
 [ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ("host" true | "target-quoted" true). Using default ';' 'target'.
 [ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ":" true | ";" true. Using default ';' 'target'.
 [ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ("host" true | "target-quoted" true). Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ":" true | ";" true. Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ("host" true | "target-quoted" true). Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ":" true | ";" true. Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ("host" true | "target-quoted" true). Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ":" true | ";" true. Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ("host" true | "target-quoted" true). Using default ';' 'target'.
 Done.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ":" true | ";" true. Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ("host" true | "target-quoted" true). Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ":" true | ";" true. Using default ';' 'target'.
+[ERROR] Formula can't be completely resolved : RAF_ENVSET_UNRES ("host" true | "target-quoted" true). Using default ';' 'target'.
 ### opam env | sort | grep "RAF_ENV"
 RAF_ENVSET_ATOM='\is\atom'; export RAF_ENVSET_ATOM;
 RAF_ENVSET_DBL='sec/ond;fir/st'; export RAF_ENVSET_DBL;

The change in env.win32.test is straightforward since you've done the same change in env.unix.test. Since you've only ran the testsuite on Unix, only the tests that work on Unix were ran and the ones that only run on Windows were skipped. So to fix that you can either run the testsuite on Windows again or simply trust the CI and apply that patch.

Now for env-idempotent.test, i'm not exactly sure, it depends what that test was supposed to show, but it could probably be fixed by adding another | grep -v XDG_DATA_DIRS to the command

[WardBrian]

Now for env-idempotent.test, i'm not exactly sure, it depends what that test was supposed to show

I am not entirely sure myself, I tried digging through the git history but also came up empty handed. Since it's already filtering out manpath, an extra filter doesn't seem so bad