Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove warning about spaces? #5163

Closed
jonahbeckford opened this issue Jun 15, 2022 · 1 comment · Fixed by #5596
Closed

Remove warning about spaces? #5163

jonahbeckford opened this issue Jun 15, 2022 · 1 comment · Fixed by #5596
Assignees
Projects
Milestone

Comments

@jonahbeckford
Copy link
Contributor

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

opam/src/core/opamSystem.ml

Lines 507 to 509 in e229b8f

(* Check that the command doesn't contain whitespaces *)
if None <> try Some (String.index cmd ' ') with Not_found -> None then
OpamConsole.warning "Command %S contains space characters" cmd;

and

opam/src/core/opamSystem.ml

Lines 531 to 532 in e229b8f

if OpamStd.String.contains_char cmd ' ' then
OpamConsole.warning "Command %S contains space characters" cmd;

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).

@dra27
Copy link
Member

dra27 commented Jul 8, 2022

It's been there for a long time, well before Windows was being thought (added in f3f6465)! 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 dra27 added this to the 2.2.0~alpha milestone Jul 8, 2022
@dra27 dra27 added this to To do in Opam 2.2.0 via automation Jul 8, 2022
@dra27 dra27 self-assigned this Jul 11, 2022
@rjbou rjbou moved this from To do to After ~alpha; before release in Opam 2.2.0 Jun 7, 2023
@rjbou rjbou removed this from the 2.2.0~alpha milestone Jun 7, 2023
@rjbou rjbou added this to the 2.2.0~alpha2 milestone Jul 6, 2023
@rjbou rjbou moved this from After ~alpha; before release to PR to review for alpha2 in Opam 2.2.0 Jul 6, 2023
Opam 2.2.0 automation moved this from PR to review for alpha2 to Done Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Opam 2.2.0
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants