Assembly Catalog #2025
Assembly Catalog #2025
Conversation
| private static void OnAssemblyLoad(object sender, AssemblyLoadEventArgs args) | ||
| { | ||
| AssemblyProcessor.ProcessAssembly(args.LoadedAssembly); | ||
| AssemblyProcessor.ProcessAssemblies(catalog.GetAssemblies()); |
There was a problem hiding this comment.
Is there a nice way that we can move this work out of the constructor and into another method?
|
I wonder whether we should be emulating MVC Core with its ApplicationParts it uses for stuff such as finding controllers or TagHelpers Without too much familiarity with their implementation, I believe both systems are trying to do the same thing, so I'd lean to trust that the MVC is very familiar with how the new tooling works (which is the reason we are proactively changing this in preparation to our port, but not really testing it yet in that environment). Maybe it doesn't make sense, but it's worth a small research at least. |
|
Ok, updated with most of the feedback. @jdom, I'm been looking the the AspNet Core parts and tagHelpers as you suggested... I still think that it will fall in the same case as MEF... A bunch of code for small win. I still thinking that now we removed all that bunch of code for scanning and we are close to what people asked which is provide a list of assemblies either by code or XML or, by replacing some sort of catalog implementation (we will add DI support to it later on). |
|
This may be relevant: https://github.com/khellang/Scrutor This is a nice way of scanning assemblies and registering them with |
|
|
||
| public AssemblyCatalog() | ||
| { | ||
| logger = LogManager.GetLogger("AssemblyCatalog", LoggerType.Provider); |
|
"Project file does not exist." Hmm... |
|
I think it is .net CLI not installed... |
|
|
||
| namespace Orleans.Runtime | ||
| { | ||
| public class AssemblyCatalog : IAssemblyCatalog |
There was a problem hiding this comment.
I wonder if it were useful to separate the handling logic from the traversal logic in creating something like
public static IEnumerable<T> AsDepthFirst<T>(this IEnumerable<T> elements, Func<T, IEnumerable<T>> descendantSelector)
{
var stack = new Stack<T>(elements);
while(stack.Any())
{
var next = stack.Pop();
yield return next;
foreach(var descendant in descendantSelector(next))
{
stack.Push(descendant);
}
}
}
}
Then it would be possible to do something like rootAssemblies.AsDepthFirst(assembly => assembly.GetReferencedAssemblies().Where(i => !blacklist.Contains(i.Name));. This likely varies here, taken from the Gitter discussion. This wouldn't certainly cause stack overflow either.
There was a problem hiding this comment.
@veikkoeeva can you put a PR to this branch? Appreciate that. Thanks
There was a problem hiding this comment.
@galvesribeiro How long can this branch wait? :-P One the first though this modification would seem sensible, but would require one to suss out the details with APIs. I'm glad if you consider this and find a way, or someone else, but as of myself, well, you'd need to wait a week or two at minimum. Though if this makes sense, I, you, or someone else, could do a PR later.
There was a problem hiding this comment.
Sure. The way it is today may not be the 10th wonder now but it does what is suppose to do. So I think we can enhance it later. What matter is the interface and its usage. The catalog implementation itself can change anytime.
There was a problem hiding this comment.
There might be changes to the catalog interface too, but I'd be surprised if they couldn't be backawards compatible if needed.
|
@dotnet-bot test this please |
|
Any chance this is on track to go in? This would likely help clean up a lot of the .NET Core stuff and i'd still like to help :) |
|
Yes, I'm actually really close to pushing some branches related to core to the main repo (see my repo if interested). I found a way to not migrate to dotnet cli for now (and hence we can sill use our msbuild tasks, at least until the new tooling RTMs). I might start reviewing this on Monday, but feel free to review it yourself and accelerate this. Also, if you have experience with asm scanning in aspnet, then your input to whether it applies here too would be useful. |
|
Cool. I'll wait for a pull request or something as hunting around for the code doesn't feel like I'm finding the right thing. I have two .NET Core projects in prod (one using containers!). One uses global assembly scanning with |
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static Assembly MatchWithLoadedAssembly(AssemblyName searchFor, AppDomain appDomain) | ||
| { | ||
| return | ||
| return |
There was a problem hiding this comment.
is this function still being used ? I think we are dropping using AppDomain to scan assemblies.
Just a little confused.
There was a problem hiding this comment.
@xiazen I wonder does that mean we support both as per #2145. There was a bit of discussion if the AppDomain could be shimmed ("polyfilled") at https://gitter.im/dotnet/orleans?at=57b6079287f779f0690a9fca – also some NETStandard code on assembly loading (that likely doesn't work correctly).
There was a problem hiding this comment.
@xiazen You are correct. This thing shouldn't be used anymore. I'll get back to it later on. Thanks
There was a problem hiding this comment.
@veikkoeeva I think this PR is about removing dependency on AppDomain to move to support .Net Core, which is part of #2145 . That's why I get confused.
About shimmed AppDomain, it might accelate our process on going to .Net Core. But it might also confuse people with AppDomain in .Net. I'm slightly leaning in the direction to not create that shimmed AppDomainclass. :)
There was a problem hiding this comment.
hehe, funny that this was mentioned, but yes, until Assembly loading gets fixed, I'm introducing a AppDomain shim for now, otherwise there is no way I can get help fixing all the porting holes we currently have, and would have to spend a lot of time fixing every build break in the correct manner myself until I can get help with it. A PR is coming really soon.
There was a problem hiding this comment.
haha. well I'm fine with it being there temporarily, just for an intermediate step for us to go to coreclr.
|
We are using bits and pieces of this PR as part of the 2.0 work, but won't merge it as a whole. So I'm closing it to avoid confusion. |
Initial implementation of
IAssemblyCatalog.This is part of the move to support .Net Core on #368 by removing the the dependency on AppDomains.
The default catalog implementation (
AssemblyCatalog) requires the developer to set the assemblies list by XML<Assemblies>or code config. Later on, this implementation can be replaced by using DI.Some changes were required on codegen in order to support it. So now, there is no scan for assemblies on a specific folder anymore. You have to inform the assembly list that needs to be processed for runtime codegen.