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

New (backwards compat) CLI for FAKE that includes FSI cmd args passing #455

Merged
merged 7 commits into from Jun 9, 2014

Conversation

bentayloruk
Copy link
Contributor

While working on #438, I realised that the feature would make the existing FAKE command line brittle. The existing code looks for argument strings regardless of their position or relation to switches. Adding support for passing arbitrary string args to FSI, could easily break this. Therefore, I have added the support I needed for #438 and created a new CLI for FAKE that I think will be much easier to understand and extend in the future. I hope you agree. If not, I would be happy to discuss and fix an concerns.

This PR makes FAKE.exe dependant on the UnionArgParser library. If figured this would be ok as FAKE includes other assemblies and this one is fairly small and focused. Using it to specify the FAKE CLI args is really clean.

type FakeArg = 
    | [<AltCommandLine("-ev")>] EnvVar of string * string
    | [<AltCommandLine("-ef")>] EnvFlag of string
    | [<AltCommandLine("-lf")>] LogFile of string
    | [<AltCommandLine("-pd")>] PrintDetails
    | [<AltCommandLine("-v")>] Version
    | [<Rest>] FsiArgs of string
    | [<Rest>] Boot of string
    interface IArgParserTemplate with
        member x.Usage = 
            match x with
            | EnvVar _ -> "Set environment variable <name> <value>. Supports multiple."
            | EnvFlag _ -> "Set environment variable flag <name> 'true'. Supports multiple."
            | LogFile _ -> "Build output log file path."
            | PrintDetails _ -> "Print details of FAKE's activity."
            | FsiArgs _ -> "Pass args after this switch to FSI when running the build script."
            | Version _ -> "Print FAKE version information."
            | Boot _ -> "TBC"

An example usage now looks like this:

fake.exe build.fsx clean --envvar key1 val1 --envar key2 val2

We also support the short switches:

fake.exe build.fsx -ev key1 val1 -ev key2 val2 -ef blah

fake --help now looks like this:
cli

I don't think too much is outstanding, although I appreciate it is important that this is tested well, do we don't break old scripts. If the new CLI args don't parse, we fall back to exactly the same code as before the new CLI, so I hope this provides the safest experience.

TODO:

  • Improve the switch documentation strings (I don't know much about Boot for example).
  • There is some FSI arg crossover with Vasily's work on Pass optional script arguments into deployment script #453. I have merged this, but I think it could be cleaned up/generalised a little more.
  • The --help arg defaults to a UnionArgParser parse error and we fall through to the old command line passing. This works for now.
  • Done. I appreciate that previously the build script arg was first and did not require a switch. I could add support for this, but thought I would find out how important this is first.

Look forward to feedback!

* New CLI for FAKE that uses UnionArgParser.
* If new CLI cannot parse, use old CLI for backwards compat.
* Strongly typed and easy to read and modify.
* Less brittle in face of changes needed for fsprojects#438 etc.
* Generates documentation.
* Supports combination with app.config settings.
* Support long and short switches (e.g. --version or -v).
* Support multi-values (e.g. --envVar key value --envVar key value).
* Make env flag switches explicit (i.e. --envFlag nameWhoseValWillBeTrue).
* Print old and new help.
Conflicts:
	src/app/FakeLib/FSIHelper.fs
@forki
Copy link
Member

forki commented Jun 4, 2014

This looks pretty cool.
Making it backwards compatibile is very important. Thanks for taking care of this.

The build script arg in first position is actually used very frequently. Especially from a cmd prompt. Think of things like "fake clean" or "fake test".

@bentayloruk
Copy link
Contributor Author

Thanks for the feedback. I'll update the PR with support for the positional, non-switch arguments (and fix up the target usage).

* Update usage documentation strings and usage print function.
* Add basic tests for CLI positional argument parsing.
@bentayloruk
Copy link
Contributor Author

@forki I've added back support for the positional arguments. You now have:

fake [<script>] [<target>] [switches]

I've updated the PR description with a new image and current switches. I've also added a few simple tests. I didn't create a new test project, but added them (and ref to fake.exe) in the FakeCore test lib. Hope this is OK.

It appears the build is failing, so I'll have a look at that.

@bentayloruk
Copy link
Contributor Author

The build does not like the DU field names I've used on FsiArgs. Guessing this is some Mono F# build version/support issue...

@forki
Copy link
Member

forki commented Jun 6, 2014

It's probably not supported in the F# version which is used by mono. I don't know if we can change that version.

@bentayloruk
Copy link
Contributor Author

I'll remove the usage for now then.

@bentayloruk
Copy link
Contributor Author

Building. I'm not planning on doing anything else on this for now. Think it is good to go, so will await the feedback!

forki added a commit that referenced this pull request Jun 9, 2014
New (backwards compat) CLI for FAKE that includes FSI cmd args passing
@forki forki merged commit dd42061 into fsprojects:master Jun 9, 2014
@forki
Copy link
Member

forki commented Jun 9, 2014

I'll release a prerelease version and check our build infrastructure against this.
Good work.

@bentayloruk
Copy link
Contributor Author

Great, thanks! Glad to finally start putting something back into FAKE after all the use I have had from it.

@forki
Copy link
Member

forki commented Jun 10, 2014

There is a bug. Please try "build.cmd clean" from the command line. I get:

image

@bentayloruk
Copy link
Contributor Author

I see the problem. I handle fake.exe build.fsx clean or fake.exe clean or fake.exe build.fsx. I did not think about the usage fake.exe build.fsx target=clean as it is not explicit in the code and I assumed the positional usage rather than the kvp usage. Will fix now.

@forki
Copy link
Member

forki commented Jun 11, 2014

thanks for fixing.

next cool step would be to improve https://github.com/fsharp/FAKE/blob/master/help/commandline.md - are you interested?

@bentayloruk
Copy link
Contributor Author

Yes, I will do this. OSS without good docs is not good OSS!

@bentayloruk
Copy link
Contributor Author

Wonder what the world would be like if PRs got rejected without good docs...

@bentayloruk
Copy link
Contributor Author

BTW - did you see this nice Markdown extension for VS? http://visualstudiogallery.msdn.microsoft.com/0855e23e-4c4c-4c82-8b39-24ab5c5a7f79

@forki
Copy link
Member

forki commented Jun 11, 2014

Wonder what the world would be like if PRs got rejected without good docs...

Then we would probably have no OSS at all.

@bentayloruk
Copy link
Contributor Author

Maybe, maybe :)

@bentayloruk
Copy link
Contributor Author

Do you think --EnvVar and --EnvFlag should be --EnvironmentVariable and --EnvironmentFlag? We have the short versions -ev and -ef, so maybe the long ones, should be long!

@forki
Copy link
Member

forki commented Jun 12, 2014

sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants