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

Update PMI to use the new System.CommandLine parser #209

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

AndyAyersMS
Copy link
Member

Replace PMI's home-grown command line parsing with the new System.CommandLine
version.

Syntax changes slightly as the "suffix" options (eg PREPALL-QUIET) now need
to be separated out (PREPALL --quiet). Only real "breaking" change is that the
method index for PREPALL and PREPONE is now a named option.

With this new package we get built in help and (in some shells) command line
completion.

I plan to eventually migrate all the utilites over to this package as the one
they are using is now orphaned and unsupported. See #193.

Replace PMI's home-grown command line parsing with the new System.CommandLine
version.

Syntax changes slightly as the "suffix" options (eg `PREPALL-QUIET`) now need
to be separated out (`PREPALL --quiet`). Only real "breaking" change is that the
method index for `PREPALL` and `PREPONE` is now a named option.

With this new package we get built in help and (in some shells) command line
completion.

I plan to eventually migrate all the utilites over to this package as the one
they are using is now orphaned and unsupported. See dotnet#193.
@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

This also includes two bug fixes for PREPONE -- one to format the elapsed time on the same line, the other to break out of a generic instantiation loop properly.

Some examples. Overall help:

> dotnet exec D:\repos\jitutils\src\pmi\bin\Debug\netcoreapp3.0\pmi.dll --help
Usage:
  pmi [options] [command]

Options:
  --version    Display version information

Commands:
  COUNT <assemblyName>       Count the number of types and methods in an assembly.
  PREPALL <assemblyName>     JIT all the methods in an assembly or all assemblies in a directory tree.
  PREPONE <assemblyName>     JIT exactly one method in an assembly.
  DRIVEALL <assemblyName>    JIT all the methods in an assembly robustly, skipping methods with asserts.

Per-command help:

>dotnet exec D:\repos\jitutils\src\pmi\bin\Debug\netcoreapp3.0\pmi.dll PREPONE --help
PREPONE:
  JIT exactly one method in an assembly.

Usage:
  pmi PREPONE [options] <assemblyName>

Arguments:
  <assemblyName>    Assembly file to process

Options:
  --method <method>    Index of the method to jit
  --cctors             Try and invoke cctors first
  --quiet              Don't show progress messages
  --time               Show elapsed time information

@mikedn
Copy link
Contributor

mikedn commented May 24, 2019

Are command names case insensitive? If not then perhaps you should make them lower case, upper case commands seem to be the exception rather than the norm.

@AndyAyersMS
Copy link
Member Author

Case sensitive.

I can add "aliases" so you can specify either case; I don't know yet if you can make them case insensitive.

@AndyAyersMS
Copy link
Member Author

Re case sensitivity -- looks like not (yet): dotnet/command-line-api#133

@CarolEidt
Copy link
Contributor

PMI treats its arguments as case-insensitive: https://github.com/dotnet/jitutils/blob/master/src/pmi/pmi.cs#L1329

I always use lower case when invoking it.

@AndyAyersMS
Copy link
Member Author

Unfortunately this causes a version clash with the other tools, because the old fxlabs assembly and this new assembly are both called System.CommandLine and we're "flat publishing" all the jitutils dependencies into one directory.

So I either need to convert everything over to the new format or find some way around this. Was planning on converting everything eventually, so perhaps I'll just start working on that.

Kind of pointless as it is referring to the old CI server, but perhaps
it can serve as a template for something to retrieve things from AzDO.

But it at least shows how to adapt the `Config` we see in the other projects
over to the new format without too radical of changes.
@AndyAyersMS
Copy link
Member Author

I have most of the other utilities converted but am still working on jit-diff...

@AndyAyersMS
Copy link
Member Author

Have everything converted over and (mostly) working, but have hit a new snag: pmi needs to be able to run in the locally built coreclr, and there's a copy of the old System.CommandLine in core root.

Looks like it comes from R2R dump perhaps. So we may need to update that too.

@BruceForstall
Copy link
Member

Thanks for doing this conversion. IMO, we should just move everything to lower-case. Not even sure we need upper-case aliases.

@AndyAyersMS
Copy link
Member Author

Converting some of the other tools was a bit messy. Let me push my latest and you can get an idea.

Things I hit:

  • Less flexibility in conveying option value to internal fields. Basically names have to match. We sometimes had quite different names.
  • For options that represent sets of things this name matching is painful, eg "--assembly a.dll" and "--assembly a.dll b.dll" seems natural, but it means the internal property needs to be named Assembly even though it may represent more than one.
  • New parser wants to run the app logic as an async task, this leads to ominous looking stack traces when things go south.
  • Bugs in the new setup
  • The need for all-or-nothing conversion (including R2R dump)

@kunalspathak
Copy link
Member

@AndyAyersMS - I see that R2RDump was the last blocker in getting this PR merged. I see that you had suggested changes in dotnet/runtime#12766. Do you need any help in pushing that through, so we can complete this work item?

@AndyAyersMS
Copy link
Member Author

I no longer think this is a good idea -- I like the behavior of the current parser better.

Suggest we instead find a better supported implementation of what we have now -- see #193 (comment)

Base automatically changed from master to main March 10, 2021 13:05
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.

6 participants