search

dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.89k stars 782 forks source link

DotnetFscCompilerPath property is wrapped into double quotes #16375

Open auduchinok opened 10 months ago

auduchinok commented 10 months ago

I've been investigating some of the MSBuild properties and found that DotnetFscCompilerPath is wrapped into double quotes, and it's the only property to do so in a console app project. There are a lot of other properties with paths to various targets, props, SDK, outputs, and so on, but none of the other properties are wrapped into double quotes, at least on my test project. Should we make DotnetFscCompilerPath consistent with the properties defined by the rest of the dotnet/MSBuild toolset?

cc @KevinRansom Do you by any chance remember if there was a specific reason to do it this way?

KevinRansom commented 10 months ago

@auduchinok it can be a user supplied value. And we can't guarantee that they have added quotes on paths with a space. It contains the path to an .exe or .dll that is to be executed and historically the "Program Files" has been used as a vector for a class of security bugs. Although I suppose they probably wouldn't be classed that way now, since in reality one needs permission to write to the c:\ directory.

We could do it a bit differently and add the quotes at the point we generate the command line in the build task if there is an issue?

So let's go with it's a bug.

Hmm wierd, I don't remember typing: So let's go with it's a bug. but it looks like something I would type, and the message hasn't been edited. It's too early in the day for me to be doing "Thinking (tm)".

auduchinok commented 10 months ago

@KevinRansom I've looked into it more, and there indeed are some other properties that get wrapped. On F# projects this was the only one that I've seen, though. We've added a workaround for the issue we'd had, so it's not critical for us. If there's a chance that changing this could break some things and/or would add excessive churn, then it's probably fine to leave it as is.