Add fix for #343 Use ResolveReferences task

[7sharp9]

This task seems to stably return the reference list now.
Added a test using an executable that references a pcl assembly

[rneatherway]

We should enable all the PCL tests on linux now that that is supported.

[7sharp9]

Do you want to do that as a separate PR or?

[rneatherway]

I think it's best here so we can see the tests passing across the board.

[rneatherway]

Otherwise your new test isn't exercised on Unix.

[7sharp9]

Seems some tests are failing, not sure if the correct tasks are in place for all situations, might have to drop this completly if I cant get it working tomorrow.

[dsyme]

Yes, two failing on Travis

9191) Test Failure : FSharp.Compiler.Service.Tests.ProjectOptionsTests.Project file parsing -- 2nd level references
2920     Expected: "Found 'FSharp.Core.dll'"
2921Actual: "Failed to find 'FSharp.Core.dll'"


29312) Test Failure : FSharp.Compiler.Service.Tests.ProjectOptionsTests.Project file parsing -- Exe with a PCL reference
2932     Expected: set
2933  ["FSharp.Core.dll"; "SQLite.Net.Platform.Generic.dll";
2934   "SQLite.Net.Platform.Win32.dll"; "SQLite.Net.dll";
2935   "System.Collections.Concurrent.dll"; "System.Collections.dll";
2936   "System.ComponentModel.Annotations.dll";
2937   "System.ComponentModel.EventBasedAsync.dll"; "System.ComponentModel.dll"; ...]
2938Actual: set
2939  ["FSharp.Core.dll"; "System.Core.dll"; "System.Numerics.dll"; "System.dll";
2940   "mscorlib.dll"]

[7sharp9]

Ha switching to ResolveReferences fixes the referenced pcl issue but breaks all other use cases, it seems that the evaluated item ReferencePath is now empty but the evaluated item Reference exists although the references are not path resolved just fully qualified names.

[rneatherway]

Can you explain a bit more? Almost all the tests pass so things don't seem too bad! Your new test project should probably be under data with the other non-canonical tests, and I think it will have some difficulty passing because it references assemblies from the non-existent packages directory (I think SQLite.Net comes from NuGet). Perhaps they can just be removed?

[rneatherway]

I do see that things are worse on Windows though.

[rneatherway]

Should have posted this here:

2nd level references test fails on Linux. Output includes:

InvalidCastException, ITask: Microsoft.Build.Framework.ITask, Microsoft.Build.Framework, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a Task type: Microsoft.Build.Tasks.Message, Microsoft.Build.Tasks.v4.0, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
Error initializing task Message: Cannot cast from source type to destination type.
Error initializing task Message: System.InvalidCastException: Cannot cast from source type to destination type.
  at Microsoft.Build.BuildEngine.BuildTask.InitializeTask () [0x00000] in <filename unknown>:0 

Classic old friend

[rneatherway]

I've pushed what I tried here, which seems decent, although your new test doesn't pass. As I said, the SQLite dlls are not there so it isn't that surprising.

[7sharp9]

Having pcl working is the most important think for me, it's unshippable without that. :-(

On 2 Jun 2015, at 12:17, Robin Neatherway [email protected] wrote:

I've pushed what I tried here, which seems decent, although your new test doesn't pass. As I said, the SQLite dlls are not there so it isn't that surprising.

—
Reply to this email directly or view it on GitHub.

[rneatherway]

Have you tried it with the SQLite dlls present? Perhaps it works fine. All the other PCL tests work OK.

[7sharp9]

Yes Ive tried, we are no further forward its still exactly the same issue, no implicit pcl references are present.

[7sharp9]

@rneatherway I just corrected the package folder, although it makes no difference even if you restore the package.

[7sharp9]

@rneatherway The only way I can figure at the moment is evaluating the named evaluation group _DesignTimeFacadeAssemblies and if it exists return those assemblies too.

[rneatherway]

Hmm. I don't understand that because we are running ImplicitlyExpandDesignTimeFacades. What is different when compared to this project and all the others where the PCL works? What is the diff between the log outputs?

[7sharp9]

Heres a gist of the 259 vs sqllite test:
https://gist.github.com/7sharp9/9f1466ae40b86919a8f1

[7sharp9]

It makes no difference, mine has the package restored

[7sharp9]

With the sqllite one there is an extra property _DesignTimeFacadeAssemblies which is not present in the 259 test.

[7sharp9]

The only reference I can find is here:
https://github.com/Microsoft/msbuild/blob/82177a50da735cc0443ac10fa490d69368403d71/src/XMakeTasks/Microsoft.NETFramework.CurrentVersion.targets#L99-L102

[rneatherway]

Hey did you notice this at the end of the sqlite one:

Target named 'ImplicitlyExpandTargetFramework' not found in the project.

[7sharp9]

Yep, that target doesn't exist as its a property of the pcl reference rather than the project under test

[rneatherway]

What do you mean? That target is found in Microsoft.Common.targets. If I build your sqlite project with xbuild then it runs that target.

[7sharp9]

Im guessing its not looking there

[rneatherway]

Ah, my mistake. I assumed that as it was a Task it was in the Tasks dll. You're right, the binding redirect should be fine there.

I tried to construct a test for problem (2), but the WebSharper issue no longer occurs and I don't have access to Android projects. @7sharp9 how could we add a test for that?

I'm strongly leaning towards bundling a version of MSBuild with FCS as being the correct resolution for this issue, but it would be great to ask somebody in the know.

As for the windows issue, with massive thanks again to MS for the recent open-sourcing spree, I have found a suitable logger to subsitute for my BasicStringLogger (https://github.com/Microsoft/msbuild/blob/master/src/XMakeBuildEngine/Logging/SerialConsoleLogger.cs) so I should be able to get some more debug output for that tonight.

[7sharp9]

@rneatherway I dont know, Android projects will require the android msbuild tasks to be present on the cI server:

  <Import Project="$(MSBuildExtensionsPath)\Xamarin\Android\Xamarin.Android.FSharp.targets" />

[dsyme]

You could make a new task DLL, e.g. by copying and modifying this stuff? Or this DLL or this DLL are MSBuild task DLLs with a 4.0 dependency? I presume trying to load that as a custom task will fail?

[rneatherway]

Should be OK to just modify the paths of the targets file and custom task assembly then put them in the project directory. If you do that then we probably don't want to commit the assemblies to this repo but we could at least test whether it is working on an ad-hoc basis. I'm not aware of any other custom tasks that exhibit this behaviour now that Websharper works. If you know of any we could use that instead.

[7sharp9]

The other thing you could do to harden the testing is to invoke type checking on the project and assert that there are no Errors present.

[rneatherway]

Well I tried using a ConsoleLogger, but unfortunately for some reason it doesn't give and more information really, just:

Target "ResolveAssemblyReferences" in file "C:\Program Files (x86)\MSBuild\12.0\bin\amd64\Microsoft.Common.CurrentVersion.targets":
  Using "ResolveAssemblyReference" task from assembly "Microsoft.Build.Tasks.v12.0, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a".
  Task "ResolveAssemblyReference"
  Done executing task "ResolveAssemblyReference".
Done building target "ResolveAssemblyReferences" in project "sqlite-net-spike.fsproj".

But not what and how it happened. By contrast, running msbuild.exe from the command line gives much information:

Target "ResolveAssemblyReferences: (TargetId:12)" in file "C:\Program Files (x86)\MSBuild\12.0\bin\amd64\Microsoft.Common.CurrentVersion.targets" from project "D:\dev\FSharp.Compiler.Service\tests\service\data\sqlite-net-spike\sqlite-net-spike.fsproj" (target "ResolveReferences" depends on it):
Using "ResolveAssemblyReference" task from assembly "Microsoft.Build.Tasks.v12.0, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a".
Task "ResolveAssemblyReference" (TaskId:7)
  Task Parameter:
      Assemblies=
          mscorlib
          FSharp.Core
          System
          System.Numerics
          SQLite.Net.Platform.Win32
                  HintPath=..\..\..\..\packages\SQLite.Net-PCL.3.0.5\lib\net4\SQLite.Net.Platform.Win32.dll
          SQLite.Net
                  HintPath=..\..\..\..\packages\SQLite.Net-PCL.3.0.5\lib\net40\SQLite.Net.dll
          SQLite.Net.Platform.Generic
...
<snip>

but I don't know the name of this logger to use it programmatically.

[rneatherway]

OK... apparently there is a property called ResolveAssemblyReferencesSilent which disables logging for that task specifically. I'll have to try that, but it will be after the weekend.

<ResolveAssemblyReferencesSilent 
  Condition="'$(ResolveAssemblyReferencesSilent)' == '' and
  '$(TraceDesignTime)' != 'true' and
  '$(BuildingProject)' == 'false'">
    true
</ResolveAssemblyReferencesSilent>
<ResolveAssemblyReferencesSilent 
  Condition="'$(ResolveAssemblyReferencesSilent)' == ''">
    false
</ResolveAssemblyReferencesSilent>

[7sharp9]

It would be good to get this one resolved so that project cracking is usable :-(

[rneatherway]

I couldn't get it working without using the Build target. Of course this then actually builds things, which is slow and therefore bad. So I ripped off the sneaky HostCompilation trick from visualfsharp's salsa.fs, which skips the compilation step and keeps things fast. A console app with a proof of concept is at https://github.com/rneatherway/msbuildresolution, and is much easier to test than waiting 10 minutes for the FCS unit tests.

[rneatherway]

This seems to work locally. I had to relax the condition on Dave's new PCL test because Windows uses a slightly different set of assemblies, but I think it still suffices to check that the PCL reference assemblies are being pulled in.

[dsyme]

@rneatherway sounds like progress!

[rneatherway]

It is my belief that this is ready to be merged now. I don't think it fixes #342 (android), but it would be nice to have confirmation from @7sharp9. I've come to also believe that this should be fixed by using an assembly binding redirect (and bundling MSBuild a la #338).

[7sharp9]

A binding redirect would have to be applied to the originator process, unless this was invoked oop

[rneatherway]

Yes I understand. I think this is consistent with xbuild, msbuild and Visual Studio.

[7sharp9]

Ok, I was going over this issue with one of the guys here and he was of the opinion that this function should be done out of process otherwise unknown external dlls could be loaded into the monodevelop process. We have a similar method in MonoDevelop which is running targets for MonoDevelop, all out of process for these reasons.

[rneatherway]

Ah right, because of the custom tasks etc? That makes sense. Would that mean having an exe wrapper that just printed the results on standard out? Or is there a cleverer way of doing that in .NET?

Either way, I would be glad to merge this and do a release as it at least fixes the PCL referencing issue and generally makes things more robust. @dsyme are you OK with that?

[7sharp9]

@rneatherway Did you make changes to both new and old project cracking?

[rneatherway]

Yes, the old API now uses the ResolveReferences with the BuildingInsideVisualStudio property set. It's actually easier with the old API than the new.

[7sharp9]

@rneatherway MonoDevelop does it like this:
https://github.com/mono/monodevelop/blob/master/main/src/core/MonoDevelop.Core/MonoDevelop.Projects.Formats.MSBuild/MSBuildProjectService.cs#L504-L608

And theres a reference resolution task already in existence too, I might have linked you to this a week back maybe.
https://github.com/mono/monodevelop/blob/master/main/src/core/MonoDevelop.Core/MonoDevelop.Projects.Formats.MSBuild/RemoteProjectBuilder.cs#L188-L216

[rneatherway]

I see. I think they should be using ResolveReferences there with the property set like I did. They can probably avoid adding the PCL references afterwards manually that way.

I notice this comment:

//always start the remote process explicitly, even if it's using the current runtime and fx
//else it won't pick up the assembly redirects from the builder exe

is pretty telling. I suppose we could always use that method, but I'm pretty happy with where we are now.

[7sharp9]

Having unknown references polluting the current runtime process is quite nasty too so its better to be an isolationist here I think.

[dsyme]

@7sharp9 - it seems it would be best to implement the isolation via an appdomain or an external process. Can you do that as part of XS/MD addin implementation or do you think we should do it in FCS?

@rneatherway - yes we can merge and re-release.

[7sharp9]

@dsyme Well I would expect to be able to call an exposed method without having to wrap it in another process, it probably should be part of FCS.

Ive been told calling msbuild has to be done out of process, so I wont be able to use this functionality as it stands. :-(

[dsyme]

@7sharp9 - well, you can use it, you just have to call it out of process :)

I agree it would be good to do that automatically though, I suppose by including a fsservice.exe in the "tools" directory that can be invoked behind the scenes

[7sharp9]

At the moment I don't have time to refactor to use out of process though, Ill have to do that before this release goes out, I suppose Android is broken until then too.