Address F# Assembly Resolution Issues #893
Conversation
Previously, F# was compiling to a dynamic assembly, which was causing assembly resolution issues. Now, F# compiles to a static assembly, then loads the bytes from disk, emulating the way the C# compile works. In order to make this work, the mangled assembly name had to be changed to create a name with no illegal path characters in it.
|
Hi @sylvanc, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
|
Thanks @fabiocav :) |
|
No problem! Thanks for sending this! On the way to the office. Will address any issues/questions (including some information @dsyme has requested) as soon as I get there. |
| otherFlags.Add("--out:" + Path.ChangeExtension(Path.GetTempFileName(), "dll")); | ||
| var asmName = FunctionAssemblyLoader.GetAssemblyNameFromMetadata(functionMetadata, compilation.AssemblyName); | ||
| var dllName = Path.GetTempPath() + asmName + ".dll"; | ||
| var pdbName = Path.ChangeExtension(dllName, "pdb"); |
There was a problem hiding this comment.
We should remove these files from disk in the finally clause.
There was a problem hiding this comment.
Do we truly need to persist those files to disk? I have a feeling you found the issue, which was caused by the assembly name not matching what was expected when looking up the FunctionAssemblyLoadContext. If that hasn't been done, it would be worth testing compiling to dynamic assembly, but with the expected assembly name, using the GetAssemblyNameFromMetadata result above (assuming you can specify the assembly name when compiling to dynamic). With C#, we don't currently persist the assemblies to disk.
If that works, we should be able to undo the changes we had to make to the FunctionAssemblyLoader
There was a problem hiding this comment.
@fabiocav We would need a new version of FSharp.Compiler.Service for that - one that writes to memory streams. There's a prototype of that around and it's not too hard but it's work.
FCS also checks the in/out file names and assembly names for illegal characters (which seemed reasonable until now - I'm surprised .NET and roslyn don't check these conditions too - but I guess it's legit to use funky characters in assembly names), so we'd need to lift that restriction too in this scenario. There are a couple of other updates to FCS we'd make, such as the list of assembly references implied by F# scripts.
Doing that would take a bit of time so for now it seems easiest to clear the bugs here and do that separately?
|
DLL and PDB are now being correctly deleted, and the test cases are merged :) |
|
The appveyor failure is due to: http://windows.php.net/downloads/releases/php-7.0.12-Win32-VC14-x86.zip ...having been replaced with: http://windows.php.net/downloads/releases/php-7.0.13-Win32-VC14-x86.zip I didn't find an |
|
The download link was updated yesterday. I've re-triggered the build. |
|
@sylvanc, were you expecting those new tests to pass? I'd would be happy to spend some time looking at any issues related to resolution in the morning as I can probably save you some time there. With the changes you've made to the compilation service, things should flow pretty well and it wouldn't take me long to track any issues down. |
|
@fabiocav Yes, we were :( Testing locally (as best we can), those references succeed. Any help would be greatly appreciated! |
|
The Twilio.Api tests now work. I've disabled the NugetChartingReferences test for now: it uses a project.json file, and I'm not entirely convinced that's supported in the testing framework. The error is that @fabiocav, do you happen to know if project.json nuget dependencies are supported in the test harness? |
|
This looks like it's ready to go. |
|
@sylvanc I'll take a look at this today. I wouldn't expect you to have issues with the NuGet dependencies in tests, so I'll take a look at what is going on there. |
|
@fabiocav Adding a C# test that uses a very simple project.json would be great (I don't think there is one right now) |
| { | ||
| await TwilioReferenceInvokeSucceedsImpl(isDotNet: true); | ||
| } | ||
|
|
||
| //[Fact] |
| @@ -172,6 +178,24 @@ public FSharpEndToEndTests(TestFixture fixture) : base(fixture) | |||
| Assert.Equal("secondary type value", request.Properties["DependencyOutput"]); | |||
| } | |||
|
|
|||
| //[Fact] | |||
| @@ -2,7 +2,7 @@ | |||
| // This prelude allows scripts to be edited in Visual Studio or another F# editing environment | |||
|
|
|||
| #if !COMPILED | |||
| #I "../../../../../bin/Binaries/WebJobs.Script.Host" | |||
There was a problem hiding this comment.
I think you should just remove all of these !COMPILED blocks from all the sample/test functions. You don't have to block on this if you feel strongly, but I think it adds more confusion/bloat than it is worth. Furthermore, even with your update I don't get any intellisense - I think your paths are still wrong :) My vote is to just remove and keep the functions clean.
There was a problem hiding this comment.
If kept, I believe we're going going too far up and the path should be ../../src/WebJobs.Script.Host/bin/Debug
| @@ -0,0 +1,60 @@ | |||
| //---------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
So this function isn't needed anymore? Tests were commented out above. If you still need some of this, I think you should just put all the references stuff in a single test function, e.g. the one above.
| @@ -19,7 +19,8 @@ public class FunctionAssemblyLoader : IDisposable | |||
| { | |||
| // Prefix that uniquely identifies our assemblies | |||
| // i.e.: "ƒ-<functionname>" | |||
There was a problem hiding this comment.
nit: update (or remove) the comment
|
Hello @fabiocav! I know this is a bit off-topic, but what is the workflow after this change is closed. How many hours/days should I wait in order to see the changes on Azure? I'm asking for sake of curiosity also! |
|
@mariomeyrelles @dsyme we're wrapping up some additional items and should have a beta deployment that would include this fix soon. I'll update this issue once that is available (you can consume the beta runtime with a small app setting change). There will also be some changes on how we track issues/milestones that will enhance visibility over release dates (when the milestone is completed, we'd have a release). But we're still formalizing that process. |
Previously, F# was compiling to a dynamic assembly, which was causing
assembly resolution issues. Now, F# compiles to a static assembly, then
loads the bytes from disk, emulating the way the C# compile works.
In order to make this work, the mangled assembly name had to be changed
to create a name with no illegal path characters in it.