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

fix #9842 #9951 #8553: `nim -r` and parseopt.cmdLineRest are now correct #10291

Merged
merged 3 commits into from Jan 15, 2019

Conversation

Projects
None yet
3 participants
@timotheecour
Copy link
Contributor

commented Jan 13, 2019

  • fix #9951: parseopt.cmdLineRest is now correct (previously didn't handles spaces, quotes, single quotes, empty arguments)
    • this cleans up implementation, see lib/pure/parseopt.nim
  • fix #9842: nim -r is now correct, as a result (ditto, previously didn't handles spaces, quotes, single quotes, empty arguments)
  • fix #8553
  • unrelated improvement in compiler/unittest_light.nim which I could put in separate PR if really needed
  • verbosity:0 now silences hintCC

note

I had to add joinable:false to avoid test failure, but #10293 will allow to remove joinable:false

@timotheecour timotheecour force-pushed the timotheecour:pr_fix_cmdLineRest branch 2 times, most recently from 39d6f84 to bbb0a69 Jan 13, 2019

@timotheecour timotheecour changed the title fix #9842 #9951: `nim -r` and parseopt.cmdLineRest are now correct fix #9842 #9951: `nim -r` and parseopt.cmdLineRest are now correct; refs nim-lang/RFCs#119 Jan 14, 2019

@timotheecour timotheecour force-pushed the timotheecour:pr_fix_cmdLineRest branch from bbb0a69 to 023b416 Jan 14, 2019

@timotheecour timotheecour changed the title fix #9842 #9951: `nim -r` and parseopt.cmdLineRest are now correct; refs nim-lang/RFCs#119 fix #9842 #9951: `nim -r` and parseopt.cmdLineRest are now correct Jan 14, 2019

@krux02

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

This really seems to clean up the implementation. I do like that. But at the moment, I can't verify if these changes are correct, because I don't know what you tried to do. How was the behavior before? What was wrong with it? How does/should it behave after this patch? How did you fix it?

At the moment you just claim that you fixed issues, but to me it is not clear how these issues should be fixed.

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

I don't know what you tried to do

first I tried to fix using system.addQuoted ; that fixed quoting issues for some cases (empty arg, arg containing double quote) but failed for cases involving single quote; using quoteShellCommand worked in all the cases, and made nim r work as a byproduct since it relies on cmdLineRest

How was the behavior before?

see failing tests as reported in #9951 and #9842 ; the tests I added should cover everything reported there, see tests/misc/tparseopt.nim + other cases:

block: # fix #9951
    var p = parseopt.initOptParser(@["echo \"quoted\""])
    assertEquals parseopt.cmdLineRest(p), """'echo "quoted"'"""
    let args = @["a1b", "a2 b", "", "a4\"b", "a5'b", r"a6\b", "a7\'b"]
    var p2 = parseopt.initOptParser(args)
    assertEquals parseopt.cmdLineRest(p2),
      """a1b 'a2 b' '' 'a4"b' 'a5'"'"'b' 'a6\b' 'a7'"'"'b'"""
    doAssert "a5'b" == "a5\'b"

   block: # fix #9842
    let exe = buildDir / "D20190112T145450".addFileExt(ExeExt)
    defer: removeFile exe
    let args = @["a1b", "a2 b", "", "a4\"b", "a5'b", r"a6\b", "a7\'b"]
    let cmd = "$# c -r --verbosity:0 -o:$# -d:testament_tparseopt $# $#" %
      [getCurrentCompilerExe(), exe, currentSourcePath(),
          args.quoteShellCommand]
    var ret = execCmdEx(cmd, options = {})
    if ret.exitCode != 0:
      # before bug fix, running cmd would show:
      # sh: -c: line 0: unexpected EOF while looking for matching `"'\n
      echo "exitCode: ", ret.exitCode, " cmd:", cmd
      doAssert false
    stripLineEnd(ret.output)
    assertEquals ret.output,
      """
@["a1b", "a2 b", "", "a4\"b", "a5\'b", "a6\\b", "a7\'b"]
arg 0 ai.len:3 :{a1b}
arg 1 ai.len:4 :{a2 b}
arg 2 ai.len:0 :{}
arg 3 ai.len:4 :{a4"b}
arg 4 ai.len:4 :{a5'b}
arg 5 ai.len:4 :{a6\b}
arg 6 ai.len:4 :{a7'b}"""

you can see old behavior by running nim c -r tests/misc/tparseopt.nim with nim compiled before vs after this PR;
(before PR, cmdLineRest, nim -r, and other tools depending on cmdLineRest didn't work if some arg were empty, or contained a single or double quote)

@timotheecour timotheecour requested a review from krux02 Jan 14, 2019

@timotheecour timotheecour force-pushed the timotheecour:pr_fix_cmdLineRest branch from bfad42e to 09e007e Jan 14, 2019

@krux02

krux02 approved these changes Jan 14, 2019

Copy link
Contributor

left a comment

Thanks for the explanation.

@krux02

krux02 approved these changes Jan 14, 2019

Copy link
Contributor

left a comment

good work

@timotheecour timotheecour force-pushed the timotheecour:pr_fix_cmdLineRest branch from 09e007e to c56ee9f Jan 14, 2019

@timotheecour timotheecour merged commit 9e68b2c into nim-lang:devel Jan 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@timotheecour timotheecour deleted the timotheecour:pr_fix_cmdLineRest branch Jan 15, 2019

@timotheecour timotheecour changed the title fix #9842 #9951: `nim -r` and parseopt.cmdLineRest are now correct fix #9842 #9951 #8553: `nim -r` and parseopt.cmdLineRest are now correct Jan 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.