Improve the driver downloader

[0xced]

• Use Spectre.Console.Cli instead of CommandLineParser, which provides a free spinner while downloading drivers.
• Automatically compute a default for the --basepath option (it would not even be required anymore).
• Actually run the EnsurePrerequisitsRan target so that people trying to build the Playwright project have a clear error with the solution to download the missing drivers.

[mxschmitt]

Thank you for your contribution, but since this is not a user-facing change we'd prefer to keep this as it is. Changing command line parsers and code around introduces a risk of breaking our dev workflows Als this dependency is not very popular which opens it up for potential supply chain attacks.

Thanks for your understanding!

[0xced]

since this is not a user-facing change we'd prefer to keep this as it is.

I think being a developer-only feature doesn't preclude it from being friendly. Using Spectre.Console opens up the possibility for a much better experience than the current implementation with progress bars, akin the Mongo2Go downloader: https://asciinema.org/a/7JtLaRBbyNdIINiTu2pWIw6pd That's what I had in mind for future improvements based on Spectre.Console.

Changing command line parsers and code around introduces a risk of breaking our dev workflows.

We are talking about a command line tool that accepts a single command (download-drivers) with a single argument (--basepath). The proposed change keeps compatibility with the previous implementation while adding some improvements.

• --basepath is not required anymore, it becomes optional thanks to the use of the CallerFilePath attribute.
• The --help prints more comprehensible and more correct information.

Before:

$ dotnet run -- --help
Playwright.Tooling 1.0.0+510f4934ce86377fefaa2fd910ad6c6b987cf15a
Copyright (C) 2023 Playwright.Tooling

  --basepath    Required. Solution path.

  --help        Display this help screen.

  --version     Display version information.

After:

$ dotnet run -- --help
USAGE:
    Playwright.Tooling.dll [OPTIONS] <COMMAND>

OPTIONS:
    -h, --help       Prints help information   
    -v, --version    Prints version information

COMMANDS:
    download-drivers    Downloads the playwright drivers for all supported platforms

$ dotnet run -- download-drivers --help
DESCRIPTION:
Downloads the playwright drivers for all supported platforms.

USAGE:
    Playwright.Tooling.dll download-drivers [OPTIONS]

OPTIONS:
    -h, --help        Prints help information
        --basepath    Solution path          

Also this dependency is not very popular which opens it up for potential supply chain attacks.

Well, Spectre.Console has 7.7k stars on GitHub vs 4.2k for CommandLineParser. It has a team a 5 maintainers (full disclosure: which I'm part of) who diligently review pull requests before merging. It is supported by the .NET Foundation, has an up to date documentation web site and scans pull requests with Snyk to automatically find potential vulnerabilities. Overall, it's a pretty good OSS citizen. I can't really agree with you on the potential supply chain attack here.

Finally, I also fixed the EnsurePrerequisitsRan target which did never run previously. 😉 I was lucky to stumble on that target when working on my previous contribution (#2740) to understand what command I needed to run in order to get the project to compile.

[mxschmitt]

We appreciate the contribution but we'll close it as per #2752 (comment) for now since it does not include any user-facing changes / fix something and introduces a new dependency.

[0xced]

I'm disappointed that this was not reconsidered. 😔

does not include any user-facing changes

Well, it adds a spinner indicating that the drivers are being downloaded, the help text is improved and the --basepath is not required anymore. 🤷‍♂️

does not fix something

The EnsurePrerequisitsRan target which did never run previously was fixed. I have the feeling that my last reply (where I talk about it) was skimmed very very fast and not considered at all.

introduces a new dependency

Well, it removes another one.