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

Bug ~ quoting? failure on windows-platform #296

Closed
rivy opened this issue Jun 2, 2020 · 10 comments · Fixed by #678
Closed

Bug ~ quoting? failure on windows-platform #296

rivy opened this issue Jun 2, 2020 · 10 comments · Fixed by #678
Labels
help wanted Extra attention is needed windows

Comments

@rivy
Copy link

rivy commented Jun 2, 2020

In example, for bat:

C:\>bat.exe --paging=always --pager="cmd.exe /c more.com" README.md
       │ File: README.md
   1   │ uutils coreutils
   2   │ ================
   3   │
   4   │ [![Discord](https://img.shields.io/badge/discord-join-7289DA.svg?logo=discord&longCache=true&style=flat)](https://discord.
       │ gg/wQVJbvJ)
   5   │ [![License](http://img.shields.io/badge/license-MIT-blue.svg)](https://github.com/uutils/coreutils/blob/master/LICENSE)
   6   │ [![FOSSA Status](https://app.fossa.io/api/projects/git%2Bgithub.com%2Fuutils%2Fcoreutils.svg?type=shield)](https://app.fos
       │ sa.io/projects/git%2Bgithub.com%2Fuutils%2Fcoreutils?ref=badge_shield)
   7   │ [![LOC](https://tokei.rs/b1/github/uutils/coreutils?category=code)](https://github.com/Aaronepower/tokei)
   8   │ [![dependency status](https://deps.rs/repo/github/uutils/coreutils/status.svg)](https://deps.rs/repo/github/uutils/coreuti
       │ ls)
   9   │
  10   │ [![Build Status](https://api.travis-ci.org/uutils/coreutils.svg?branch=master)](https://travis-ci.org/uutils/coreutils)
  11   │ [![Build Status (Windows)](https://ci.appveyor.com/api/projects/status/787ltcxgy86r20le?svg=true)](https://ci.appveyor.com
       │ /project/Arcterus/coreutils)
  12   │ [![Build Status (FreeBSD)](https://api.cirrus-ci.com/github/uutils/coreutils.svg)](https://cirrus-ci.com/github/uutils/cor
       │ eutils/master)

When testing with hyperfine:

C:\>hyperfine "bat.exe --paging=always --pager=\"cmd.exe /c more.com\" README.md"
Benchmark #1: bat.exe --paging=always --pager="cmd.exe /c more.com" README.md                                                     0
Error: Command terminated with non-zero exit code. Use the '-i'/'--ignore-failure' option if you want to ignore this. Alternatively, use the '--show-output' option to debug what went wrong.

but, using a command-string without internal quoting (by passing arguments to bat via the environment):

env BAT_PAGER="cmd.exe /c more.com" hyperfine "bat.exe --paging=always README.md"
Benchmark #1: bat.exe --paging=always README.md                                                                                   0
  Time (mean ± σ):     199.9 ms ±  51.9 ms    [User: 10.3 ms, System: 6.8 ms]                                                      0
  Range (min … max):   179.8 ms … 347.5 ms    10 runs

  Warning: The first benchmarking run for this command was significantly slower than the rest (347.5 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.

The text of the target command shown by hyperfine looks correct, but the target is not being executed as displayed.

This is likely a shell quoting issue.

@sharkdp
Copy link
Owner

sharkdp commented Jun 5, 2020

Thank you for reporting this.

So what are the actual quoting/escaping rules for cmd.exe? I seem to remember that they are highly non-intuitive (for a Unix user). Can

"bat.exe --paging=always --pager=\"cmd.exe /c more.com\" README.md"

be passed to other programs without any problem?

@rivy
Copy link
Author

rivy commented Jun 7, 2020

You're correct; parsing command lines on Windows is complicated and quirky.

And the rules changed in 2008.

David Deley has a good explanation of the various rules and changes (with examples) "How Command Line Parameters Are Parsed".

If you're handing the command line off to cmd, you should be able to just mechanically escape every character with ^ (eg, cmd.exe /d/c ^e^c^h^o ^"^t^h^i^s^ ^>^ ^w^o^r^k^s^").

If you're handing off directly to another windows process, I think it's ok to assume the 2008+ parsing behavior, except that rust is currently using the older, pre-2008, parsing behavior (which is extra "quirky"). But it does look like that is likely to change. I'm sure there must be mechanical translation recipe that would work for the direct process approach, but I haven't been able to create or find it.

Command is struggling with this as well.

That's why I fell back to cmd.exe /d/c ... for env in coreutils.

@sharkdp
Copy link
Owner

sharkdp commented Jun 8, 2020

Thank you for the detailed explanation and for all the references, especially "How Command Line Parameters Are Parsed". I didn't know it was that complicated 😦

I'm inclined to accept the cmd.exe /d/c ^…^… workaround for hyperfine, if you think that this would solve most issues.

Alternatively, would it help to use PowerShell instead of cmd.exe? This could be easily tested via hyperfines --shell option (if PowerShell also uses the syntax /c <cmd>).

@rivy
Copy link
Author

rivy commented Jun 9, 2020

Alternatively, would it help to use PowerShell instead of cmd.exe? This could be easily tested via hyperfines --shell option (if PowerShell also uses the syntax /c <cmd>).

I don't think PowerShell is a solution at this point. The initial design was not well thought out and it's also struggling with command line escaping and still evolving.

cmd.exe is, in comparison, a known, relatively stable, problem space.

@sharkdp
Copy link
Owner

sharkdp commented Jul 27, 2021

It looks like the resolution of rust-lang/rust#29494 (a new raw_arg method on Windows) could help us resolve this issue. It's currently only available on nightly though.

@rivy
Copy link
Author

rivy commented Jul 28, 2021

It looks like the resolution of rust-lang/rust#29494 (a new raw_arg method on Windows) could help us resolve this issue. It's currently only available on nightly though.

👍🏻
I've been keeping an eye on that issue as well.

@ofek
Copy link

ofek commented Nov 14, 2021

Any update on this?

@sharkdp
Copy link
Owner

sharkdp commented Mar 5, 2022

Looks like this might be stabilized in 1.61, so it will take a few more months (at least).

In the meantime, you might be able to use the new -N/--shell=none option of hyperfine to disable the shell completely.

@sharkdp
Copy link
Owner

sharkdp commented Apr 24, 2023

FYI @rivy

raw_arg has been stabilized for a while now: https://doc.rust-lang.org/std/os/windows/process/trait.CommandExt.html#tymethod.raw_arg

Someone with access to a Windows machine might be able to resolve this, ideally backed up by some new (regression) tests.

@PedroWitzel
Copy link
Contributor

Hello,
I can take a shot a this. I've being meaning to get more into OSS and rust, and I run mostly on windows, specially at work.

Pretty cool tool, btw. It's my go-to for performance tests 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants