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

Better Old-Style Arg Parsing #1301

Merged
merged 1 commit into from Jul 14, 2016

Conversation

Projects
None yet
3 participants
@baronfel
Copy link
Contributor

commented Jul 13, 2016

Imagine a command line Fake invocation like so:
fake.exe ./build.fsx package --single-target variable=foo

In this case, Fake parses the single-target flag incorrectly and so the package target is run along all dependencies.

I took a look and this is due to the way we process old vs new-style arguments. Specifically, as soon as there is an old-style argument in the args the parser breaks and the special handling of --single-target, --print-details and other internal arguments that is done by the Choice1Of2 branch in Program.fs is skipped over by the old-style argument parsing.

So to correct this, I've added a bit of logic to the Cli.parseArgs function that normalizes various forms of the flags that are valid for new-style args into the versions of those flags that are expected later in the pipeline.

In the command line above, Fake results in the following list of arguments: [("target", "package"); ("--single-target", "true"); ("variable", "foo")]
Inside the codebase, Fake is using hasBuildParam to look for "single-target", which is not found in the list above.
With my changes, Fake results in the following list of arguments: [("target", "package"); ("single-target", "true"); ("variable", "foo")], which contains the key looked for.

Chet Husk
make the parsing of FAKE commands that are used internal to target pr…
…ocessing more resilient when we're parsing old-style command line args
@forki

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

This is very dangerous territory. We need to make sure that we don't break it again ;-)

/cc @matthid

@baronfel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2016

Oh I agree. I think this change is pretty limited and simple, though, just creating maps for new-style commands that need to be transformed into something that the old-style parser can understand.

Sort of related: Argu3 with the 'unparsed members' support may allow for this 'old-style'/'new-style' split to go away. you can parse the new-style members in the first pass, then parse any unparsed members in the old style, generating a single, unified list of fsi args and build args to be passed into the runner.

let split (arg:string) =
let pos = arg.IndexOfAny splitter
[| arg.Substring(0, pos); arg.Substring(pos + 1, arg.Length - pos - 1) |]
let (|KeyValue|Flag|TargetName|) ((i,arg) : int * string) =

This comment has been minimized.

Copy link
@baronfel

baronfel Jul 14, 2016

Author Contributor

This is the same logic from the previous version, just captured into an AP to make it very clear what case is what.

else arg, "true")
|> Seq.mapi (fun i a -> match (i, a) with
| TargetName t -> "target", t
| Flag f when Map.containsKey f specialFlags -> Map.find f specialFlags, "true"

This comment has been minimized.

Copy link
@baronfel

baronfel Jul 14, 2016

Author Contributor

This is the only new case, really, where we expand out new-style flags that had special handling in the code (See Program.fs where we yield ('single-target', true) for the new-style args as an example).

let fakeArgs = cmdArgs |> Array.filter (fun x -> x.StartsWith "-d:" = false)
let fsiArgs = cmdArgs |> Array.filter (fun x -> x.StartsWith "-d:") |> Array.toList
let args = CommandlineParams.parseArgs (fakeArgs |> Seq.filter ((<>) buildScriptArg) |> Seq.filter ((<>) "details"))
let fsiArgs, fakeArgs = cmdArgs |> Array.partition (fun x -> x.StartsWith "-d:")

This comment has been minimized.

Copy link
@baronfel

baronfel Jul 14, 2016

Author Contributor

Now that '-pd' and '--print-details' are being expanded, we can't go check back in cmdArgs for 'details', we have to check the inflated list of arguments. That's why I removed the details filter here. Then the lines below do the correct 'details' handling.

@forki

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

ok let's see what happens. ;-)

Thanks for caring.

@forki forki merged commit 5edc4a9 into fsharp:master Jul 14, 2016

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

unfortunately it broke our TC builds. I had to revert it.

[17:38:15][Step 2/2] Starting: D:\Work\284e8225f0c0a25f\packages\FAKE\tools\Fake.exe build.fsx VCS=UEN_060015 CompileAlone ServerMode=SQL IncludeTags=INITCU
[17:38:15][Step 2/2] in directory: D:\Work\284e8225f0c0a25f
[17:38:23][Step 2/2] FsiEvaluationException:
[17:38:23][Step 2/2] 
[17:38:23][Step 2/2] Error: System.TypeInitializationException: Der Typeninitialisierer f�r "<StartupCode$FSI_0005>.$FSI_0005_Settings-teamcity$fsx" hat eine Ausnahme verursacht. ---> System.Exception: Der Kommandozeilenparameter VCS mit der VCS-ID muss zur Ermittlung des Hashwertes angegeben werden!
[17:38:23][Step 2/2]       bei <StartupCode$FSI_0005>.$FSI_0005_Settings-teamcity$fsx..cctor() in D:\Work\284e8225f0c0a25f\settings-teamcity.fsx:Zeile 8.
[17:38:23][Step 2/2]       --- Ende der internen Ausnahmestapel�berwachung ---
[17:38:23][Step 2/2]       bei <StartupCode$FSI_0005>.$FSI_0005_Build$fsx.main@() in D:\Work\284e8225f0c0a25f\build.fsx:Zeile 38.
[17:38:23][Step 2/2]    Stopped due to error
[17:38:23][Step 2/2]    
[17:38:23][Step 2/2] 
[17:38:23][Step 2/2] Output: [Loading D:\Work\284e8225f0c0a25f\settings-default.fsx
[17:38:23][Step 2/2]     Loading D:\Work\284e8225f0c0a25f\settings-gitlab.fsx
[17:38:23][Step 2/2]     Loading D:\Work\284e8225f0c0a25f\settings-teamcity.fsx
[17:38:23][Step 2/2]     Loading D:\Work\284e8225f0c0a25f\build.fsx]
[17:38:23][Step 2/2]    
[17:38:23][Step 2/2] 
[17:38:23][Step 2/2] Input: D:\Work\284e8225f0c0a25f\build.fsx
[17:38:23][Step 2/2] 
[17:38:23][Step 2/2] Exception: Yaaf.FSharp.Scripting.FsiEvaluationException: Error while compiling or executing fsharp snippet. ---> System.Exception: Operation failed. The error text has been print the error stream. To return the corresponding FSharpErrorInfo use the EvalInteractionNonThrowing, EvalScriptNonThrowing or EvalExpressionNonThrowing
[17:38:23][Step 2/2]    bei Microsoft.FSharp.Compiler.Interactive.Shell.FsiEvaluationSession.commitResult[a,b](FSharpChoice`2 res)
[17:38:23][Step 2/2]    bei Microsoft.FSharp.Compiler.Interactive.Shell.FsiEvaluationSession.EvalScript(String filePath)
[17:38:23][Step 2/2]    bei Yaaf.FSharp.Scripting.Helper.evalScript@1097.Invoke(String arg00) in C:\code\fake\paket-files\matthid\Yaaf.FSharp.Scripting\src\source\Yaaf.FSharp.Scripting\YaafFSharpScripting.fs:Zeile 1097.
[17:38:23][Step 2/2]    bei Yaaf.FSharp.Scripting.Helper.save_@1072-2.Invoke(Unit unitVar0) in C:\code\fake\paket-files\matthid\Yaaf.FSharp.Scripting\src\source\Yaaf.FSharp.Scripting\YaafFSharpScripting.fs:Zeile 1073.
[17:38:23][Step 2/2]    bei Yaaf.FSharp.Scripting.Helper.consoleCapture[a](TextWriter out, TextWriter err, FSharpFunc`2 f) in C:\code\fake\paket-files\matthid\Yaaf.FSharp.Scripting\src\source\Yaaf.FSharp.Scripting\YaafFSharpScripting.fs:Zeile 1018.
[17:38:23][Step 2/2]    bei Yaaf.FSharp.Scripting.Helper.redirectOut@1044[a](Boolean preventStdOut, OutStreamHelper out, OutStreamHelper err, FSharpFunc`2 f) in C:\code\fake\paket-files\matthid\Yaaf.FSharp.Scripting\src\source\Yaaf.FSharp.Scripting\YaafFSharpScripting.fs:Zeile 1051.
[17:38:23][Step 2/2]    bei Yaaf.FSharp.Scripting.Helper.save_@1071-1.Invoke(String text) in C:\code\fake\paket-files\matthid\Yaaf.FSharp.Scripting\src\source\Yaaf.FSharp.Scripting\YaafFSharpScripting.fs:Zeile 1072.
[17:38:23][Step 2/2]    --- Ende der internen Ausnahmestapel�berwachung ---
[17:38:23][Step 2/2]    bei Yaaf.FSharp.Scripting.Helper.save_@1071-1.Invoke(String text) in C:\code\fake\paket-files\matthid\Yaaf.FSharp.Scripting\src\source\Yaaf.FSharp.Scripting\YaafFSharpScripting.fs:Zeile 1080.
[17:38:23][Step 2/2]    bei Yaaf.FSharp.Scripting.Helper.session@1100.Yaaf-FSharp-Scripting-IFsiSession-EvalScriptWithOutput(String ) in C:\code\fake\paket-files\matthid\Yaaf.FSharp.Scripting\src\source\Yaaf.FSharp.Scripting\YaafFSharpScripting.fs:Zeile 1102.
[17:38:23][Step 2/2]    bei Fake.FSIHelper.runScriptUncached(Boolean useCache, String scriptPath, IEnumerable`1 fsiOptions, Boolean printDetails, CacheInfo cacheInfo, TextWriter out, TextWriter err) in C:\code\fake\src\app\FakeLib\FSIHelper.fs:Zeile 471.
[17:38:23][Step 2/2] System.TypeInitializationException: Der Typeninitialisierer f�r "<StartupCode$FSI_0005>.$FSI_0005_Settings-teamcity$fsx" hat eine Ausnahme verursacht. ---> System.Exception: Der Kommandozeilenparameter VCS mit der VCS-ID muss zur Ermittlung des Hashwertes angegeben werden!
[17:38:23][Step 2/2]    bei <StartupCode$FSI_0005>.$FSI_0005_Settings-teamcity$fsx..cctor() in D:\Work\284e8225f0c0a25f\settings-teamcity.fsx:Zeile 8.
[17:38:23][Step 2/2]    --- Ende der internen Ausnahmestapel�berwachung ---
[17:38:23][Step 2/2]    bei <StartupCode$FSI_0005>.$FSI_0005_Build$fsx.main@() in D:\Work\284e8225f0c0a25f\build.fsx:Zeile 38.
[17:38:23][Step 2/2] Stopped due to error
@baronfel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2016

Interesting. I'll take another look and rerun these tests locally. They worked earlier though? Anyway, I'll look some more.

@baronfel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2016

Oh, that looks like an ordering issue there. The VCS parameter came before the target, and I think some code is expecting the target to be first. let me see if I can narrow that down.

@baronfel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2016

Ok, i see what the problem is. In the Active Pattern I changed the order of the parsing of scripts. In the original version this we:

  1. parse out key-value pairs
  2. else if the first argument then must be the target
  3. else then the argument is a flag

Given this logic, your command that failed above, [|"Fake.exe"; "build.fsx"; "VCS=UEN_060015"; "CompileAlone"; "ServerMode=SQL"; "IncludeTags=INITCU"|], gets parsed into

parsePositionalArgs [|"Fake.exe"; "build.fsx"; "VCS=UEN_060015"; "CompileAlone"; "ServerMode=SQL"; "IncludeTags=INITCU"|];;
val it : Args =
{Script = Some "build.fsx";
Target = null;
Rest =
[|"Fake.exe"; "VCS=UEN_060015"; "CompileAlone"; "ServerMode=SQL";
"IncludeTags=INITCU"|];}
`

and the Rest then get parsed into

[("VCS", "UEN_060015"); ("CompileAlone", "true"); ("ServerMode", "SQL"); ("IncludeTags", "INITCU")]

So all we need to do is change the order of the AP processing in my commit and the behavior will be the same. Example below after I change this to match:

[("VCS", "UEN_060015"); ("CompileAlone", "true"); ("ServerMode", "SQL"); ("IncludeTags", "INITCU")]

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2016

This is indeed dangerous territory... As this PR tries to simplify the logic it is probably a good idea. But we should really have a lot of unit tests before changing anything.
(The current failure might be a good test case)

This is one of the things I will clean up with the dotnetcore version as well: Long term there will be a simplified argu-based command line interface.

I have no idea if we keep that level of compatibility or just stop updating the old version (and add a warning). It's too early to tell, but just keep that in mind when working on this.

@baronfel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2016

I could work on pulling out the parsing of args int a function string [] -> some_args_type and then make fscheck tests that generate random args and verify that the old logic and the new logic parse the same output set of env and fsi args?

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2016

@baronfel That would in fact be awesome. If we have this I would definitely be more willing to write a compat layer later for the "new" version :)

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.