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

Adds additional handling around ScriptArgs to be more flexible #12

Closed
wants to merge 2 commits into from
Closed

Adds additional handling around ScriptArgs to be more flexible #12

wants to merge 2 commits into from

Conversation

phil-scott-78
Copy link

This is more of a request for comments, but I think it is a step in the right direction. Currently passing in parameters to a cake script from the powershell bootstrapper isn't consistent, plus kind of weird especially with handling filenames.

Let's take this "build" script


#addin "Cake.FileHelpers"

var target = Argument("target", "Default");
var file = Argument<string>("file");
var someBool = Argument<bool>("someBool", true);
var someString =Argument<string>("someString", "hello!");

Task("Do-the-do")
    .Does(() =>
{
    Information("Bool value? {0}", someBool.ToString());
    Information("someString? {0}", someString);
    var exists = FileExists(file).ToString();
    Information("File {0} exists? {1}", file, exists);    
});

Task("Default")
    .IsDependentOn("Do-the-do");

RunTarget(target);

With the current implementation to run this script with diagnostic verbosity you must run to get this to work (note no equal after verbosity but required for the others)

.\build.ps1 -verbosity diagnostic -file="""file.txt""" -someBool=true

Additionally, if you make any mistake it'll blindly pass those args in cake giving some less than helpful errors because cake expects things differently than the bootstrapper. Some of these error messages actually point you in the wrong direction (see the verbosity help that is outputted)

 .\build.ps1 -verbosity=diagnostic -file="""file.txt"""

you get this output

Preparing to run build script...
Running build script...
Multiple arguments with the same name (verbosity).
Module directory does not exist.

Usage: Cake.exe [script] [--verbosity=value]
            [--showdescription] [--dryrun] [..]

Example: Cake.exe
Example: Cake.exe build.cake --verbosity=quiet

Or if you don't properly quote the file name

.\build.ps1 -verbosity diagnostic -file=file.txt

you get this output

Preparing to run build script...
Running build script...
More than one build script specified.
Module directory does not exist.

This change allows you to run

.\build.ps1 -verbosity diagnostic -file file.txt -someBool

Looks more like powershell and still sends things into cake.exe as expected. File.txt will be properly quoted and someBool will be sent as someBool=true. Not sure if I've caught all the edge cases here especially around what values need to be quoted, but I wanted to throw this out and see what you all think. I'll be away on holiday until the 1st but hopefully I'll be able to check in and answer any questions or get tweaks implemented if this is something valuable.

The current implementation just blindly passes the extra parameters into
cake. This leads to some inconsistency in how parameters are passed into
build.ps1. For example, to pass in a file name with verbosity set to
diagnostic you need to run

    ./buid.ps1 -verbosity diagnostic -file="""file.txt"""

This change allows you to run

    ./build.ps1 -vebosity diagnostic -file=file.txt
Copy link

@shaneray shaneray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks pretty straight forward to me and it makes calling the powershell script much nicer.

@patriksvensson
Copy link
Member

Perhaps an ever easier solution would be to remove all the PowerShell arguments to the bootstrapper so there's a 1:1 mapping with Cake.

build.ps1 --verbosity=diagnostic --foo=bar

Not saying that we should do that (need some discussion) but our goal is to make the bootstrappers simpler and this would be a step in that direction.

@cake-build/team Any thoughts about this?

@phil-scott-78
Copy link
Author

@patriksvensson - sorry for the delay getting back. Internet connection was worse than expected while traveling. So I tossed the idea around but honestly I couldn't wrap my head around a way to handle it without breaking backwards compatibility with anyone that might be using the current powershell like syntax. All my attempts ended with a huge if statement mess with a nasty combination of string parsing and examining $MyInvocation

The big hurdle I encountered even when trying to rip out the powershell arguments is PS's insistence that it doesn't want to tell you about the quotes that might have been passed in on the command line. So if, for example, you passed in -file="hello.text" then $args is -file=hello.txt. Without the quotes the cake.exe command line parser chokes on the . $MyInvocation.Line does have the quotes, but as I said then you get into the weird world of parsing that out. Much easier if you break backwards compatibility with the current script's parameter binding, but huge headache otherwise.

@pascalberger
Copy link
Member

We did some maintenance work on this repo which included removing of the develop branch which closed this PR.

@phil-scott-78 In the meantime Cake .NET Tool has been made the recommended approach for running Cake scripts. If you feel this is still an issue please start a discussion.

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.

7 participants