Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Address F# Assembly Resolution Issues #893

Closed
wants to merge 16 commits into from
Closed

Address F# Assembly Resolution Issues #893

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
159e37c
add twilio test
dsyme Nov 8, 2016
0d20f6b
function renaming
dsyme Nov 8, 2016
5fbc7ff
add nuget references test
dsyme Nov 8, 2016
a0b687e
Address F# Assembly Resolution Issues
sylvanc Nov 8, 2016
2a7090f
delete temporary dll and pdb after compilation
sylvanc Nov 9, 2016
f5e81d2
Merge branch 'dsyme-888' into fsharp-assembly-resolve
sylvanc Nov 9, 2016
31e7b3c
Merge branch 'dsyme-891' into fsharp-assembly-resolve
sylvanc Nov 9, 2016
d22c965
Update run.csx
dsyme Nov 11, 2016
f5aeaf4
Update function.json
dsyme Nov 11, 2016
6e6a926
Update run.csx
dsyme Nov 11, 2016
ad90d51
add new tests to csproj
sylvanc Nov 11, 2016
b1f21cb
put twilio reference back in run.csx
sylvanc Nov 11, 2016
785a86c
fix file references
sylvanc Nov 11, 2016
40053e2
add function.json for new tests
sylvanc Nov 11, 2016
05093fa
add project.json to NugetChartingReferences
sylvanc Nov 11, 2016
4395106
disabled NugetChartingReferences
sylvanc Nov 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 40 additions & 11 deletions src/WebJobs.Script/Description/DotNet/FSharp/FSharpCompilationService.cs
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
using System.IO;
using System.Reflection;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Scripting;
Expand All @@ -25,11 +26,13 @@ internal class FSharpCompiler : ICompilationService
= new Lazy<InteractiveAssemblyLoader>(() => new InteractiveAssemblyLoader(), LazyThreadSafetyMode.ExecutionAndPublication);

private readonly OptimizationLevel _optimizationLevel;
private readonly Regex _hashRRegex;

public FSharpCompiler(IFunctionMetadataResolver metadataResolver, OptimizationLevel optimizationLevel)
{
_metadataResolver = metadataResolver;
_optimizationLevel = optimizationLevel;
_hashRRegex = new Regex(@"^\s*#r\s+", RegexOptions.Compiled | RegexOptions.IgnoreCase);
}

public string Language
Expand All @@ -50,8 +53,28 @@ public IEnumerable<string> SupportedFileTypes

public ICompilation GetFunctionCompilation(FunctionMetadata functionMetadata)
{
// First use the C# compiler to resolve references, to get consistenct with the C# Azure Functions programming model
Script<object> script = CodeAnalysis.CSharp.Scripting.CSharpScript.Create("using System;", options: _metadataResolver.CreateScriptOptions(), assemblyLoader: AssemblyLoader.Value);
// First use the C# compiler to resolve references, to get consistency with the C# Azure Functions programming model
// Add the #r statements from the .fsx file to the resolver source
string scriptSource = GetFunctionSource(functionMetadata);
var resolverSourceBuilder = new StringBuilder();

using (StringReader sr = new StringReader(scriptSource))
{
string line;

while ((line = sr.ReadLine()) != null)
{
if (_hashRRegex.IsMatch(line))
{
resolverSourceBuilder.AppendLine(line);
}
}
}

resolverSourceBuilder.AppendLine("using System;");
var resolverSource = resolverSourceBuilder.ToString();

Script<object> script = CodeAnalysis.CSharp.Scripting.CSharpScript.Create(resolverSource, options: _metadataResolver.CreateScriptOptions(), assemblyLoader: AssemblyLoader.Value);
Compilation compilation = script.GetCompilation();

var compiler = new SimpleSourceCodeServices(msbuildEnabled: FSharpOption<bool>.Some(false));
Expand All @@ -77,7 +100,6 @@ public ICompilation GetFunctionCompilation(FunctionMetadata functionMetadata)
scriptFileBuilder.AppendLine("# 0 @\"" + functionMetadata.ScriptFile + "\"");

// Add our original script
string scriptSource = GetFunctionSource(functionMetadata);
scriptFileBuilder.AppendLine(scriptSource);

File.WriteAllText(scriptFilePath, scriptFileBuilder.ToString());
Expand Down Expand Up @@ -132,7 +154,10 @@ public ICompilation GetFunctionCompilation(FunctionMetadata functionMetadata)
}

// This output DLL isn't actually written by FSharp.Compiler.Service when CompileToDynamicAssembly is called
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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove these files from disk in the finally clause.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@dsyme dsyme Nov 8, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

otherFlags.Add("--out:" + dllName);

// Get the #load closure
FSharpChecker checker = FSharpChecker.Create(null, null, null, msbuildEnabled: FSharpOption<bool>.Some(false));
Expand All @@ -149,14 +174,18 @@ public ICompilation GetFunctionCompilation(FunctionMetadata functionMetadata)
// Add the (adjusted) script file itself
otherFlags.Add(scriptFilePath);

// Make the output streams (unused)
var outStreams = FSharpOption<Tuple<TextWriter, TextWriter>>.Some(new Tuple<TextWriter, TextWriter>(Console.Out, Console.Error));

// Compile the script to a dynamic assembly
var result = compiler.CompileToDynamicAssembly(otherFlags: otherFlags.ToArray(), execute: outStreams);

// Compile the script to a static assembly
var result = compiler.Compile(otherFlags.ToArray());
errors = result.Item1;
assemblyOption = result.Item3;
var code = result.Item2;

if (code == 0)
{
var assemblyBytes = File.ReadAllBytes(dllName);
var pdbBytes = File.ReadAllBytes(pdbName);
var assembly = Assembly.Load(assemblyBytes, pdbBytes);
assemblyOption = FSharpOption<Assembly>.Some(assembly);
}
}
finally
{
Expand Down
42 changes: 27 additions & 15 deletions src/WebJobs.Script/Description/DotNet/FunctionAssemblyLoader.cs
View file
Open in desktop
Expand Up @@ -19,7 +19,8 @@ public class FunctionAssemblyLoader : IDisposable
{
// Prefix that uniquely identifies our assemblies
// i.e.: "ƒ-<functionname>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: update (or remove) the comment

public const string AssemblyPrefix = "\u0192-";
public const string AssemblyPrefix = "f-";
public const string AssemblySeparator = "__";

private readonly ConcurrentDictionary<string, FunctionAssemblyLoadContext> _functionContexts = new ConcurrentDictionary<string, FunctionAssemblyLoadContext>();
private readonly Regex _functionNameFromAssemblyRegex;
Expand All @@ -29,7 +30,7 @@ public FunctionAssemblyLoader(string rootScriptPath)
{
_rootScriptUri = new Uri(rootScriptPath, UriKind.RelativeOrAbsolute);
AppDomain.CurrentDomain.AssemblyResolve += ResolveAssembly;
_functionNameFromAssemblyRegex = new Regex(string.Format(CultureInfo.InvariantCulture, "^{0}(?<name>.*?)#", AssemblyPrefix), RegexOptions.Compiled);
_functionNameFromAssemblyRegex = new Regex(string.Format(CultureInfo.InvariantCulture, "^{0}(?<name>.*?){1}", AssemblyPrefix, AssemblySeparator), RegexOptions.Compiled);
}

public void Dispose()
Expand All @@ -51,22 +52,33 @@ internal Assembly ResolveAssembly(object sender, ResolveEventArgs args)
FunctionAssemblyLoadContext context = GetFunctionContext(args.RequestingAssembly);
Assembly result = null;

if (context != null)
try
{
result = context.ResolveAssembly(args.Name);
}
if (context != null)
{
result = context.ResolveAssembly(args.Name);
}

// If we were unable to resolve the assembly, apply the current App Domain policy and attempt to load it.
// This allows us to correctly handle retargetable assemblies, redirects, etc.
if (result == null)
// If we were unable to resolve the assembly, apply the current App Domain policy and attempt to load it.
// This allows us to correctly handle retargetable assemblies, redirects, etc.
if (result == null)
{
string assemblyName = ((AppDomain)sender).ApplyPolicy(args.Name);

// If after applying the current policy, we now have a different target assembly name, attempt to load that
// assembly
if (string.Compare(assemblyName, args.Name) != 0)
{
result = Assembly.Load(assemblyName);
}
}
}
catch (Exception e)
{
string assemblyName = ((AppDomain)sender).ApplyPolicy(args.Name);

// If after applying the current policy, we now have a different target assembly name, attempt to load that
// assembly
if (string.Compare(assemblyName, args.Name) != 0)
if (context != null)
{
result = Assembly.Load(assemblyName);
context.TraceWriter.Warning(string.Format(CultureInfo.InvariantCulture,
"Exception during runtime resolution of assembly '{0}': '{1}'", args.Name, e.ToString()));
}
}

Expand Down Expand Up @@ -165,7 +177,7 @@ private FunctionAssemblyLoadContext GetFunctionContext(string functionName)

public static string GetAssemblyNameFromMetadata(FunctionMetadata metadata, string suffix)
{
return AssemblyPrefix + metadata.Name + "#" + suffix;
return AssemblyPrefix + metadata.Name + AssemblySeparator + suffix.GetHashCode().ToString();
}

public string GetFunctionNameFromAssembly(Assembly assembly)
Expand Down