opam icon indicating copy to clipboard operation
opam copied to clipboard

Remove warning about spaces?

Open jonahbeckford opened this issue 3 years ago • 1 comments

I can submit a PR; this issue will be linked to it if approved. This is just a pre-check to make sure I'm not missing anything.

Both

https://github.com/ocaml/opam/blob/e229b8f1073b783afbb0537791ebe75c3a5e4d92/src/core/opamSystem.ml#L507-L509

and

https://github.com/ocaml/opam/blob/e229b8f1073b783afbb0537791ebe75c3a5e4d92/src/core/opamSystem.ml#L531-L532

complain if an Opam command has a space in it. On Windows spaces are common ... especially in people's usernames (ex. C:\Users\Alice Lake).

Obviously the warnings are there for a reason. I just don't know what the reason is.

I do some testing in Windows on a Vagrant virtual machine, and spaces seem to work in Opam. With some tweaks spaces mostly work in the broader OCaml ecosystem as well. Here is a log with the warning:

    en-US: Added 'PATH += "C:\\Users\\vagrant\\AppData\\Local\\Programs\\DISKUV~1\\0\\usr\\bin"' to field setenv in switch C:\vagrant\playground
    en-US: Set to '["C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe"]' the field wrap-build-commands in switch C:\vagrant\playground
    en-US: Set to '["C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe"]' the field wrap-install-commands in switch C:\vagrant\playground
    en-US: Set to '["C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe"]' the field wrap-remove-commands in switch C:\vagrant\playground
    en-US: Set to '[]' the field pre-build-commands in switch C:\vagrant\playground
    en-US: Set to '[]' the field post-install-commands in switch C:\vagrant\playground
    en-US: Set to '[]' the field pre-remove-commands in switch C:\vagrant\playground
    en-US: [NOTE] External dependency handling not supported for OS family 'windows'.
    en-US:        You can disable this check using 'opam option --global depext=false'
    en-US: The following actions will be performed:
    en-US: === install 5 packages
    en-US:   - install csexp             1.5.1 (pinned)      [required by dune-configurator]
    en-US:   - install dune              2.9.3+shim (pinned) [required by graphics]
    en-US:   - install dune-configurator 2.9.3 (pinned)      [required by graphics]
    en-US:   - install graphics          5.1.2 (pinned)
    en-US:   - install result            1.5                 [required by dune-configurator]
    en-US: 
    en-US: <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
    en-US: -> retrieved csexp.1.5.1  (cached)
    en-US: -> retrieved dune.2.9.3+shim, dune-configurator.2.9.3  (cached)
Warning: : [WARNING] Command "C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe" contains space characters
    en-US: -> retrieved result.1.5  (cached)
    en-US: -> retrieved graphics.5.1.2  (https://github.com/ocaml/graphics/releases/download/5.1.2/graphics-5.1.2.tbz)
Warning: : [WARNING] Command "C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe" contains space characters
Warning: : [WARNING] Command "C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe" contains space characters
Warning: : [WARNING] Command "C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe" contains space characters
    en-US: -> installed dune.2.9.3+shim
Warning: : [WARNING] Command "C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe" contains space characters
Warning: : [WARNING] Command "C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe" contains space characters
    en-US: -> installed csexp.1.5.1
    en-US: -> installed result.1.5
Warning: : [WARNING] Command "C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe" contains space characters
    en-US: -> installed dune-configurator.2.9.3
Warning: : [WARNING] Command "C:\\Users\\vagrant\\AppData\\Local\\Programs\\Diskuv WithSpace OCaml\\0\\dkml\\_opam\\bin\\with-dkml.exe" contains space characters
    en-US: -> installed graphics.5.1.2
    en-US: C:\vagrant\playground\_opam\.opam-switch\build\dune-configurator.2.9.3\test\blackbox-tests\test-cases - The directory is not empty.
    en-US: C:\vagrant\playground\_opam\.opam-switch\build\dune.2.9.3+shim\test\blackbox-tests\test-cases - The directory is not empty.
    en-US: Done.
    en-US: # Run (& opam env) -split '\r?\n' | ForEach-Object { Invoke-Expression $_ } to update the current shell environment

PS. On Windows one mitigation is to use DOS 8.3 paths. But that requires that DOS 8.3 paths are enabled. PPS. On macOS spaces are everywhere as well (see ls /Library).

jonahbeckford avatar Jun 15 '22 18:06 jonahbeckford

It's been there for a long time, well before Windows was being thought (added in https://github.com/ocaml/opam/commit/f3f6465b1fa406573917d46bc02f261f0906590f)! We should probably be conservative about turning the warning off universally, but I agree that it should certainly be suppressed for Windows.

I expect it was there because there was a risk of the command not being quoted properly when passed to shells, etc. It really shouldn't be an issue given the rewrites of the process engine in opam since early 0.x/1.0.x days (i.e. we never pass commands to the shell)

dra27 avatar Jul 08 '22 08:07 dra27