[MSBuild] Fixed trailing garbage #2739

0x53A · 2023-03-10T10:49:05Z

The previous use of appendQuotedIfNotNull doesn't make any sense, it passes Some as a function as the first parameter, which then get's formatted through %A into a string like <fun:quoteString@686>. The intention was probably (Some str), but this again doesn't work.

Just wrapping it in quotes is probably not strictly correct, we would also need to escape existing quotes inside the string, but it quickly fixes the code without a regression.

yazeedobaid · 2023-03-13T09:01:56Z

Thanks for the PR. I think append with an object formatting gives extra options for parameters passed. For example, passing objects with custom toString methods. I'm not aware of the history of this function. But as it is used in MSBuild and AppVeyor modules yes it doesn't make sense. In TypeScript module it is used with a string passed as the first parameter.

For the use case, as you said it should also account for nested quotes, so can you please include that in PR. I digged more into legacy process module and found a similar helper that did this (legacy process module):

/// Adds quotes around the string
/// [omit]
[<System.Obsolete("FAKE0001 Use the Fake.Core.Process module instead")>]
let quote (str:string) = "\"" + str.Replace("\"","\\\"") + "\""

/// Adds quotes around the string if needed
/// [omit]
[<System.Obsolete("FAKE0001 Use the Fake.Core.Process module instead")>]
let quoteIfNeeded str =
    if isNullOrEmpty str then ""
    elif str.Contains " " then quote str
    else str

0x53A · 2023-03-13T22:22:59Z

Unfortunately, just quoting isn't enough, see https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way for the mess that is command line parsing.

Fortunately, the process module of Fake already implements this, and by using fromRawCommand instead of fromRawCommandLine we can pass the arg array down to that level instead of having to escape it ourselves.

0x53A · 2023-03-13T22:57:51Z

I'll leave it at this for today - I believe it should be more or less correct, but I have not done any manual tests

0x53A · 2023-03-15T06:35:14Z

uhhh, how much do y'all trust the unit/integration test coverage? From my side I believe it's ready, but I have (still) not done any manual testing.

yazeedobaid · 2023-03-16T09:21:30Z

src/app/Fake.DotNet.Cli/DotNet.fs

-            addBinaryLogger msBuildArgs.DisableInternalBinLog (args + " " + argString) common
+            addBinaryLogger msBuildArgs.DisableInternalBinLog (args @ argString) common


Why space has been removed here?

Before, args and argString were plain strings, which were just concated with a space between them.

Now, args and argString are both string list, which get combined into a single list with the @ operator.

The later code then uses either Args.toWindowsCommandLine (or CreateProcess.fromRawCommand) to convert the string list into a single, properly quoted, string.

with all that said I should also rename the variable from argString to argList

yazeedobaid · 2023-03-16T09:22:07Z

src/app/Fake.BuildServer.AppVeyor/AppVeyorInternal.fs

+    /// Adds quotes around the string
+    /// [omit]


Can you please change the function documentation to XML format?

yazeedobaid · 2023-03-16T09:22:16Z

src/app/Fake.BuildServer.AppVeyor/AppVeyorInternal.fs

+    /// Adds quotes around the string if needed
+    /// [omit]


yazeedobaid · 2023-03-16T09:22:40Z

src/app/Fake.DotNet.Cli/DotNet.fs

                options

        run cmdArgs options

+    /// the 'exec' above with args as a single string is public and therefore can't be changed, create a second overload with takes a string list


Can you please change the function documentation to XML format?

yazeedobaid · 2023-03-16T09:25:25Z

src/app/Fake.DotNet.MSBuild/MSBuild.fs

+        let p = MSBuildParams.Create() |> setParams
+        p, fromCliArguments p.CliArguments |> Args.toWindowsCommandLine
+
+    /// [omit]


no need to omit comment, if the function will not make it to documentation then u can use visibility modified, maybe change it to internal? Also, can you come up with another name instead of buildArgs2. While reading this, I had to search if buildArgs exists and compare both of them to see the difference.

yazeedobaid · 2023-03-16T09:26:23Z

src/app/Fake.DotNet.Cli/DotNet.fs

                options

        run cmdArgs options

+    /// the 'exec' above with args as a single string is public and therefore can't be changed, create a second overload with takes a string list
+    let private exec2 (buildOptions: Options -> Options) (command: string) (args: string list) =


same as with buildArgs2, can you come up with a descriptive name instead of exec2

yazeedobaid · 2023-03-16T09:30:04Z

Thanks for the new changes. I have added some comments about them.

Regarding handling the other cases in addition to just escaping quotes, that was discussed recently in #2735.
@matthid could you please advise on this and the changes made in this PR to replace fromRawCommandLine with fromRawCommand?

0x53A · 2023-03-17T18:25:58Z

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

@@ -140,7 +140,7 @@ type FilePath = string
 module Args =
    /// <summary>
    /// Convert the given argument list to a conforming windows command line string, escapes parameter in quotes if
-    /// needed (currently always but this might change).
+    /// needed.


this just corrects the comment, the function already did only quote and escape if required.

0x53A · 2023-03-17T18:26:34Z

src/app/Fake.BuildServer.AppVeyor/AppVeyorInternal.fs

-        StringBuilder()
-        |> StringBuilder.appendQuotedIfNotNull Some str
-        |> StringBuilder.toText
+    /// quote and escape the single argument, if required.


all other functions in this file use simple non-xml comments, and it's private anyway

0x53A · 2023-03-17T18:26:57Z

src/app/Fake.DotNet.Cli/DotNet.fs

+    /// <param name="buildOptions">build common execution options</param>
+    /// <param name="command">the sdk command to execute <c>test</c>, <c>new</c>, <c>build</c>, ...</param>
+    /// <param name="args">command arguments</param>
+    let private execArgsList (buildOptions: Options -> Options) (command: string) (args: string list) =


renamed exec2 to execArgsList

0x53A · 2023-03-17T18:28:21Z

src/app/Fake.DotNet.MSBuild/MSBuild.fs


    /// [omit]
-    let buildArgs (setParams: MSBuildParams -> MSBuildParams) =
+    let buildArgs (setParams: MSBuildParams -> MSBuildParams) : MSBuildParams * string =


I added explicit return type annotation (: MSBuildParams * string) to both variants

voronoipotato · 2023-05-26T18:28:07Z

Who has the rights to merge this? @yazeedobaid do you or is it just @matthid ? If it's just @matthid maybe we could get a secondary on this? I'd be open to being a temporary secondary (I use F# and Fake daily at work) until we can find someone who is a regular contributor.

kantora · 2023-07-30T19:31:11Z

Any hints when this could be merged? Can something to be done to push this? As I can understand - without this fix Fake.MSBuild is totally unusable.

Still waiting for fsprojects/FAKE#2739 for the proper workflow

voronoipotato · 2023-07-31T13:18:34Z

maybe we should fork and call it fmake.

fixes fsprojects#2738

…gs ourselves

xperiandri · 2023-10-01T13:43:04Z

Have we finished all the discussions and are confident to merge?

xperiandri · 2023-10-01T16:01:01Z

@0x53A you may press a merge button now, I suppose it is available

0x53A · 2023-10-29T04:45:04Z

0x53A closed this Mar 10, 2023

0x53A reopened this Mar 10, 2023

0x53A closed this Mar 14, 2023

0x53A reopened this Mar 14, 2023

yazeedobaid reviewed Mar 16, 2023

View reviewed changes

yazeedobaid requested a review from matthid March 16, 2023 09:30

yazeedobaid mentioned this pull request Mar 16, 2023

StringBuilder.appendQuotedIfNotNull is used incorrectly. #2728

Open

0x53A commented Mar 17, 2023

View reviewed changes

0x53A closed this Mar 17, 2023

0x53A reopened this Mar 17, 2023

0x53A closed this Mar 20, 2023

0x53A reopened this Mar 20, 2023

kantora added a commit to kantora/KlusterKite that referenced this pull request Jul 30, 2023

Build scripts are fixed to Fake 6

f70e9e3

Still waiting for fsprojects/FAKE#2739 for the proper workflow

xperiandri changed the title ~~msbuild: fix trailing garbage~~ [MSBuild] Fixed trailing garbage Oct 1, 2023

0x53A added 14 commits October 1, 2023 17:41

msbuild: fix trailing garbage

aaab0e4

fixes fsprojects#2738

run fantomas

91f6fe9

[msbuild] pass arg array to process module instead of escaping the ar…

27d049d

…gs ourselves

[msbuild] re previous commit: restore exact same log output

058888d

[msbuild/dotnet.cli] fix build

27dbb6f

dotnet fantomas src/app/ src/template/ src/test/ --recurse

d124bdb

[msbuild] remove unused function

077c021

[dotnet.cli] undo change to public api

08f8d99

[msbuild] undo change to public api

f55eab2

dotnet fantomas src/app/ src/template/ src/test/ --recurse

b3a05a4

[appveyor] quoting

46754b1

fantomas

ecbf514

[msbuild/dotnet/appveyor] quoting, reduce diff, xml comments

cf3ba7c

dotnet fantomas src/app/ src/template/ src/test/ --recurse

74545ff

xperiandri force-pushed the msbuild/trailing-garbage branch from 215c1a7 to 74545ff Compare October 1, 2023 13:41

xperiandri approved these changes Oct 1, 2023

View reviewed changes

xperiandri merged commit 2de6216 into fsprojects:master Oct 29, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MSBuild] Fixed trailing garbage #2739

[MSBuild] Fixed trailing garbage #2739

0x53A commented Mar 10, 2023 •

edited

Loading

yazeedobaid commented Mar 13, 2023

0x53A commented Mar 13, 2023

0x53A commented Mar 13, 2023

0x53A commented Mar 15, 2023

yazeedobaid Mar 16, 2023

0x53A Mar 17, 2023

0x53A Mar 17, 2023

yazeedobaid Mar 16, 2023

yazeedobaid Mar 16, 2023

yazeedobaid Mar 16, 2023

yazeedobaid Mar 16, 2023

yazeedobaid Mar 16, 2023

yazeedobaid commented Mar 16, 2023

0x53A Mar 17, 2023

0x53A Mar 17, 2023

0x53A Mar 17, 2023

0x53A Mar 17, 2023

voronoipotato commented May 26, 2023 •

edited

Loading

kantora commented Jul 30, 2023

voronoipotato commented Jul 31, 2023

xperiandri commented Oct 1, 2023

xperiandri commented Oct 1, 2023

0x53A commented Oct 29, 2023

		addBinaryLogger msBuildArgs.DisableInternalBinLog (args + " " + argString) common
		addBinaryLogger msBuildArgs.DisableInternalBinLog (args @ argString) common

[MSBuild] Fixed trailing garbage #2739

[MSBuild] Fixed trailing garbage #2739

Conversation

0x53A commented Mar 10, 2023 • edited Loading

yazeedobaid commented Mar 13, 2023

0x53A commented Mar 13, 2023

0x53A commented Mar 13, 2023

0x53A commented Mar 15, 2023

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

Choose a reason for hiding this comment

yazeedobaid commented Mar 16, 2023

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

voronoipotato commented May 26, 2023 • edited Loading

kantora commented Jul 30, 2023

voronoipotato commented Jul 31, 2023

xperiandri commented Oct 1, 2023

xperiandri commented Oct 1, 2023

0x53A commented Oct 29, 2023

0x53A commented Mar 10, 2023 •

edited

Loading

voronoipotato commented May 26, 2023 •

edited

Loading