Fixing a few issues that were primarily impacting F# #1049

fabiocav · 2016-12-14T01:57:55Z

1 - Ensuring that package resolver does not return duplicate references
2 - Ensuring that the MetadataResolver handles duplicates when adding default references
3 - Fixing F# compilation service to get the script references and removing dependency on C# compilation
4 - Fixing issue preventing the load context from being initialized for F#, which would trigger dependency resolution failures.

Resolves #1044
Resolves #1027
Resolves #985
Mitigates #178 (this addresses the most common scenarios)

fabiocav · 2016-12-14T03:03:14Z

src/WebJobs.Script/Description/DotNet/CSharp/CSharpCompilationService.cs


        public CSharpCompilationService(IFunctionMetadataResolver metadataResolver, OptimizationLevel optimizationLevel)
        {
            _metadataResolver = metadataResolver;
            _optimizationLevel = optimizationLevel;
        }

-        public string Language


Just some cleanup and small optimizations here since I was working in this area.

fabiocav · 2016-12-14T03:06:19Z

src/WebJobs.Script/Description/DotNet/DotNetFunctionInvoker.cs

@@ -258,15 +258,16 @@ private object[] ProcessInputParameters(object[] parameters)
                using (_metricsLogger.LatencyEvent(eventName))
                {
                    ICompilation compilation = _compilationService.GetFunctionCompilation(Metadata);
+
+                    Assembly assembly = compilation.EmitAndLoad(cancellationToken);


With this change, we now emit before validation. This does remove an optimization in the case of C# where we can perform code analysis for the small check we run below, but it's not significant in this code path (this is not triggered as part of diagnostics compilations) and the complexity to handle both scenarios (where emit is required and when it isn't) is probably not justified at this point.

fabiocav · 2016-12-14T03:08:19Z

src/WebJobs.Script/Description/DotNet/FSharp/FSharpCompilationService.cs

-                // However F# has its own view on default options. For scripts these should include the
-                // following framework facade references.
-
-                otherFlags.Add("-r:System.Linq.dll"); // System.Linq.Expressions.Expression<T> 


@sylvanc were these really necessary? After switching so to get the script references, these references were causing issues and compilation, execution and tests succeed without them. We did add System.Runtime to the default references as we have been planning to do that for a while.

Btw, I did try using types that (as defined here) would depend on those references and didn't seem to run into issues. I think it would probably be a good idea to have some tests covering those for validation.

Those don't appear to be necessary, no. Good catch. As far as I can tell, git claims those were introduced before the work @dsyme and I did, which means I should have noticed them, sorry about that!

NP! Glad to have confirmation that those are not needed.

fabiocav · 2016-12-14T03:09:34Z

src/WebJobs.Script/Description/DotNet/FSharp/FSharpCompilationService.cs

@@ -77,7 +69,6 @@ public ICompilation GetFunctionCompilation(FunctionMetadata functionMetadata)
            var resolverSource = resolverSourceBuilder.ToString();

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


This dependency has been removed. We get all the references from the script and run them directly against the metadata resolver. This call was not cheap, so I'm happy to get rid of it.

fabiocav · 2016-12-14T03:10:32Z

src/WebJobs.Script/Description/DotNet/FSharp/FSharpCompilationService.cs

@@ -196,13 +172,17 @@ public ICompilation GetFunctionCompilation(FunctionMetadata functionMetadata)
                    var assembly = Assembly.Load(assemblyBytes, pdbBytes);
                    assemblyOption = FSharpOption<Assembly>.Some(assembly);
                }
+                else
+                {
+                    _traceWriter.Verbose($"F# Compilation failed with arguments: { string.Join(" ", otherFlags) }");


In case of failures, we now log the arguments, which will help us identify issues like the duplicate references we ran into.

nit: lower case Compilation to match new trace below. Also, should this be logged at Error level? Our default log level is Info I thought - does this even make it out? Or is the error already logged elsewhere and this is just verbose details?

The actual compilation/diagnostics errors are already logged and reported back. This gives us some additional details in verbose mode to show the entire set of arguments passed to the compiler.

fabiocav · 2016-12-15T23:03:57Z

@sylvanc could take a quick look at this changes (particularly on the comment I tagged you on)?

sylvanc · 2016-12-16T09:31:28Z

src/WebJobs.Script/Description/DotNet/DotNetCompilationServiceFactory.cs

@@ -60,7 +61,7 @@ public ICompilationService CreateService(ScriptType scriptType, IFunctionMetadat
                case ScriptType.CSharp:
                    return new CSharpCompilationService(metadataResolver, OptimizationLevel);
                case ScriptType.FSharp:
-                    return new FSharpCompiler(metadataResolver, OptimizationLevel);
+                    return new FSharpCompiler(metadataResolver, OptimizationLevel, _traceWriter);


sylvanc · 2016-12-16T09:54:30Z

This looks great, thanks so much @fabiocav . Can't wait to try this out live :)

brettsam

I'm not an expert here, but it mostly looks good. The only thing that jumps out at me is the lack of tests that would have caught this. Or tests that prove the tracing happens as you expect now.

mathewc · 2016-12-16T16:39:35Z

I agree with Brett that for the number of bugs being resolved with these changes, I would expect us to have some new regression tests.

fabiocav · 2016-12-16T19:43:35Z

@brettsam and @mathewc agree with the comments on the tests. Will add some tests to make sure the scenarios we're fixing are covered.

1 - Ensuring that package resolver does not return duplicate references 2 - Ensuring that the MetadataResolver handles duplicates when adding default references 3 - Fixing F# compilation service to get the script references and removing dependency on C# compilation 4 - Fixing issue preventing the load context from being initialized for F#, which would trigger dependency resolution failures.

dnfclas added the cla-already-signed label Dec 14, 2016

fabiocav added the in progress label Dec 14, 2016

fabiocav force-pushed the fsharpfixes branch 3 times, most recently from 90eb340 to 3121f34 Compare December 14, 2016 02:48

fabiocav commented Dec 14, 2016

View reviewed changes

fabiocav force-pushed the fsharpfixes branch from 3121f34 to 5255116 Compare December 14, 2016 03:17

fabiocav requested review from mathewc and brettsam December 15, 2016 23:03

sylvanc reviewed Dec 16, 2016

View reviewed changes

brettsam reviewed Dec 16, 2016

View reviewed changes

mathewc approved these changes Dec 16, 2016

View reviewed changes

fabiocav force-pushed the fsharpfixes branch from 818d4c4 to 0080db7 Compare December 16, 2016 22:25

fabiocav merged commit 0080db7 into Azure:dev Dec 16, 2016

fabiocav removed the in progress label Dec 16, 2016

fabiocav deleted the fsharpfixes branch December 16, 2016 22:26

fabiocav mentioned this pull request Dec 17, 2016

Fix dependency resolution for PCL assemblies #178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing a few issues that were primarily impacting F# #1049

Fixing a few issues that were primarily impacting F# #1049

fabiocav commented Dec 14, 2016 •

edited

Loading

fabiocav Dec 14, 2016

fabiocav Dec 14, 2016

fabiocav Dec 14, 2016

fabiocav Dec 14, 2016

sylvanc Dec 16, 2016

fabiocav Dec 16, 2016

fabiocav Dec 14, 2016

sylvanc Dec 16, 2016

fabiocav Dec 14, 2016

mathewc Dec 16, 2016 •

edited

Loading

fabiocav Dec 16, 2016

fabiocav commented Dec 15, 2016

sylvanc Dec 16, 2016

sylvanc commented Dec 16, 2016

brettsam left a comment

mathewc commented Dec 16, 2016

fabiocav commented Dec 16, 2016

Fixing a few issues that were primarily impacting F# #1049

Fixing a few issues that were primarily impacting F# #1049

Conversation

fabiocav commented Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiocav commented Dec 15, 2016

Choose a reason for hiding this comment

sylvanc commented Dec 16, 2016

brettsam left a comment

Choose a reason for hiding this comment

mathewc commented Dec 16, 2016

fabiocav commented Dec 16, 2016

fabiocav commented Dec 14, 2016 •

edited

Loading

mathewc Dec 16, 2016 •

edited

Loading