Skip to content

Conversation

@cartermp
Copy link
Contributor

@cartermp cartermp commented Jun 3, 2019

#6921

This probing code is called only when not in the F# tools for Visual Studio. However...

...it looks for MSBuild v12, which is installed only by an older Visual Studio

...which means it only works on Windows

...and VS 2019 doesn't install MSBuild v12

So any modern, released version of F# on Windows requires you to either hunt down the last Visual Studio that installs this old MSBuild toolset, or hunt down the old toolset yourself. And then hope that the reflection code worked. All in service to some slightly better diagnostics for #load and #r directives in F# script files.

@KevinRansom
Copy link
Contributor

@cartermp have you tested this out on VS 4 MAc and mono?

@cartermp
Copy link
Contributor Author

cartermp commented Jun 3, 2019

No, but it doesn't matter. See @auduchinok's comment here: #6921 (comment)

We've removed a reference to MSBuild12 resolver and moved to using the simulated one about a year ago and haven't seen any problems nor got any complaints so far.

VS for Mac doesn't deploy the MSBuild v12 toolset, and I'm not even sure if it's even possible to load in a non-Windows environment.

@KevinRansom
Copy link
Contributor

@cartermp. check this out: https://github.com/dotnet/fsharp/blob/master/fcs/FSharp.Compiler.Service.MSBuild.v12/FSharp.Compiler.Service.MSBuild.v12.fsproj#L28

Please note that is our master branch.

I promise I will look at this at some time, but priorities are such that removing a bit of "Ostensibly dead code" isn't really that high.

Kevin

@cartermp
Copy link
Contributor Author

cartermp commented Jun 3, 2019

@KevinRansom I don't understand your point at all. This is as-stated in the README:

FSharp.Compiler.Service.MSBuild.v12 adds legacy MSBuild v12 support to an instance of FSharp.Compiler.Service, if exact compatibility for scripting references such as #r "Foo, Version=1.3.4" is required.

But, as I said in the PR and this issue, it is for providing slightly better diagnostics in F# scripts. It is not exactly the most useful thing in the world.

Additionally, I cannot find any evidence that this works anywhere other than Windows machines. And said Windows machines do not have any modern (i.e., since at least VS 2017) environments that deploy the MSBuild v12 toolset. So this reflection code is so likely to fail that it's not worth keeping.

I promise I will look at this at some time, but priorities are such that removing a bit of "Ostensibly dead code" isn't really that high.

Then why have you been merging my other PRs that remove dead code?

@KevinRansom
Copy link
Contributor

@cartermp Dead code removal is a good thing to do, when we know it's dead.

However, I don't think we know this is dead code. We do know that no VS or coreclr scenario requires it, so we are good with removing it. But there are community deployments we know nothing about, for example that fcs project I pointed to, it definitely references msbuild 12. Before we remove this, we should understand it's purpose, and determine whether it is no longer required.

@cartermp
Copy link
Contributor Author

cartermp commented Jun 3, 2019

@KevinRansom again, I don't understand your point. I'm fully aware that FCS references it.

Before we remove this, we should understand it's purpose, and determine whether it is no longer required.

I also understand what it's trying to do. Which is why I'm removing it.

@KevinRansom
Copy link
Contributor

@dsyme, said it can go. And he has a better handle on community code use than I do … so ...

@KevinRansom KevinRansom merged commit 14be767 into dotnet:master Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants