Load NuGet assemblies #115
Conversation
Changes the check for previously loaded assemblies to only fail if we already registered them via MSBuildLocator. Then adds previously loaded assemblies to the list of loaded assemblies so they aren't loaded again.
Don't throw an error if the assembly is already loaded.
src/MSBuildLocator/MSBuildLocator.cs
Outdated
| return assembly; | ||
| } | ||
|
|
||
| // Finds and loads NuGet assemblies if msbuildPath is in a VS installation |
There was a problem hiding this comment.
should this be wrapped in an #if NETFRAMEWORK or something so we don't waste the time on Core?
src/MSBuildLocator/MSBuildLocator.cs
Outdated
| } | ||
|
|
||
| // Finds and loads NuGet assemblies if msbuildPath is in a VS installation | ||
| targetAssembly = Path.GetFullPath(Path.Combine(msbuildPath, "..", "..", "..", "Common7", "IDE", "CommonExtensions", "Microsoft", "NuGet", assemblyName.Name + ".dll")); |
There was a problem hiding this comment.
Instead of hardcoding this relative path, should we add an argument
public static void RegisterMSBuildPath(string msbuildPath, string[] additionalDirectories)And use that in RegisterInstance?
src/MSBuildLocator/MSBuildLocator.cs
Outdated
| foreach (string path in msbuildSearchPaths.Where(path => !Directory.Exists(path))) | ||
| { | ||
| throw new ArgumentException($"Directory \"{msbuildPath}\" does not exist", nameof(msbuildPath)); | ||
| throw new ArgumentException($"Directory \"{path}\" does not exist", nameof(msbuildSearchPaths)); |
There was a problem hiding this comment.
Throwing inside a foreach is a bit fishy. Collect 'em all into a single exception?
| if (string.IsNullOrWhiteSpace(msbuildPath)) | ||
| RegisterMSBuildPath(new string[] { | ||
| msbuildPath | ||
| #if NET46 |
There was a problem hiding this comment.
I think this needs an if-not-mono, right?
There was a problem hiding this comment.
I agree, but it's worth noting that without it, it would just do an extra check without actually failing.
There was a problem hiding this comment.
Also occurred to me that we don't support mono quite yet, but that's changing soon.
src/MSBuildLocator/MSBuildLocator.cs
Outdated
| if (msbuildSearchPaths.Any(string.IsNullOrWhiteSpace)) | ||
| { | ||
| throw new ArgumentException("Value may not be null or whitespace", nameof(msbuildPath)); | ||
| throw new ArgumentException("Value may not be null or whitespace", nameof(msbuildSearchPaths)); |
There was a problem hiding this comment.
Include index of bogus entry?
There was a problem hiding this comment.
0-indexed or 1-indexed? I made it 1-indexed but would be happy to change it.
| string targetAssembly = Path.Combine(msbuildPath, assemblyName.Name + ".dll"); | ||
| if (File.Exists(targetAssembly)) | ||
| { | ||
| assembly = Assembly.LoadFrom(targetAssembly); |
There was a problem hiding this comment.
Not unique to this PR but should we improve error handling here? Like if LoadFrom fails because it exists but is a native DLL or something? Shouldn't be a major problem since the naming we're searching is very restricted.
There was a problem hiding this comment.
I haven't checked to see what (or how bad) the error message is for an invalid dll, but it would seem unnecessarily confusing to put out two assemblies with identical names and extensions in (at least roughly) the same spot.
There was a problem hiding this comment.
Yeah, let's just leave it.
| string targetAssembly = Path.Combine(msbuildPath, assemblyName.Name + ".dll"); | ||
| if (File.Exists(targetAssembly)) | ||
| { | ||
| assembly = Assembly.LoadFrom(targetAssembly); |
There was a problem hiding this comment.
Yeah, let's just leave it.
src/MSBuildLocator/MSBuildLocator.cs
Outdated
| if (string.IsNullOrWhiteSpace(msbuildPath)) | ||
| RegisterMSBuildPath(new string[] { | ||
| msbuildPath | ||
| #if NET46 && !MONO |
There was a problem hiding this comment.
Because mono runs "unmodified" .NET Framework binaries, it doesn't usually get a compile-time constant (and can't easily have a specific binary in a NuGet package). I think this check would need to happen at runtime and use IsRunningOnMono introduced in #82. Since that isn't landed yet, let's just drop it here and add a note to that PR to merge this and add the check?
Adds an overload to permit supplying additional paths in which to search for MSBuild assemblies beyond the default. Uses this new overload to add the search path for NuGet assemblies for when in a VS install. The latter fixes microsoft#86.
2 participants
This is based off of #107. Review that first.
That PR means we won't error if you load an assembly that we also know how to load. This PR teaches MSBuildLocator how to find NuGet assemblies in VS scenarios.
Fixes #86.