Add custom ResponseFile handler #76271

am11 · 2022-09-27T21:07:28Z

Co-authored-by: Jon Sequeira <[email protected]>

src/coreclr/tools/aot/ILCompiler/Program.cs

MichalStrehovsky · 2022-09-28T04:58:42Z

Crossgen2 that was switched over to S.CommandLine in #72082 as well has the same problem. We don't consistently quote things that go into the response file: https://github.com/dotnet/sdk/blob/cf2fd1e514c393cb12e745ab488b6478320fc232/src/Tasks/Microsoft.NET.Build.Tasks/RunReadyToRunCompiler.cs#L324

jkotas · 2022-09-28T05:30:33Z

@am11 Could you please move this next to the other command line helpers and use it for crossgen too?

am11 · 2022-09-28T11:06:00Z

When I was testing it on mac, crossgen2's rsp file had quoted -r:"path with spaces" etc. and ilc's rsp file had without the quotes -r:path with spaces. This handler takes care of latter case. In the former case, this handler will break things because it literally passes "path with spaces" value and the value splitter does not unquote it after tokenization. It is either-or but not both.

I think this inconsistent use of quotes is not ideal for any tool. Quoting all paths wouldn't surprise anyone and it is the known way to support paths with space. Internal CLI parser's has number of issues as well when dealing with -r: foo bar -r:baz on the same line and -r:foo "bar" where quotes are literally part of the path etc.

jkotas · 2022-09-28T14:40:32Z

src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs

            new(new[] { "--reference", "-r" }, result => Helpers.BuildPathDictionay(result.Tokens, false), true, "Reference file(s) for compilation");
        public Option<string> OutputFilePath { get; } =
-            new(new[] { "--out", "-o" }, "Output file path");
+            new(new[] { "--out", "-o" }, result => Helpers.Unquote(result.Tokens), true, "Output file path");


Can the unquoting be done in TryReadResponseFile instead of around every single argument?

I tried it first in TryReadResponseFile and it gets complicated. For the starters, we will need to replicate the "argument-value splitting" logic in order to support all these simultaneously:

path with space /r:path with space -r:path with space --reference path with space // and their quoted variants

Can we strip the first and last quote if the string ends with a quote?

if (s.EndsWith('"')) { int firstQuote = s.IndexOf('"'); if (firstQuote >= 0 && firstQuote < s.Length - 1) s = s[..firstQuote] + s[(firstQuote+1)..^1]; }

It should be good-enough to maintain compatibility with what we had, without the manual stripping of quotes everywhere.

That will work. 👍

jkotas · 2022-09-28T14:54:33Z

I think this inconsistent use of quotes is not ideal for any tool.

I agree. Do you think that it would be better we chase down all places where we write paths to response files and quote them?

jkotas · 2022-09-28T15:04:48Z

Are there examples of command line response files in Linux-native tools? What do they do for paths with spaces?

jkotas · 2022-09-28T15:27:53Z

Are there examples of command line response files in Linux-native tools? What do they do for paths with spaces?

I have tried fromfile_prefix_chars from Python argparse that is probably the most frequently used command line parser out there. It is not doing anything weird with quotes. If you have quotes in the response file, you will get quotes in the parsed argument.

am11 · 2022-09-28T15:46:32Z

Are there examples of command line response files in Linux-native tools? What do they do for paths with spaces?

I have tried fromfile_prefix_chars from Python argparse that is probably the most frequently used command line parser out there. It is not doing anything weird with quotes. If you have quotes in the response file, you will get quotes in the parsed argument.

Can you show the script, how are you comparing and what is expected?

foo.rsp

-x path with spaces
-y "path with spaces"
-z path\ with\ spaces

I expect -x to be path, -y to be "path with spaces" and -z to be path with spaces on a Unix shell. This is exactly what is happening with CommandLine.

jkotas · 2022-09-28T19:44:23Z

The default python argparse handling of response files is geared towards one command line arg per line. It avoid all problems with escaping of quotes, spaces in names, etc. Very simple Unix-like approach. So the response file for your example that gives the expected result would be:

-x
path with spaces
-y
"path with spaces"
-z
path with spaces

I like this as a north-star a lot. I am wondering how we can get closer to it.

I expect -x to be path, -y to be "path with spaces" and -z to be path with spaces on a Unix shell.

Right, it is the shell interpreting all escaping using shell-specific rules. The program that you are invoking does not need to deal with it.

jonsequitur · 2022-09-28T19:51:01Z

For what it's worth, I think this is actually the most useful response file format. Bypassing shell-specific escaping rules and taking each line as entirely literal is the simplest and most easily understood convention among the many I've looked at. (It just also happened to be less common among Windows tools.)

Adding this back as an available option to System.CommandLine is something we could do very easily.

am11 · 2022-09-28T20:01:49Z

-y "path with spaces"

CommandLine with custom handler (which we are adding in this PR) sets this one as string y = "\"path with spaces\"";, so I had to add Unquote helper which is invoked after all the parsing steps are done; post-processing the parsed value passed to consumer. Unquoting in the handler means we need to take responsibility of splitting the arguments and some other steps as well, which makes the handler quite heavy.

Adding this back as an available option to System.CommandLine is something we could do very easily.

This will be great. For now we need to implement both, the handler and post-processor, for the mixed response files; some paths are quoted, others are not in the same response file.

Do you think that it would be better we chase down all places where we write paths to response files and quote them?

I think this is the cleanest solution and default behavior works with it (no custom handler and no extra post-processing required).

jkotas · 2022-09-28T20:19:07Z

I think this is actually the most useful response file format.

+1

Do you think we can have a mode where this .rsp works?

-r:"Path passed the Windows way using quotes"
-r
Path passed the Unix way. It can have " in the middle just fine.

jkotas · 2022-09-28T20:19:32Z

cc @adamsitnik

am11 · 2022-09-28T20:26:41Z

The rsp file which crossgen2 is generating today has this mixed format:

--option1 path with spaces
--option2 "path with spaces"

so a mode "one or the other" wouldn't help.

jkotas · 2022-09-28T20:33:43Z

The rsp file which crossgen2 is generating today has this mixed format:

My idea was:

If there is more than just the option name on the line, expect the rest to be quoted and the quotes to be stripped. This provides compat with the current .rsp files.
If the line is just the option name and nothing else, expect the next line to be taken verbatim as the option value. I gets us to where we want to be. We can switch the .rsp files format to this over time.

jkotas · 2022-09-28T20:39:29Z

I think this is the cleanest solution and default behavior works with it (no custom handler and no extra post-processing required).

Let's do this then. Could you please prepare PR to do it?

jkotas · 2022-09-28T20:47:38Z

I would be also ok with merging this PR if the Helpers.Unquote hack is replaced with centralized logic. It is ok it that the logic is a hack. The Helpers.Unquote calls that are spread around make the command line parsing even more convoluted than what it is.

am11 · 2022-09-28T20:51:47Z

Sure, I will work on a separate PR for runtime first. I will keep this one open as the discussion is on going.

If there is more than just the option name on the line, expect the rest to be quoted and the quotes to be stripped. This provides compat with the current .rsp files.

Just to be clear, for compat, treat the rest of the line as value and trim the quotes from edges (if present). This is because, in a single rsp file generated by the sdk for crossgen2 today, regardless of the operating system:

jkotas

Thank you!

am11 requested a review from MichalStrehovsky as a code owner September 27, 2022 21:07

ghost added area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member labels Sep 27, 2022

am11 requested review from eerhardt and jonsequitur September 27, 2022 21:07

am11 force-pushed the feature/deps/system.commandline_usage2 branch 4 times, most recently from 8f26f5c to 9773bf5 Compare September 27, 2022 23:17

Add custom ResponseFile handler

fc0668e

Co-authored-by: Jon Sequeira <[email protected]>

am11 force-pushed the feature/deps/system.commandline_usage2 branch from 9773bf5 to fc0668e Compare September 27, 2022 23:23

jonsequitur reviewed Sep 27, 2022

View reviewed changes

src/coreclr/tools/aot/ILCompiler/Program.cs Outdated Show resolved Hide resolved

jonsequitur approved these changes Sep 27, 2022

View reviewed changes

am11 added 2 commits September 28, 2022 16:05

Move handler to helpers and use in CG2

8093167

Trim quotes from path-like arguments

9c88d6f

jkotas reviewed Sep 28, 2022

View reviewed changes

Strip leading and trailing quotes from value

ebd3f5f

jkotas approved these changes Sep 29, 2022

View reviewed changes

jkotas merged commit e3ed6c1 into dotnet:main Sep 29, 2022

ghost locked as resolved and limited conversation to collaborators Oct 29, 2022

Add custom ResponseFile handler #76271

Add custom ResponseFile handler #76271

Uh oh!

Conversation

am11 commented Sep 27, 2022

Uh oh!

Uh oh!

MichalStrehovsky commented Sep 28, 2022

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

am11 commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

am11 Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

jkotas Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

am11 Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

jkotas commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

am11 commented Sep 28, 2022

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

jonsequitur commented Sep 28, 2022

Uh oh!

am11 commented Sep 28, 2022

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

am11 commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

jkotas commented Sep 28, 2022

Uh oh!

am11 commented Sep 28, 2022

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

am11 commented Sep 28, 2022 •

edited

Loading

jkotas commented Sep 28, 2022 •

edited

Loading

am11 commented Sep 28, 2022 •

edited

Loading