Better Old-Style Arg Parsing #1301
Conversation
…ocessing more resilient when we're parsing old-style command line args
|
This is very dangerous territory. We need to make sure that we don't break it again ;-) /cc @matthid |
|
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) = |
There was a problem hiding this comment.
This is the same logic from the previous version, just captured into an AP to make it very clear what case is what.
|
ok let's see what happens. ;-) Thanks for caring. |
|
unfortunately it broke our TC builds. I had to revert it. |
|
Interesting. I'll take another look and rerun these tests locally. They worked earlier though? Anyway, I'll look some more. |
|
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. |
|
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:
Given this logic, your command that failed above,
and the Rest then get parsed into
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:
|
|
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. 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. |
|
I could work on pulling out the parsing of args int a function |
|
@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 :) |
Imagine a command line Fake invocation like so:
fake.exe ./build.fsx package --single-target variable=fooIn this case, Fake parses the single-target flag incorrectly and so the
packagetarget 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-detailsand 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.