Warn when fail to parse cmd line args. Close #1082

[bentayloruk]

Print a warning when fail to parse command line args with Argu. We still fall back to try and run with the pre-2.1.8 argument style.

Extracted exception formatting from the traceException function, so it could be used when tracing an exception as a warning.

This should help avoid confusion, as witnessed in #1072 and #594.

It looks like the image below. This was the result of ben.fsx target=Bob -st, which is a mix of the old and new. It is traced as a warning, but the text says Error parsing command line arguments..., so I'm not sure about the use of the word Error here. We show the error and then in green, explain we are trying to fallback. It would be good to get feedback on if this actually makes sense when you read it!

[forki]

oups I merged WIP. Should I revert?

[bentayloruk]

I think it should work OK, so maybe leave it in and see what happens if/when people notice it?

[forki]

[11:07:31][Step 2/2] Starting: D:\Work\ff5afcbda01118f4\packages\FAKE\tools\Fake.exe build.fsx Test CompileAlone BuildHelp DeployAddins VCS=UEN_0600 ServerMode=SQL
[11:07:31][Step 2/2] in directory: D:\Work\ff5afcbda01118f4
[11:07:32][Step 2/2] Error parsing command line arguments.  You have a mistake in your args, or are using the pre-2.1.8 argument style:
[11:07:32][Step 2/2] Exception Message:unrecognized argument: 'CompileAlone'.
[11:07:32][Step 2/2] 
[11:07:32][Step 2/2]    --envvar [-ev] <string> <string>: Set environment variable <name> <value>. Supports multiple.
[11:07:32][Step 2/2]    --envflag [-ef] <string>: Set environment variable flag <name> 'true'. Supports multiple.
[11:07:32][Step 2/2]    --logfile [-lf] <string>: Build output log file path.
[11:07:32][Step 2/2]    --printdetails [-pd]: Print details of FAKE's activity.
[11:07:32][Step 2/2]    --version [-v]: Print FAKE version information.
[11:07:32][Step 2/2]    --fsiargs [-fa] <string> ...: Pass args after this switch to FSI when running the build script.
[11:07:32][Step 2/2]    --boot [-b] <string> ...: Boostrapp your FAKE script.
[11:07:32][Step 2/2]    --break [-br]: Pauses FAKE with a Debugger.Break() near the start
[11:07:32][Step 2/2]    --single-target [-st]: Runs only the specified target and not the dependencies.
[11:07:32][Step 2/2]    --nocache [-nc]: Disables caching of compiled script
[11:07:32][Step 2/2]    --help [-h|/h|/help|/?]: display this list of options.
[11:07:32][Step 2/2] 
[11:07:32][Step 2/2] **************************************************
[11:07:32][Step 2/2] Type: System.ArgumentException
[11:07:32][Step 2/2] TargetSite: b Nessos-Argu-IExiter-Exit[b](System.String, Microsoft.FSharp.Core.FSharpOption`1[System.Int32])
[11:07:32][Step 2/2] Source: Argu
[11:07:32][Step 2/2] HResult: -2147024809
[11:07:32][Step 2/2] 
[11:07:32][Step 2/2] StackTrace
[11:07:32][Step 2/2] **************************************************

I think that message is incorrect

[bentayloruk]

This is doing what I expect, because the invocation Fake.exe build.fsx Test CompileAlone BuildHelp DeployAddins VCS=UEN_0600 ServerMode=SQL is the pre-2.1.8 style. However, as you don't expect this, it means we have a different understanding of the CLI and we probably need to get on the same page, so I can fix it.

Currently we try and parse with Argu. If that fails, we fall back to the pre-Argu CLI code. In the example here, Argu has failed and we show a warning, because you are not using the Argu based CLI. For example, with Argu, you'd use -ev VCS UEN_0600. However, we then fallback to the old CLI and your build should run OK, does it?. You can see what Argu expects.

I was aiming for this warning to point out that you're using the pre-Argu CLI, so you could change to the Argu style. Hence I would expect it to show here. However, to complicate this, the Argu configuration does not currently support multiple targets like you are issuing here. I could remedy this though.

Make sense? Happy to Skype/Hangout if not, would be good to get it sorted.

[forki]

CompileAlone is just a build script parameter.
We use it with hasBuildParam and getBuildParam

Looks like we introduced deeper issues

[bentayloruk]

Just to confirm though, your build should have still run fine. Did it? The only new thing here is a warning. If there are deeper issues, they were introduced when the Argu code was merged.

Try Fake.exe build.fsx Test -ef CompileAlone -ef BuildHelp -ef DeployAddins -ev VCS UEN_0600 -ev ServerMode SQL. I'm visiting a friend today, so can't confirm this is correct usage, but I think it is correct.

[bentayloruk]

On the positive side, I'm happy that the warning is shining a light on the confusion!

[forki]

Yes the build runs fine (for couple of years now)
On Jan 21, 2016 12:51 PM, "Ben Taylor" [email protected] wrote:

On the positive side, I'm happy that the warning is shining a light on the
confusion!

—
Reply to this email directly or view it on GitHub
#1090 (comment).

[bentayloruk]

Yes, so only the warning is new. Is it possible to try Fake.exe build.fsx Test -ef CompileAlone -ef BuildHelp -ef DeployAddins -ev VCS UEN_0600 -ev ServerMode SQL too, just so we can confirm support in the Argu style?

[forki]

The thing is: I will have to change 160 build configs ;-)

2016-01-21 12:56 GMT+01:00 Ben Taylor [email protected]:

Yes, so only the warning is new. Is it possible to try Fake.exe build.fsx
Test -ef CompileAlone -ef BuildHelp -ef DeployAddins -ev VCS UEN_0600 -ev
ServerMode SQL too, just so we can confirm support in the Argu style?

—
Reply to this email directly or view it on GitHub
#1090 (comment).

[bentayloruk]

Understood. I wasn't expecting you to do that, just wanted to know if it does actually work in your scenario. If there is any easy way to try one, let me know how it goes. I'll start thinking about what we can do to sort things out!

[forki]

yeah it actually works. but I wished we can keep that part of the syntax and just remove things that are really a conflict

[bentayloruk]

Makes sense. I could probably pre-parse to look for this part of the syntax and convert it to the Argu style. Could trace the conversion too, so people could see. Will have a look when back at the mill.

[vbfox]

I was pointed here by @forki after pointing this change on the chat, so here are my 2c.

Old style arguments are a lot more readable and when you use fake everyday in the command line they are really easy to type / use. I think that they should still work by default without warning or being deprecated.

That's easy to type & remember :

fake test WithCoverage only Database=foo

(In our scripts only mean don't run most dependent targets kinda like -st but with a few exceptions, and WithCoverage switches between Nunit and DotCoverNunit)

The new version is a lot less rememberable :

fake test -ef WithCoverage -ef only -ev Database foo

In our specific case we always wrap fake.exe invocation in a cmd script so having something like -- followed by old style arguments would be enough but I suspect that most people would prefer old and new style arguments to blend together nicely.

So the conversion, allowing a mix of both worlds would be a perfect solution 👍

fake Build -st database=foo

[bentayloruk]

Thanks for writing up your 2c. It's my fault, because I should have included the warning in the original 2014 PR, or made it more clear that backwards compat was only about not breaking existing scripts. It did not include support for mixing the two, because the original PR was done to support the --fsiargs switch, which was difficult/impossible to do safely in the original format, because it supports parsing arbitrary arguments to the build script file. In addition, I thought trying to do it, would probably be bug prone in the long run, when new options are added.

How about this. A PR that:

• Checks args for any of the Argu switches.
• If Argu switches detected, require Argu parsing.
• If Argu parsing fails, error the build.
• If no Argu switches detected, run old style.

This PR would eliminate the warning you see AND kill off any problems with people mixing, which is not currently supported. It should also be fairly simple, assuming the switches are available from the Argu API.

We could then take time and look if a blend might work. Perhaps we could take the original style and convert them into Argu style, so we don't have two CLI parsers? I'm not sure how easy this would be, but the first PR would give us time and keep things correct for those people that are mixing and getting confused.

[vbfox]

The proposed PR seem good as a first step to allow old style argument when only them are provided and make clear that for now mixing isn't supported.