Fix `fromRawWindowsCommandLine` (now `fromRawCommandLine`), fixes #2197 #2206

matthid · 2018-11-18T12:24:18Z

Description

matthid · 2018-11-18T12:25:43Z

src/test/Fake.Core.UnitTests/Fake.DotNet.FxCop.fs

@@ -178,7 +178,7 @@ let testCases =
              let assemblies = [ Guid.NewGuid().ToString() ]
              let proc = FxCop.composeCommandLine p assemblies
              let expected =
-                  sprintf """%s /c "/f:\"%s\"" "/o:\"%s\"" /s /v""" dummy
+                  sprintf """%s /c /f:\"%s\" /o:\"%s\" /s /v""" dummy


Seems like there was a slight behavior change here. @SteveGilham I hope this still works. Actually I'm a bit puzzled that this even works with the argument containing the quoted quotes.

The escaping of quotes is required when they fall within a quoted parameter, and only then.
This style works

"/o:\"_Reports/FxCopReport.xml\""

and this style works

/o:"_Reports/FxCopReport.xml"

but this one (the one as per the diff above) doesn't

/o:\"_Reports/FxCopReport.xml\"

I feel like this command line style is similar to the problem this PR tries to solve (#2197)

Does fxcop accept "/o:_Reports/FxCopReport.xml" (ie without the inner quotes)?
Which would simplify this.

Yes (with the proviso that the file name contains no spaces).

So "/o:_Reports/with space/FxCopReport.xml" doesn't work even it is correctly escaped and given to fxcop as single argument?

"/o:_Reports/with space/FxCopReport.xml" works it's /o:_Reports/with space/FxCopReport.xml that, of course, doesn't.

Ok thanks for confirming, than we should do it like ed0bb86.

matthid · 2018-11-18T15:21:47Z

I think this is actually ready, but I'd like to wait until @vbfox reviews and gives feedback about concat, because once released it will break people if BlackFox.CommandLine is updated...

vbfox · 2018-11-18T22:58:55Z

Concat API is made for being piped and it's ordering is weird if not used like that... It's documented but I admit it's weird.

I'm tempted to align the lib to the standard modules naming, renaming appendXXX to addXXX, having append concatenate 2 CmdLine and concat take a sequence of CmdLine might as well do that now and release a 1.0 then not change the API anymore.

vbfox · 2018-11-18T23:14:00Z

So in the end I won't change Append but i'll change concat signature to go with the CmdLine seq one so it's clearer what's going on

(Breaking change, see fsprojects/FAKE#2206 for the confusion created by the previous API)

vbfox · 2018-11-18T23:40:49Z

Ok 1.0.0 published, let's see !

src/app/Fake.Core.Process/CmdLineParsing.fs

vbfox · 2018-11-19T08:32:47Z

src/app/Fake.Core.Process/CmdLineParsing.fs

+        { Args = a.Args |> CmdLine.appendSeq s }
+        //Arguments.OfArgs(Seq.append a.Args s)
+
+/// Forward API from https://github.com/vbfox/FoxSharp/tree/master/src/BlackFox.CommandLine


I think I understand why but it mean that the API in FAKE might diverge at some point.

Also you're losing both the "." style API and doc comments

If the APIs diverge something most likely breaks?

Yes I was not speaking about breakage, mostly playing catch-up with whatever is added on one side needing an equivalent PR on the other

vbfox · 2018-11-19T08:34:15Z

src/app/Fake.Core.Process/CmdLineParsing.fs

+    let fromArray s =  { Args = CmdLine.fromArray s }
+    let toList (a:Arguments) = CmdLine.toList a.Args
+    let toArray (a:Arguments) = CmdLine.toArray a.Args
+    let toStringForMsvcr e (a:Arguments) = CmdLine.toStringForMsvcr e a.Args


This might be a little too low level.

If you wrap, maybe add a fromCmdLine or something and users can go to my lib directly if they need advanced features

Yes I just don't want people to convert back and forth ... Maybe with a different entry point? (see new changes)

Co-Authored-By: matthid <[email protected]>

matthid · 2018-11-19T20:05:23Z

Ok 1.0.0 published, let's see !

Well, I hope you are a bit more careful with releasing breaking changes in the future ;)

matthid · 2018-11-19T20:18:46Z

src/app/Fake.Core.Process/CreateProcess.fs

+    ///     |> CreateProcess.fromCmdLine "dotnet.exe"
+    ///     |> Proc.run
+    ///     |> ignore
+    let fromCmdLine command args =


@vbfox Maybe that is Okish if we says fake only has build-in APIs for working with argument lists and properly escaping them and other weird stuff (or the builder-API) is external

I like that but i'm not 100% objective on the subjects :)

It also suggest that other builder libraries could happen.

If needed I can produce another package with only the escaping part completely separate from the builder one. FAKE core would then depend on it and each FAKE package could use whatever builder it wants.

Not sure it's worth the complexity but it's doable

matthid · 2018-11-19T20:20:54Z

Honestly, somehow I'm a bit nervous with this change. Probably because the dependency is "out of control" and a potential source of breakage at the very core ;)

kblohm · 2018-11-19T20:29:56Z

I feel like something that is so core to fake should preferably be included out ouf the box. Every module should probably use this api in the long run, so having a bit more control might be nice.

vbfox · 2018-11-19T21:01:51Z

Well, I hope you are a bit more careful with releasing breaking changes in the future ;)

I'll follow SemVer as much as I can so not before 2.0 ;) whenever it's soon or not will depend on problems found XD

Anything less than 1.0 is free for all (That's also why I bumped VsWhere, no way to change it now it's too used)

vbfox · 2018-11-19T21:20:28Z

Honestly, somehow I'm a bit nervous with this change. Probably because the dependency is "out of control" and a potential source of breakage at the very core ;)

I can't really help much on that part, taking dependencies is always a risk. And it's also a bit scary for me too.

I added you as a collaborator on the repo (There is also VsWhere that FAKE references) so if you ends up using it we can work together keeping the lib compatible.

matthid · 2018-11-19T21:46:38Z

@vbfox
Or maybe asked differently: Is there any particular reason you think it should be separate?
Usually when building command line you also want to start the process afterwards, which is exactly what Fake.Core.Process does.

Also related: We could include the thing via paket-files, but that is not something I'd like to do for various reasons (just something I wanted to mention here)

…e runtime)

vbfox · 2018-11-19T22:06:57Z

Or maybe asked differently: Is there any particular reason you think it should be separate?

Well I use it in FAKE libraries but also in code not linked to FAKE at all and Fake.Core.Process is too much linked to FAKE to be usable outside of it. So I personally and professionally need a separate library & will continue to maintain it.

Now why FAKE shouldn't copy paste the code, then include it I don't have any good reason except maintenance (Always the consequence of NIH syndrome).

Usually when building command line you also want to start the process afterwards

Once you have a command line string, the framework contains a perfectly suitable class to start a process ;)

vbfox · 2018-11-19T22:09:51Z

@matthid we can discuss on gitter/slack whatever if you want

matthid · 2018-11-19T22:26:04Z

Well I use it in FAKE libraries but also in code not linked to FAKE at all and Fake.Core.Process is too much linked to FAKE to be usable outside of it.

It's a bit of an outside discussion, but it was one of the design goals to be able to use the packages standalone which we didn't achieve apparently. It would be nice if you could elaborate on that (either here or a separate issue). However that is only partially related to this.

we can discuss on gitter/slack whatever if you want

Sure, feel free to ping me :). However, there shouldn't be the impression that I own fake and need to be convinced. In fact it feels like there might be a deeper issue (maybe with how I maintain the repository?!) which we hopefully uncover with the discussion above.

Personally, I'm probably fine with merging as is, but I can see users asking why they need to use some other component to build command lines or open BlackFox... it's also something that is not "consistent". I learned the hard way that some people really really really like consistency ;)

Also the aspect from @kblohm

…s workaround for #2197 and usage of external libraries like https://github.com/vbfox/FoxSharp/tree/master/src/BlackFox.CommandLine

matthid · 2018-12-03T20:13:28Z

OK I think I have figured out how I want to do this. Basically the problem of #2197 was that there is no "raw" API. With the most recent commit I changed fromRawCommandLine (previously fromRawWindowsCommandLine) to actually behave properly and use the exact given string (if no transformations are made). There are probably still edge cases when you actually do transformations, but at least you can workaround now.
Also I removed BlackFox library from the public API surface as I feel like that is actually a cross cutting concern and not really required for this "bugfix".

If there is no reason against this change this is probably how I want to do it. What do you think @vbfox and @kblohm ? I also added documentation on how you can use BlackFox.CommandLine with this "fixed" API.

vbfox · 2018-12-03T20:58:01Z

👍, It does a double parsing when an external lib is used but it can be removed if needed and that's not much of an issue.

I'm a fan of small libs & compositions 😉

kblohm · 2018-12-03T22:04:06Z

I feel like support to build commandline-arguments should be included. Starting external processes is one of the main use-cases of FAKE, so it should be as easy as possible.

I learned the hard way that some people really really really like consistency ;)

Consistency does make it easier to get started. I looked into some of the existing modules before i started on one and all of them did the command-line stuff differently. At least for first time contributors, a dedicated way of doing all the command-line-building would make it easier.

switch to BlackFox.CommandLine, first step in fixing #2197

b880698

matthid commented Nov 18, 2018

View reviewed changes

matthid mentioned this pull request Nov 18, 2018

Incorrect argument escaping #2197

Closed

matthid added 3 commits November 18, 2018 14:28

concat is broken @vbfox

61ea463

Merge branch 'release/next' into fix_2197

05016e4

change comment line escaping accoding to feedback

ed0bb86

vbfox added a commit to vbfox/FoxSharp that referenced this pull request Nov 18, 2018

CmdLine.concat changed to be similar to standard modules

28d9623

(Breaking change, see fsprojects/FAKE#2206 for the confusion created by the previous API)

vbfox reviewed Nov 19, 2018

View reviewed changes

src/app/Fake.Core.Process/CmdLineParsing.fs Outdated Show resolved Hide resolved

vbfox reviewed Nov 19, 2018

View reviewed changes

Update src/app/Fake.Core.Process/CmdLineParsing.fs

15b837f

Co-Authored-By: matthid <[email protected]>

matthid added 3 commits November 19, 2018 21:12

use a different entry point as suggested.

07fd826

Merge branch 'fix_2197' of github.com:fsharp/FAKE into fix_2197

d32dd57

whoops

832e16b

matthid commented Nov 19, 2018

View reviewed changes

paket update

a85d137

matthid added 2 commits November 19, 2018 22:55

add some apis required for testing

b263685

we should agree on paket.core (or at least not use an older one in th…

bfe4035

…e runtime)

matthid added 3 commits December 3, 2018 19:12

Merge branch 'release/next' of github.com:fsharp/FAKE into fix_2197

4136096

Make sure fromRawCommandLine actually keeps whatever it gets, allow…

ba41f8f

…s workaround for #2197 and usage of external libraries like https://github.com/vbfox/FoxSharp/tree/master/src/BlackFox.CommandLine

blackfox commandline not actually required

134cb96

matthid changed the title ~~switch to BlackFox.CommandLine, first step in fixing #2197~~ Fix fromRawWindowsCommandLine (now fromRawCommandLine), fixes #2197 Dec 3, 2018

matthid merged commit 6bbc98b into release/next Dec 3, 2018

matthid deleted the fix_2197 branch March 17, 2019 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `fromRawWindowsCommandLine` (now `fromRawCommandLine`), fixes #2197 #2206

Fix `fromRawWindowsCommandLine` (now `fromRawCommandLine`), fixes #2197 #2206

matthid commented Nov 18, 2018

matthid Nov 18, 2018

SteveGilham Nov 18, 2018

matthid Nov 18, 2018

SteveGilham Nov 18, 2018

matthid Nov 18, 2018

SteveGilham Nov 18, 2018

matthid Nov 18, 2018

matthid commented Nov 18, 2018

vbfox commented Nov 18, 2018

vbfox commented Nov 18, 2018

vbfox commented Nov 18, 2018

vbfox Nov 19, 2018

matthid Nov 19, 2018

vbfox Nov 19, 2018

vbfox Nov 19, 2018

matthid Nov 19, 2018

matthid commented Nov 19, 2018

matthid Nov 19, 2018

vbfox Nov 19, 2018

matthid commented Nov 19, 2018

kblohm commented Nov 19, 2018

vbfox commented Nov 19, 2018

vbfox commented Nov 19, 2018

matthid commented Nov 19, 2018

vbfox commented Nov 19, 2018

vbfox commented Nov 19, 2018

matthid commented Nov 19, 2018

matthid commented Dec 3, 2018

vbfox commented Dec 3, 2018

kblohm commented Dec 3, 2018

Fix fromRawWindowsCommandLine (now fromRawCommandLine), fixes #2197 #2206

Fix fromRawWindowsCommandLine (now fromRawCommandLine), fixes #2197 #2206

Conversation

matthid commented Nov 18, 2018

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Nov 18, 2018

vbfox commented Nov 18, 2018

vbfox commented Nov 18, 2018

vbfox commented Nov 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Nov 19, 2018

kblohm commented Nov 19, 2018

vbfox commented Nov 19, 2018

vbfox commented Nov 19, 2018

matthid commented Nov 19, 2018

vbfox commented Nov 19, 2018

vbfox commented Nov 19, 2018

matthid commented Nov 19, 2018

matthid commented Dec 3, 2018

vbfox commented Dec 3, 2018

kblohm commented Dec 3, 2018

Fix `fromRawWindowsCommandLine` (now `fromRawCommandLine`), fixes #2197 #2206

Fix `fromRawWindowsCommandLine` (now `fromRawCommandLine`), fixes #2197 #2206