jacereda / fsatrace

Filesystem access tracer
ISC License
78 stars 12 forks source link

Support @foo command lines #5

Closed ndmitchell closed 8 years ago

ndmitchell commented 8 years ago

In trying to support the Shell command in Shake as https://github.com/ndmitchell/shake/issues/308. Part of the problem is that on Windows the built-in process stuff in Haskell does lots of quote mangling, without giving me the chance to opt out. If @foo.txt as a command line just read foo.txt and took the command line from there (as many programs already support) then I could skip the Haskell mangling and get exactly what I was after from fsatrace.

Some programs treat @foo.txt files as having one argument per line, others as one single line. I'd probably be tempted for Windows to join all lines with a space, and for Linux pass each line as a separate argument - that gives full flexibility and is simple.

jacereda commented 8 years ago

I prefer one argument per line. Let me check how hard would it be to turn that into a single string for CreateProcess().

ndmitchell commented 8 years ago

For maximum flexibility (basically so I can control the string to CreateProcess) it would be nice if you just replaced each line with a space, then people can tailor the command line exactly. If you do any quoting etc around the arguments then I no longer have full flexibility.

ndmitchell commented 8 years ago

For info, I managed to sort out the Shell stuff in Shake without requiring this, although it feels more fragile than would be ideal.

jacereda commented 8 years ago

Hmm, I would say if any of the lines isn't properly passed as is to the invoked program we can consider that as a bug in fsatrace. Having said that, what would you expect from a sequence like this?

fsatrace - -- ls *

and from an invocation with an args file containing the following?

ls
*

The second case seems more clear, no shell expansion should happen on any platform since those are the arguments we want to pass verbatim.

The former I'm not sure. I would prefer uniform behaviour across platforms and that probably means disabling glob expansion in fsatrace arguments (just a matter of linking with a different CRT object).

That means invoking the above from the command line on Windows will yield a different result from just invoking ls * in the cmd prompt, but I'm ok with that since the tool is supposed to be invoked from a build system with the right argv[]

jacereda commented 8 years ago

This branch has an implementation of this feature:

https://github.com/jacereda/fsatrace/tree/file-args

ndmitchell commented 8 years ago

Agreed, I think having fsatrace do no shell expansion is the right approach - anyone who wants that can prepend cmd /c.

ndmitchell commented 8 years ago

Merging this branch gives me:

make: *** No rule to make target `win/emit.h', needed by `win/fsatracedll32.o'.  Stop.

A make clean fixed it - not sure if that's a missing dependency somewhere?

What I'm really trying to do is create the CreateProcess command line of

cmd /c whatever the user typed unmodified

If I do:

cmd
/c
output\command\shake_helper.exe "otest x"

Then you generate:

cmd /c "output\command\shake_helper.exe \"otest x\""

Which is wrong - escaping " with \" is incorrect on Windows - it should be double or triple quote, but in insanely complicated ways which are virtually impossible to get right (and aren't even total - there are certain quotes you can't properly escape).

My favourite would be to just do unlines for Windows, and not try and reescape them, because there is no robust way of doing it - just expect the person producing the input to properly craft it. I'd be fine if this was behind a --direct flag or similar. Alternatively, maybe @@file to be the "I know what I'm doing" command, which maybe fits better with your existing syntax.

(With this, I think I'll have nailed the Shake/Shell issues entirely, and fsatrace will be able to do everything properly.)

ndmitchell commented 8 years ago

Oh, the argv output also seems a little messed up:

C:\Neil\shake>fsatrace bob.txt -- @cmds.txt
cmd /c "output\command\shake_helper.exe \"otest x\""
shake_helper.exe: shake_helper.hs:(11,9)-(19,41): Non-exhaustive patterns in case

fsatrace.exe(7504): error: command failed with code 1:
argv[0]=
argv[0]=
argv[0]=
argv[0]=
argv[0]=
argv[0]=
ar
argv[1]=v[0]=
argv[0]=
...
jacereda commented 8 years ago

The args should be dumped properly in https://github.com/jacereda/fsatrace/tree/file-args

ndmitchell commented 8 years ago

Confirmed, the args are now dumped properly. I still would like a way to do the carefully crafted command line though.

ndmitchell commented 8 years ago

As an example of where the precise command line is required, start cmd and start "cmd" do different things - the first quoted argument to start is treated as the title, whereas if the first is not quoted its treated as a command.

jacereda commented 8 years ago

I think I'll try the @@args syntax. In any case I'm tempted to say your example should be handled properly if the args are:

start
cmd
...

or:

start
"cmd"
...

And if they are not I want to fix that in the @args syntax too. IIRC args that are already quoted are passed verbatim.

ndmitchell commented 8 years ago

Quite possibly, if you consider something which is only alpha characters to be already quoted. My concern is if I want to pass foo " bar or similar to a command line I'll have to reverse your "when to escape" logic and perhaps there won't be an input that can produce the output I am after. For Shake, I also don't want to have to munge the users input at all, which is why @@ is preferable.

jacereda commented 8 years ago

Given we can just create a .bat file and run that, I think I'll also remove the @foo handling, is that OK?

ndmitchell commented 8 years ago

From Shake usage, yes. Although having an @ variant of other commands sometimes comes in handy, so it might be a future feature request from someone. That said, if it becomes more of a library, then an @ version doesn't make as much sense.