'opam env' for directories with spaces

[jonahbeckford]

• Please update master_changes.md file with your changes.

Fixes #5204 .

It was not just a Windows problem, although only macOS of the *nix variants commonly uses spaces in their directory names.

[kit-ty-kate]

LGTM

[jonahbeckford]

Err... DOS Command Prompt needs double-quotes. Sending a commit once I've built locally and tested it again.

[jonahbeckford]

All fixed.

[dra27]

Would it be OK - as with the set commands etc in opam env to do this only when it's necessary? i.e. to display --switch=foo rather than --switch='foo').

More importantly, I'd like to double-check the handling if Windows opam is called by another executable for --switch="foo" ... on the Unix shell that gets passed as the string --switch=foo in argv, but on Windows it's actually two parameters... I've got a feeling that's cmd-specific and I can't remember what cmdliner does if it actually receives the literal string --switch="foo" (the solution otherwise is to advice "--switch=foo")

[dbuenzli]

on the Unix shell that gets passed as the string --switch=foo in argv,

But that must be your shell removing the quotes.

I can't remember what cmdliner does if it actually receives the literal string --switch="foo" (the solution otherwise is to advice "--switch=foo")

That depends on your argument converter. But if you use Arg.string you get "foo" of course. Everything after the = is the option's argument.

[rjbou]

updated with a test

[jonahbeckford]

The latest commit:

1. Places the quotes around the entire argument. The examples I tested are:

• for /f "tokens=*" %i in ('opam env "--switch=C:\Person 1"') do @%i in Command Prompt
• eval $(opam env '--switch=C:\Person 1') in Bash
• (& opam env "--switch=C:\Person 1") -split '\r?\n' | ForEach-Object { Invoke-Expression $_ } in PowerShell

2. Uses quotes only when quotes are needed. An example is eval $(opam env --switch=default) in the GitHub PR CI: https://github.com/ocaml/opam/runs/7545056295?check_suite_focus=true#step:8:100

[kit-ty-kate]

Suggested change

	### opam env --sw "./$SW" | grep OPAM_SWITCH_PREFIX | ';' -> ':'
	### opam env --sw "./$SW" | grep OPAM_SWITCH_PREFIX | ';' -> ':' | '\\_opam\'' -> '/_opam\''

[rjbou]

This should be handled by test engine, see

opam/tests/reftests/env.test

Lines 15 to 16 in 35899ba

	### opam env | grep "NV_VARS" | ';' -> ':'
	NV_VARS='${BASEDIR}/OPAM/setenv/doc/nv:${BASEDIR}/OPAM/setenv/share/nv': export NV_VARS:

It passes windows ci too

[rjbou]

Anyway, this last test is not required for this PR (no extra escaping done in it)

[kit-ty-kate]

ah, then I don’t know how to fix this properly

[kit-ty-kate]

Thanks a lot!