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

Allow users to use any of the Cake runner arguments as build script arguments #3279

Open
augustoproiete opened this issue Mar 14, 2021 · 17 comments · May be fixed by #4142
Open

Allow users to use any of the Cake runner arguments as build script arguments #3279

augustoproiete opened this issue Mar 14, 2021 · 17 comments · May be fixed by #4142

Comments

@augustoproiete
Copy link
Member

Background

The Cake runner supports a number of arguments, such --version to display the version of the runner, or --debug to launch the script in debug mode.

However, this make it impossible for these names to be used as arguments in Cake build scripts. For example, if one wanted to use the argument version as an input to the build script:

var version = Argument<string>("version");

And then run Cake:

dotnet cake --version=1.2.3

They would receive an error: Flags cannot be assigned a value

Workaround

The current workaround is to use the "remaining arguments" feature when using one of these "reserved" argument names:

dotnet cake -- --version=1.2.3

Proposal

This issue intends to make the necessary updates to allow existing "flag" arguments to be used in build scripts, so that if the user runs cake with --version=1.2.3 do not assume the flag --version and instead execute the Cake script with the argument ["version"] = "1.2.3".

Command-line Expected result
dotnet cake --version Display Cake version (as it is today)
dotnet cake --version=1.2.3 Run build script and set the version argument to 1.2.3

This should work for all Cake runner arguments, except for Verbosity which is not a "flag" argument.

image


Related:

@alexandru-valeanu
Copy link

Hi @augustoproiete, as this seems still open, I'd like to give it a go.

@augustoproiete
Copy link
Member Author

@alexandru-valeanu Sure.

Heads-up that we use Spectre.Console for parsing arguments, so there's a little bit of investigation needed to see if the fix can (and should) be done directly in Cake, of if it should be fixed upstream. If you discover that the fix doesn't belong in Cake, you'd need to send a PR to Spectre.Console and the fix in Cake would be just upgrading to a newer version of Spectre.Console.

@alexandru-valeanu
Copy link

Thanks for the heads-up.

@tapika
Copy link

tapika commented Dec 6, 2021

I'm not sure if this is specified anywhere - but writing constructor like this:

    [TaskName("Default")]
    public sealed class MyTestTask : FrostingTask<BuildContext>
    {
        public MyTestTask(Spectre.Console.Cli.IRemainingArguments args)
        {

Allows to pick up any additional arguments inside Frosting application.

Wondering if this is documented in Cake or known to users of Cake.

@tapika
Copy link

tapika commented Dec 9, 2021

https://github.com/tapika/swupd/blob/develop/cakebuild/CommandLineArgs.cs

I've initially sketched command line support for your own cake frosten build application.

There are several hacks I have applied - hopefully they could be sorted out in both - in Cake and in Spectre.Console.Cli.

Overall problem list:

  1. Cake: src\Cake.Frosting\Internal\Commands\DefaultCommandSettings.cs
    Protected, cannot be used by external application. Maybe should be public instead ?

(P.S. '--version' does not work ?)

  1. Spectre.Console.Cli / Cake: There is a clear need to route some of arguments back to Cake - and need to have a list of cake compatible argument list. I have implemented via reflection (see GetCakeArgs) - but maybe there could be simpler solution ?

  2. Spectre.Console.Cli: class ParseCommandLineArgs : Command
    Just parsing command line arguments seems to be bit complex to do from user code. Could this be simplified somehow ?

  3. Spectre.Console.Cli: See in CommandLineArgumentParser.Parse
    Spectre.Console.Cli resides in it's own assembly, Cake.exe in it's own - both don't know anything about user defined cake builder assembly.

And if you want to register your own custom type - that is well protected. I've extracted like this,
but maybe these methods should be public instead ?

   FieldInfo fi = config.GetType().GetField("_registrar", BindingFlags.NonPublic | BindingFlags.Instance);
   var reg = (ITypeRegistrar)fi.GetValue(config);

   reg.RegisterLazy(typeof(CommandLineArgs), () => { return newArgs; });
  1. Spectre.Console.Cli:
    var helpOpt = new CommandOptionAttribute("-h|--help");
    var isMatch = helpOpt.GetType().GetMethod("IsMatch", BindingFlags.NonPublic | BindingFlags.Instance);

IsMatch is not public.

   MethodInfo mi = typeof(CommandApp).GetMethod("GetRenderableErrorMessage", BindingFlags.NonPublic | BindingFlags.Static);

Also non-public.

  1. Cake / Spectre.Console.Cli::

CommandLineArgs args = CommandLineArgumentParser.Parse(_args);

maybe should be provided by Cake as it's own API? But this also maps to what can be done between assemblies.

@augustoproiete
Copy link
Member Author

Hey @tapika are you still interested in taking this one?

@FrankRay78
Copy link
Contributor

Leaving a comment as required @augustoproiete - I'll now wait to see if @tapika responds.

@tapika
Copy link

tapika commented Sep 13, 2022

Can / could do it, but as this is kinda hobby, cannot use 24/7 time.

I guess need to integrate changes directly into cake ? Should we aim to improve / patch Spectre.Console.Cli ?

@tapika
Copy link

tapika commented Sep 15, 2022

5th item - raised as it's own ticket to Spectre.Console:

spectreconsole/spectre.console#967

@FrankRay78
Copy link
Contributor

FrankRay78 commented Oct 25, 2022

I have opened a PR on spectreconsole which allows command line options to be specified ahead of arguments without incurring the Flags cannot be assigned a value. error message (see spectreconsole/spectre.console#1029)

This PR, if accepted and merged, would make the following command legal syntax: dotnet cake --tree script.cake The Flags cannot be assigned a value. would still be seen for the following command, however dotnet cake script.cake --version=1.2.3

A consequence of the PR being accepted and merged is that fully implementing this cake issue can then be easily done by a further, small code change to spectreconsole. This would introduce a setting (named something like AddFlagsToRemainingArgumentsWhenCannotBeAssigned) to catch Flags cannot be assigned a value. errors that would have been thrown for any of the in-built cake options, instead adding the flag and value to the remaining arguments (and avoiding the explicit need for the -- separator).

We'd set AddFlagsToRemainingArgumentsWhenCannotBeAssigned to true and then the following command would become legal syntax: dotnet cake script.cake --version=1.2.3 (nb. the following would still run dotnet cake script.cake --version, falling back to existing behaviour and showing the cake version)

I'm going to now prepare this follow-on change to spectreconsole.

@tapika
Copy link

tapika commented Oct 25, 2022

Good progress. I do not have so much time to focus on this - working primarily on my own choco fork. Command line argument support is needed only for builder of choco, so it's kinda nice-to-have command line API addition - not primary focus for me.

What I have checked by myself - I have misused Spectre console - there was simpler interface for declaring command line arguments. In choco there is used slightly different API would be nice to harmonize them at some point of time to be the same.

@FrankRay78
Copy link
Contributor

I have raised the following issue on the spectre.console project, which (if done), would fully close off this issue: spectreconsole/spectre.console#1059

@FrankRay78
Copy link
Contributor

FrankRay78 commented Nov 11, 2022

I have implemented the change to allow flags to be converted to remaining arguments if they cannot be assigned, and submitted the PR for this: spectreconsole/spectre.console#1102

Once merged and a new version of spectre.console is available, turning this behaviour on should be a 1 line code change in the cake codebase 🙂

@tapika
Copy link

tapika commented Dec 2, 2022

FYI: Potentially unrelated, but might be related in future:

#4074

@FrankRay78
Copy link
Contributor

Update @augustoproiete - I have asked the maintainers of spectre to prioritise the above PR, so once merged, hopefully I can address this cake issue.

@augustoproiete
Copy link
Member Author

Thanks for chasing @FrankRay78 !

@FrankRay78
Copy link
Contributor

PR #4142, which fully addresses this issue, is now green and ready for review/merging.

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

Successfully merging a pull request may close this issue.

4 participants