Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSBuild.build generates an incorrect command line (FAKE 5) #2112

Closed
softlion opened this issue Sep 27, 2018 · 14 comments

Comments

Projects
None yet
2 participants
@softlion
Copy link

commented Sep 27, 2018

Description

The following script part generates a command line which fails, because the /p switch is inside a string:

Target.create "SchemaCompare" (fun _ ->
    let setParams (defaults:MSBuildParams) =
            { defaults with
                Verbosity = Some(Quiet)
                Targets = ["SqlSchemaCompare"]
                Properties =
                    [
                        "SqlScmpFilePath", scmpFilePath
                        "source", sourceDacpac
                        "target", targetConnectionString
                        "Deploy","true"
                    ]
            }

    MSBuild.build setParams "myproj.sqlproj"
)

Generates this command line, which fails:

"C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\MSBuild.exe" "myproj.sqlproj" "/t:SqlSchemaCompare" "/m" "/nodeReuse:False" "/v:q" "/p:RestorePackages=False" "/p:SqlScmpFilePath=C:\xxx\xxx.scmp" "/p:source=C:\xxx\Output\xxx.dacpac" "/p:target=Data Source=xxx,1433;Initial Catalog=xxx;User Id=xxx;Password=xxx;Integrated Security=False;Persist Security Info=True;Connect Timeout=30;Encrypt=True;MultipleActiveResultSets=True" "/p:Deploy=true" "/bl:C:\xxx\Temp\tmpxxxx.tmp.binlog"

Repro steps

see above.

Expected behavior

no fail.

Actual behavior

MSBUILD : error MSB1006: property is not valid
Switch : 1433

Known workarounds

A command which works would be the following one. The only difference is the quote.

/p:"target=..." instead of "/p:target=..."

"C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\MSBuild.exe" "myproj.sqlproj" "/t:SqlSchemaCompare" "/m" "/nodeReuse:False" "/v:q" "/p:RestorePackages=False" "/p:SqlScmpFilePath=C:\xxx\xxx.scmp" "/p:source=C:\xxx\Output\xxx.dacpac" /p:"target=Data Source=xxx,1433;Initial Catalog=xxx;User Id=xxx;Password=xxx;Integrated Security=False;Persist Security Info=True;Connect Timeout=30;Encrypt=True;MultipleActiveResultSets=True" "/p:Deploy=true" "/bl:C:\xxx\Temp\tmpxxxx.tmp.binlog"

Related information

  • Operating system: Win10
  • Branch: stable
  • Indications of severity: no workaround!
  • Version of FAKE: 5.X (dotnet tool install fake-cli -g)
@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

From a command line escaping perspective there should be no difference between using the quotes after or before :

@softlion

This comment has been minimized.

Copy link
Author

commented Sep 27, 2018

It seems the problem is caused by the comma ",1433"

@softlion

This comment has been minimized.

Copy link
Author

commented Sep 27, 2018

If i write this command in terminal, and the comma ',1433' is removed, it does not work but it does not complain. If the comma is there, it complains. If the /p: is outside the quotes, it works fine.

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

maybe comma is a special syntax to start another argument in msbuild and there need to be additional quotes around the property value, no idea

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

By the way I think one workaround is to use environment variables instead of cli arguments. I think even dotnet cli is doing that for some stuff as there are dome edge cases impossible to escape properly

@softlion

This comment has been minimized.

Copy link
Author

commented Sep 27, 2018

Dunno how to do that with env vars.

@softlion

This comment has been minimized.

Copy link
Author

commented Sep 27, 2018

The workaround is working fine

    let msBuildParams, argsString = MSBuild.buildArgs(setParams)
    let args = Process.toParam project + " " + argsString.Replace("\"/p:","/p:\"")
    
    let callMsBuildExe args =
        let result =
            Process.execWithResult (fun info ->
            { info with
                FileName = msBuildParams.ToolPath
                Arguments = args }
            |> Process.setEnvironment msBuildParams.Environment) TimeSpan.MaxValue
        if not result.OK then
            failwithf "msbuild failed with exitcode '%d'" result.ExitCode
        String.Join("\n", result.Messages)

    let handleAfterRun command exitCode project =
        let msgs =
                []
        if exitCode <> 0 then
            let errors =
                msgs
                |> List.choose (fun m -> if m.IsError then Some m.Message else None)
            let errorMessage = sprintf "'%s %s' failed with exitcode %d." command project exitCode
            raise (MSBuildException(errorMessage, errors))

    //let binlogPath, args = addBinaryLogger msBuildParams.ToolPath callMsBuildExe args msBuildParams.DisableInternalBinLog
    Trace.tracefn "Building project: %s\n  %s %s" project msBuildParams.ToolPath args
    let exitCode =
        Process.execSimple (fun info ->
        { info with
            FileName = msBuildParams.ToolPath
            Arguments = args }
        |> Process.setEnvironment msBuildParams.Environment) TimeSpan.MaxValue
    handleAfterRun "msbuild" exitCode project
    __.MarkSuccess()

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

maybe @vbfox has an idea what's going on there?

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

@softlion Are you sure your workaround is doing what you want?

I just tried to reproduce this with:

<Project>
  <Target Name="Test">
    <Message Importance="high" Text="Prop: $(MyProp)" />  
  </Target>
</Project>
>msbuild test.proj /t:Test /p:MyProp=asdas
Prop: asdas
>msbuild test.proj /t:Test /p:"target=Data Source=xxx,1433;Initial Catalog=xxx;User Id=xxx;Password=xxx;
Integrated Security=False;Persist Security Info=True;Connect Timeout=30;Encrypt=True;MultipleActiveResultSets=True"
Prop:

And the other escaping indeed leads to the error you did report.

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

It seems with comma you can assign multiple properties with a single /p:

<Project>
  <Target Name="Test">
    <Message Importance="high" Text="$Prop: $(MyProp)" />
    <Message Importance="high" Text="$Other: $(Other)" />
  </Target>
</Project>
>msbuild test.proj /t:Test /p:MyProp=asdas,Other=asd
$Prop: asdas
$Other: asd
@softlion

This comment has been minimized.

Copy link
Author

commented Sep 27, 2018

Yes the msbuild command is running fine when /p: is outside the double quotes. If it is inside, even if i remove the comma of ,1433 it does not work. Either with a message, or with no message at all (which is strange). I copy/pasted the command manually to verify while creating the workaround.

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

Ok I think I figured it out: microsoft/msbuild#2999

The correct thing to to is to use:
msbuild test.proj /t:Test "/p:MyProp=\"Data Source=xxx,1433;Initial Catalog=xxx;User Id=xxx;Password=xxx;Integrated Security=False;Persist Security Info=True;Connect Timeout=30;Encrypt=True;MultipleActiveResultSets=True\""

(ie set the "value" of the properties in quotes as well) However, we need to figure out how msbuild expectes escaping there

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

I think we will just add msbuild escaping here https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2017 (including comma) from my quick tests that seems to work the best

@matthid matthid closed this in e7e9c10 Sep 27, 2018

@matthid

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2018

@softlion Should be fixed in the next release, thanks for reporting

This was referenced Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.