search

fsprojects / FAKE

FAKE - F# Make
https://fake.build
Other
1.28k stars 582 forks source link

[MSBuild] Fixed trailing garbage #2739

Closed 0x53A closed 8 months ago

0x53A commented 1 year ago

fixes https://github.com/fsprojects/FAKE/issues/2738

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.

image

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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?

voronoipotato commented 1 year ago

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 commented 11 months ago

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.

voronoipotato commented 11 months ago

maybe we should fork and call it fmake.

xperiandri commented 9 months ago

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

xperiandri commented 9 months ago

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

0x53A commented 8 months ago
image