-
Notifications
You must be signed in to change notification settings - Fork 123
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
Embed MSBUILD dependencies in the package #338
Conversation
Looks like you'd need to do these ones too: https://github.com/fsharp/FSharp.Compiler.Service/pull/338/files#diff-d81cb596fee05d911235bc8c0730075bR599 |
OK, now it should all work! |
This is now ready to merge and release - @dsyme - could you please do that? This is coming up in another context - when you have just VS 2015, you do not have the versions of MSBUILD libraries required by the F# Compiler Service (see tpetricek/Dojo-Suave-FsHome#3), so it would be good to have a new release of F.C.S that fixes this :-) |
It would be nice to decouple msbuild entirely and replicate the project options manually, especially as the msbuild reference resolution can take several seconds.
|
Sorry I missed this PR before, but I wonder what effect bundling the Dave, what exactly do you propose? I'm not sure how we can guarantee to
|
Yes, I'm nervous about the impact of deploying all these DLLs too in various scenarios. It's risky. Tomas, how much validation have you done of the solution? For example, is it still possible to crack project files when things like Microsoft.Common.targets is not on the machine? I'd imagine not. But equally we don't really want to include that as part of the FCS package. So it seems there is still some loss of functionality on target machines. If we're trying to avoid outright crashes, then the alternative technique is to make sure that the MSBuild dependencies are only exercised on very specific pathways, e.g. when non-explicit references are used like I'm not sure what the best and safest solution is. At least, I think we should do a bit more work to quantify what will work and what won't. In the meantime I suppose we can give advice to people to manually deploy the MSBuild binaries in situations where the target machine is not a dev machine. |
In the case of the build machines at xamarin we had to get msbuild12 deployed to the build server because of the FCS dependency. I think it just needs to be noted quite clearly. I would advise against packaging like this for what is just a single feature of FCS.
|
I love this quote: "So far, the only thing I really do not like [about F#] is msbuild. How did that barf sandwich ever make it out of alpha?" |
@7sharp9 - there are two features that depend on MSBuild
Feature 1 is the one that Tomas (and transitive users of FSharp.Formatting) are hitting. Further I think this feature causes a hard dependency on the MSBuild binaries even if it is not actually used (i.e. if "simpleresolution" is used) To explain in further detail.... Feature 1 lets Feature 1 is used by F# Interactive and FsiInteractiveSession when processing scripts. In practice very many scripts rely on it, though mostly because people drop off the ".dll" from a reference name. It's also used by invocations to the F# compiler that do not pass "--simpleresolution", though those are much rarer and less important since any compilation coming from Xamarin Studio or Visual Studio does full resolution in the IDE based on project file settings and passes "--simpleresolution". In the implementation Feature 1 relies on MSBuild's capabilities to do reference resolution (in contrast --simpleresolution does resolution "manually" by looking through search paths). Feature 1 is not even enabled on Mono systems where "simple resolution" is used all the time. However this is regarded as an important discrepancy that needs to be resolved), see also this One path would be to just disable Feature 1 completely for the nuget-shipped FSharp.Compiler.Service - i.e. require that simple resolution be used. However this would give discrepancies in script reference resolution on Windows, and wouldn't place us on a path to good convergence. Alternatively we could seek to completely re-implement the relevant part of the reference resolution code (perhaps with an option to use MSBuild reference, inducing a runtime dependency on the DLLs, just in case there are discrepancies - I can see that being attractive to the Visual F# Tools, for example, when they move to use FCS). Feature 2 uses MSBuild's ability to crack and process project files. My understanding is that feature 2 is just not going to work easily without an appropriate install of MSBuild - processing project files naturally requires all the relevant .targets to be installed. I think it's reasonable to expect MSBuild to be installed in those scenarios (though we should document exactly which version needs to be installed and why in a design note similar to the one on FSharp.Core) As a first step, it would be good to remove the hard dependency on MSBuild for reference resolution in the case where Finally, I should add that I find the way FSharp.Formatting is driving FSharp.Compiler.Service a bit hard to unpick - the various "generate.fsx" files I've seen seem to be doing some reference resolution themselves, and in a somewhat adhoc way, and may be relying on the "non-simple" reference resolution feature (Feature 1 above) much more than any other client. For "generate.fsx" documentation generation I think I'd prefer the reference resolution to be done using the more standard "crack a project file" or "crack a script" rather than the adhoc "throw some text for some references at FCS and cross your fingers" approach that seems to be commonplace right now. I'm not sure about other clients of FCS besides these "generate.fsx" scripts though, it would be good to get a grip on how widespread the use of Feature 1 is. |
Feature 1 needs the older version of msbuild, feature 2 needs the newer one, version 12 I believe which results in the new references which are now needed. |
@7sharp9 - can you adjust your comment above to give the exact version numbers (DLL version numbers) you mean when you say "older" and "newer" versions of MSBuild. Also, I think this thread is about allowing basic compilation, checking and script processing functionality of FCS to be used without any version of MSBuild installed (i.e. if Feature 1 and Feature 2 are not used at all) |
Ill find out when I get a minute about the versions. The point I was making was feature 2 added new dependencies on a newer msbuild, which required installing a seperate package on the build server. |
I do not understand all the implications here. It might not be such a big issue, but I got back to this because of the issue raised here: tpetricek/Dojo-Suave-FsHome#3. Is there another solution to that issue? (If no, it means that F# Compiler Service only works when you have VS 2013 installed, which sounds wrong.) |
@tpetricek msbuild is installed separately from the .Net framework, so if you were referencing one more recent than whatever was shipped with vs2013 then you might expect to have to install it. |
If FCS does not work without a newer msbuild then that might mean that Xamarin Studio now has a dependency on a later msbuild being installed too. |
@dsyme thanks for the thorough explanation. I was only vaguely aware of "Feature 1" as I'm focused on "Feature 2" which is an important facility for FSharp.AutoComplete. I think this issue might well be traced back to the inclusion of Feature 2 (8249ee6)
Before VS2012, I think MSBuild was bundled with .NET, so most machines can be expected to have the A later change (306e478) added
I am a bit unsure of the semantics of this tag. Does it mean no later version, whereas that is normally allowed? In the issue linked by Tomas, the user had VS2015 installed, which surely comes with some more advanced version of MSBuild, so perhaps this tag prevented use of the VS2015 MSBuild assemblies? If we want to bundle, which might be a good idea, I'm not sure yet, then I can test that bundling .NET MSBuild works on Linux/OSX. I do think that MSBuild should be provided as a NuGet package. There actually is a recently created package at https://www.nuget.org/packages/MSBuild/, but I'm not sure if the maintainer is connected with the project. @rojepp I agree, but I guess we are where we are. |
No one likes msbuild but unfortunately until someone re-writes what it provides for us we have to use it. Luckily its open source so we can see the source. AFAIK the latest mono has msbuild14 Interestingly Roslyn has a hard dependency on msbuild14 too. |
@tpetricek Probably the simplest interim solution is for you to push a nuget package ("FSharp.Compiler.Service.Dependencies"?) containing the MSBuild binaries (and an .fsx script to load/reference them). Then add that as a dependency to those projects/samples using FSharp.Compiler.Service delivered to non dev machines. |
@rneatherway Yes, I think you're right. We need a list of what versions of the MSBuild SDK binaries are made available by which versions of Mono/VS/WindowsSDK. It seems: MSBuild 4.0.0.0 - Widely available as bundled with .NET 4.x But what about Mono? cheers |
This is the current situation:
I'm not sure if MSBuild 14 is the MS open-source release. It would be great if so. Another possibility is to add a binding redirect to FCS from MSBuild 12 up to 14. Is it possible to do that for a library, if not we could recommend it for applications that use FCS, as with your note on FSharp.Core. That note is incredibly useful btw. |
@rneatherway Thanks for the info. (Do you know which Linux package has the MSBuild DLLs? I presume it's not in the "minimal" mono runtime installation, but is in "mono-devel"?) The binding redirects (or a runtime assembly binding using Assembly Load Event?) are an interesting possibility |
@rneatherway Could you also show the gacutil -l for Microsoft.Build.Framework please? (I assume is that on Linux?) I notice MSBuild.exe.config on Windows has the following binding redirects (and so does devenv.exe.config), so it certainly is valid to use binding redirects for these assemblies.
|
@dsyme it is really split up amongst a bunch of packages.
It's pretty strange, the full breakdown is:
So MSBuild versions:
|
Also:
|
The
The one under I think that understanding and addressing this issue may allow us to resolve #342 as well. |
I changed the PR so that it builds a separate package ( Doing this is still not optimal, because I'm not sure what you'd do if you wanted to use, say F# Formatting (which has FCS dependency) but with FCS.Standalone. But it at least gives us a starting point that we can use for the web scenarios. Having FCS.Lightweight without MSBUILD dependencies would be even cooler though. We could certainly change F# Formatting to use that instead. |
I think we should probably go ahead and merge this. As it adds a new package it seems fairly harmless, and it has cropped up elsewhere as well. The more people have VS2015 installed and not VS2013 the worse it will get. |
Wouldn't that just lead to errors with missing *.targets files? |
Hmm, do you think the |
I'd prefer not to merge as is, at least not yet. Adding another nuget package to FCS which includes MSBuild binaries makes the FCS project responsible for distributing a bunch of MSBuild binaries in a non-standard configuration. This seems like the wrong direction without really extensive testing of the functionality this does/doesn't enable (see these two features - as I understand it the PR doesn't really enable either feature) . From past experience, if we add an extra nuget package it will be years before we remove it. That said, we certainly have a real problem here and we need to document it and enable a workaround. @tpetricek - do you think you can do as I suggested here and create a separate package (in a separate repo) that provides the MSBuild binaries? Then submit a guide like this one which explains what the MSBuild dependencies are and how to use the interim package to avoid having a specific version of MSBuild on the target machine? @rneatherway - another option is that we reflection-bind to MSBuild - either V4.0 or v12.0, so we have no static dependencies to MSBuild at all. If no MSBuild is bindable then feature 1 "reference resolution" falls back to simple resolution, and feature 2 "Project Cracking" fails with an informative exception. We could test both scenarios via fault injection. Thanks, |
Theres still underlying issues with project cracking too, the whole operation needs to be done via a separate process to support redirecting msbuild tasks to the right version. |
Added backticks around the git commands in README.md to format them properly
Closing as we now have #470 |
This is work-in-progress PR for #337.
This correctly copies the binaries to the
bin/net45
folder, but for some reason, it does not do the same for the build inbin/net40
.@dsyme @7sharp9 Do you have any idea why the change would not affect the
net40
build? Am I missing anotherfsproj
file somewhere :-)?