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

Rename Source metadata property, clean up resolution logic (#351) #352

Merged
merged 1 commit into from
May 17, 2016
Merged

Rename Source metadata property, clean up resolution logic (#351) #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/WebJobs.Script/Description/DotNet/CSharp/CSharpCompilationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ private static string GetFunctionSource(FunctionMetadata functionMetadata)
{
string code = null;

if (File.Exists(functionMetadata.Source))
if (File.Exists(functionMetadata.ScriptFile))
{
code = File.ReadAllText(functionMetadata.Source);
code = File.ReadAllText(functionMetadata.ScriptFile);
}

return code ?? string.Empty;
Expand All @@ -69,7 +69,7 @@ private static Compilation GetScriptCompilation(Script<object> script, bool debu
SyntaxTree scriptTree = compilation.SyntaxTrees.FirstOrDefault(t => string.IsNullOrEmpty(t.FilePath));
var debugTree = SyntaxFactory.SyntaxTree(scriptTree.GetRoot(),
encoding: Encoding.UTF8,
path: Path.GetFileName(functionMetadata.Source),
path: Path.GetFileName(functionMetadata.ScriptFile),
options: new CSharpParseOptions(kind: SourceCodeKind.Script));

compilationOptimizationLevel = OptimizationLevel.Debug;
Expand Down
2 changes: 1 addition & 1 deletion src/WebJobs.Script/Description/DotNet/FunctionAssemblyLoadContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public Uri FunctionBaseUri
{
if (_functionBaseUri == null)
{
_functionBaseUri = new Uri(Path.GetDirectoryName(Metadata.Source) + "\\", UriKind.RelativeOrAbsolute);
_functionBaseUri = new Uri(Path.GetDirectoryName(Metadata.ScriptFile) + "\\", UriKind.RelativeOrAbsolute);
}

return _functionBaseUri;
Expand Down
4 changes: 2 additions & 2 deletions src/WebJobs.Script/Description/DotNet/FunctionMetadataResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public ScriptOptions FunctionScriptOptions
.WithMetadataResolver(this)
.WithReferences(GetCompilationReferences())
.WithImports(DefaultNamespaceImports)
.WithSourceResolver(new SourceFileResolver(ImmutableArray<string>.Empty, Path.GetDirectoryName(_functionMetadata.Source)));
.WithSourceResolver(new SourceFileResolver(ImmutableArray<string>.Empty, Path.GetDirectoryName(_functionMetadata.ScriptFile)));
}
}

Expand All @@ -93,7 +93,7 @@ public ScriptOptions FunctionScriptOptions
/// <returns>The path to the function's private assembly folder</returns>
private static string GetBinDirectory(FunctionMetadata metadata)
{
string functionDirectory = Path.GetDirectoryName(metadata.Source);
string functionDirectory = Path.GetDirectoryName(metadata.ScriptFile);
return Path.Combine(Path.GetFullPath(functionDirectory), DotNetConstants.PrivateAssembliesFolderName);
}

Expand Down
2 changes: 1 addition & 1 deletion src/WebJobs.Script/Description/DotNet/PackageAssemblyResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public IEnumerable<string> AssemblyReferences
private static ImmutableArray<PackageReference> InitializeAssemblyRegistry(FunctionMetadata metadata)
{
var builder = ImmutableArray<PackageReference>.Empty.ToBuilder();
string fileName = Path.Combine(Path.GetDirectoryName(metadata.Source), DotNetConstants.ProjectLockFileName);
string fileName = Path.Combine(Path.GetDirectoryName(metadata.ScriptFile), DotNetConstants.ProjectLockFileName);

if (File.Exists(fileName))
{
Expand Down
2 changes: 1 addition & 1 deletion src/WebJobs.Script/Description/DotNet/PackageManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public Task RestorePackagesAsync()

try
{
string functionDirectory = Path.GetDirectoryName(_functionMetadata.Source);
string functionDirectory = Path.GetDirectoryName(_functionMetadata.ScriptFile);
string projectPath = Path.Combine(functionDirectory, DotNetConstants.ProjectFileName);
string nugetHome = GetNugetPackagesPath();

Expand Down
2 changes: 1 addition & 1 deletion src/WebJobs.Script/Description/FunctionDescriptorProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public virtual bool TryCreate(FunctionMetadata functionMetadata, out FunctionDes

BindingMetadata triggerMetadata = functionMetadata.InputBindings.FirstOrDefault(p => p.IsTrigger);

string scriptFilePath = Path.Combine(Config.RootScriptPath, functionMetadata.Source);
string scriptFilePath = Path.Combine(Config.RootScriptPath, functionMetadata.ScriptFile);

IFunctionInvoker invoker = null;

Expand Down
2 changes: 1 addition & 1 deletion src/WebJobs.Script/Description/FunctionInvokerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ protected bool InitializeFileWatcherIfEnabled()
{
if (Host.ScriptConfig.FileWatchingEnabled)
{
string functionDirectory = Path.GetDirectoryName(Metadata.Source);
string functionDirectory = Path.GetDirectoryName(Metadata.ScriptFile);
_fileWatcher = new FileSystemWatcher(functionDirectory, "*.*")
{
IncludeSubdirectories = true,
Expand Down
6 changes: 5 additions & 1 deletion src/WebJobs.Script/Description/FunctionMetadata.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ public FunctionMetadata()

public string Name { get; set; }

public string Source { get; set; }
/// <summary>
/// The primary entry point for the function (to disambiguate if there are multiple
/// scripts in the function directory).
/// </summary>
public string ScriptFile { get; set; }

public ScriptType ScriptType { get; set; }

Expand Down
2 changes: 1 addition & 1 deletion src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ internal NodeFunctionInvoker(ScriptHost host, BindingMetadata trigger, FunctionM
: base(host, functionMetadata)
{
_trigger = trigger;
string scriptFilePath = functionMetadata.Source.Replace('\\', '/');
string scriptFilePath = functionMetadata.ScriptFile.Replace('\\', '/');
_script = string.Format(CultureInfo.InvariantCulture, _functionTemplate, scriptFilePath);
_inputBindings = inputBindings;
_outputBindings = outputBindings;
Expand Down
82 changes: 46 additions & 36 deletions src/WebJobs.Script/Host/ScriptHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,8 +407,8 @@ private Collection<FunctionDescriptor> ReadFunctions(ScriptHostConfiguration con
// TODO: we need to define a json schema document and do
// schema validation and give more informative responses
string json = File.ReadAllText(functionConfigPath);
JObject configMetadata = JObject.Parse(json);
FunctionMetadata metadata = ParseFunctionMetadata(functionName, config.HostConfig.NameResolver, configMetadata);
JObject functionConfig = JObject.Parse(json);
FunctionMetadata metadata = ParseFunctionMetadata(functionName, config.HostConfig.NameResolver, functionConfig);

// determine the primary script
string[] functionFiles = Directory.EnumerateFiles(scriptDir).Where(p => Path.GetFileName(p).ToLowerInvariant() != ScriptConstants.FunctionConfigFileName).ToArray();
Expand All @@ -417,42 +417,18 @@ private Collection<FunctionDescriptor> ReadFunctions(ScriptHostConfiguration con
AddFunctionError(functionName, "No function script files present.");
continue;
}
else if (functionFiles.Length == 1)
string scriptFile = DeterminePrimaryScriptFile(functionConfig, functionFiles);
if (string.IsNullOrEmpty(scriptFile))
{
// if there is only a single file, that file is primary
metadata.Source = functionFiles[0];
}
else
{
// if there is a "run" file, that file is primary
string functionPrimary = null;
functionPrimary = functionFiles.FirstOrDefault(p => Path.GetFileNameWithoutExtension(p).ToLowerInvariant() == "run");
if (string.IsNullOrEmpty(functionPrimary))
{
// for Node, any index.js file is primary
functionPrimary = functionFiles.FirstOrDefault(p => Path.GetFileName(p).ToLowerInvariant() == "index.js");
if (string.IsNullOrEmpty(functionPrimary))
{
// finally, if there is an explicit primary file indicated
// in config, use it
JToken token = configMetadata["source"];
if (token != null)
{
string sourceFileName = (string)token;
functionPrimary = Path.Combine(scriptDir, sourceFileName);
}
}
}

if (string.IsNullOrEmpty(functionPrimary))
{
AddFunctionError(functionName, "Unable to determine primary function script.");
continue;
}
metadata.Source = functionPrimary;
AddFunctionError(functionName,
"Unable to determine the primary function script. Try renaming your entry point script to 'run' (or 'index' in the case of Node), " +
"or alternatively you can specify the name of the entry point script explicitly by adding a 'scriptFile' property to your function metadata.");
continue;
}
metadata.ScriptFile = scriptFile;

metadata.ScriptType = ParseScriptType(metadata.Source);
// determine the script type based on the primary script file extension
metadata.ScriptType = ParseScriptType(metadata.ScriptFile);

metadatas.Add(metadata);
}
Expand All @@ -466,6 +442,40 @@ private Collection<FunctionDescriptor> ReadFunctions(ScriptHostConfiguration con
return ReadFunctions(metadatas, descriptorProviders);
}

/// <summary>
/// Determines which script should be considered the "primary" entry point script.
/// </summary>
internal static string DeterminePrimaryScriptFile(JObject functionConfig, string[] functionFiles)
{
if (functionFiles.Length == 1)
{
// if there is only a single file, that file is primary
return functionFiles[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it would be a breaking change, but looking back, I wish we didn't have this special case at all, as it results in a cliff. I'd rather it fail right away when users only have one file, then mysteriously start failing when you add a second one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that functionFiles contains only the non function.json files, so this case handles the 90% case where there is just a function.json and a single file in the directory, say a DoIt.BAT. We don't want it to fail if there is only a single file.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's debatable. My take is that we cause more harm with the 'second source file cliff' than if you force people to follow a (simple) pattern from the get go.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand what you're proposing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean we'd be better off if we had the same requirements in the 1-source-file case as in the n-source-files case. Of course, changing it now is a breaking chance, but we should still consider it. Simpler rules, less magical, less cliff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's not change this now.

}
else
{
// First see if there is an explicit primary file indicated
// in config. If so use that.
string functionPrimary = null;
string scriptFileName = (string)functionConfig["scriptFile"];
Copy link
Member Author

@mathewc mathewc May 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This property used to be called "source". We're renaming this to "scriptFile". It's an undocumented property at this point, so we'll be safe in making the breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

"scriptFile", not "sourceFile" :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep :)

if (!string.IsNullOrEmpty(scriptFileName))
{
functionPrimary = functionFiles.FirstOrDefault(p =>
string.Compare(Path.GetFileName(p), scriptFileName, StringComparison.OrdinalIgnoreCase) == 0);
}
else
{
// if there is a "run" file, that file is primary,
// for Node, any index.js file is primary
functionPrimary = functionFiles.FirstOrDefault(p =>
Path.GetFileNameWithoutExtension(p).ToLowerInvariant() == "run" ||
Path.GetFileName(p).ToLowerInvariant() == "index.js");
}

return functionPrimary;
}
}

private static ScriptType ParseScriptType(string scriptFilePath)
{
string extension = Path.GetExtension(scriptFilePath).ToLowerInvariant().TrimStart('.');
Expand Down Expand Up @@ -729,7 +739,7 @@ internal static bool TryGetFunctionFromException(Collection<FunctionDescriptor>
// We use the directory name for the script rather than the full script path itself to ensure
// that we handle cases where the error might be coming from some other script (e.g. an NPM
// module) that is part of the function.
string absoluteScriptPath = Path.GetFullPath(currFunction.Metadata.Source).ToLowerInvariant();
string absoluteScriptPath = Path.GetFullPath(currFunction.Metadata.ScriptFile).ToLowerInvariant();
string functionDirectory = Path.GetDirectoryName(absoluteScriptPath);
if (errorStack.Contains(functionDirectory))
{
Expand Down
8 changes: 4 additions & 4 deletions test/WebJobs.Script.Tests/Description/CSharp/FunctionAssemblyLoaderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ public void ResolveAssembly_WithIndirectPrivateDependency_IsResolved()
{
var resolver = new FunctionAssemblyLoader("c:\\");

var metadata1 = new FunctionMetadata { Name = "Test1", Source = @"c:\testroot\test1\test.tst" };
var metadata2 = new FunctionMetadata { Name = "Test2", Source = @"c:\testroot\test2\test.tst" };
var metadata1 = new FunctionMetadata { Name = "Test1", ScriptFile = @"c:\testroot\test1\test.tst" };
var metadata2 = new FunctionMetadata { Name = "Test2", ScriptFile = @"c:\testroot\test2\test.tst" };
var traceWriter = new TestTraceWriter(TraceLevel.Verbose);

var mockResolver = new Mock<IFunctionMetadataResolver>();
Expand All @@ -43,8 +43,8 @@ public void ResolveAssembly_WithIndirectPrivateDependency_LogsIfResolutionFails(
{
var resolver = new FunctionAssemblyLoader("c:\\");

var metadata1 = new FunctionMetadata { Name = "Test1", Source = @"c:\testroot\test1\test.tst" };
var metadata2 = new FunctionMetadata { Name = "Test2", Source = @"c:\testroot\test2\test.tst" };
var metadata1 = new FunctionMetadata { Name = "Test1", ScriptFile = @"c:\testroot\test1\test.tst" };
var metadata2 = new FunctionMetadata { Name = "Test2", ScriptFile = @"c:\testroot\test2\test.tst" };
var traceWriter = new TestTraceWriter(TraceLevel.Verbose);

var mockResolver = new Mock<IFunctionMetadataResolver>();
Expand Down
10 changes: 5 additions & 5 deletions test/WebJobs.Script.Tests/Description/CSharp/PackageAssemblyResolverTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public void GivenLockFile_PackageReferencesAreResolved()
var functionMetadata = new FunctionMetadata()
{
Name = "TestFunction",
Source = _lockFilePath, /*We just need the path from this*/
ScriptFile = _lockFilePath, /*We just need the path from this*/
ScriptType = ScriptType.CSharp
};

Expand All @@ -72,7 +72,7 @@ public void GivenPackagesWithAssemblyReferences_AssemblyReferencesAreResolved()
var functionMetadata = new FunctionMetadata()
{
Name = "TestFunction",
Source = _lockFilePath, /*We just need the path from this*/
ScriptFile = _lockFilePath, /*We just need the path from this*/
ScriptType = ScriptType.CSharp
};

Expand All @@ -89,7 +89,7 @@ public void GivenPackagesWithFrameworkReferences_FrameworkReferencesAreResolved(
var functionMetadata = new FunctionMetadata()
{
Name = "TestFunction",
Source = _lockFilePath, /*We just need the path from this*/
ScriptFile = _lockFilePath, /*We just need the path from this*/
ScriptType = ScriptType.CSharp
};

Expand All @@ -105,7 +105,7 @@ public void TryResolveAssembly_WithReferencedAssemblyName_ResolvesAssemblyPathAn
var functionMetadata = new FunctionMetadata()
{
Name = "TestFunction",
Source = _lockFilePath, /*We just need the path from this*/
ScriptFile = _lockFilePath, /*We just need the path from this*/
ScriptType = ScriptType.CSharp
};

Expand All @@ -124,7 +124,7 @@ public void TryResolveAssembly_WithReferencedFrameworkAssemblyName_ResolvesAssem
var functionMetadata = new FunctionMetadata()
{
Name = "TestFunction",
Source = _lockFilePath, /*We just need the path from this*/
ScriptFile = _lockFilePath, /*We just need the path from this*/
ScriptType = ScriptType.CSharp
};

Expand Down
2 changes: 1 addition & 1 deletion test/WebJobs.Script.Tests/NodeFunctionGenerationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ private static MethodInfo GenerateMethod(BindingMetadata trigger)
string rootPath = Path.Combine(Environment.CurrentDirectory, @"TestScripts\Node");
FunctionMetadata metadata = new FunctionMetadata();
metadata.Name = "Test";
metadata.Source = Path.Combine(rootPath, @"Common\test.js");
metadata.ScriptFile = Path.Combine(rootPath, @"Common\test.js");
metadata.Bindings.Add(trigger);

List<FunctionMetadata> metadatas = new List<FunctionMetadata>();
Expand Down
Loading