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.

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

Replacing the auto scanning of assemblies. #144

Open
PureKrome opened this issue Jun 8, 2014 · 13 comments
Open

Replacing the auto scanning of assemblies. #144

PureKrome opened this issue Jun 8, 2014 · 13 comments

Comments

@PureKrome
Copy link
Member

So we've both agreed to remove the auto scanning of assemblies, etc.

Great, no probs.

But what will our option be now?

There's two things that the code does (currently).

  1. Scan all assemblies for all IAuthenticationProvider's
  2. Read in any configuration settings-data.

then once we have those two things, we then returned a filtered list of IAuthenticationProvider's for the settings-data given.

for example.

  1. Scan == google/fb/twitter/windows live/linked in/foo
  2. config == google/foo

so then we really only have 2 providers that we 'use'.

So I think we can start by just replacing the code for the 1st part -> scanning assemblies.
This means that the user needs to provide us with some assembly that contains the foo provider, in this scenario.

Thoughts?

An optional list of .. er.. Types? So I can hard code the core providers into the discovered list. and then add any other optional ones that were provided?

I think it would just get really messy if the user has to define some assembly names or namespace crap, etc in the config file.

@PureKrome PureKrome added this to the 2.0 - .NET 4.5 & Async/Await milestone Jun 8, 2014
@phillip-haydon
Copy link
Member

Ok so we should continue to scan the core assembly.

If the extra assembly exists we can scan that.

Then we need in the web.config section the ability to specify a fully qualified name for a type. Which we will add.

Then a way to specify it in app settings. And also via the code.

Like how provider keys are done now

@PureKrome
Copy link
Member Author

Ok so we should continue to scan the core assembly.

Um - isn't the whole idea to remove scanning and reflection? I have it right now just manually hard coded adding the 4x core providers Types into a list.

If the extra assembly exists we can scan that.

Again - scanning 😢

Am i missing something?

@fschwiet
Copy link

I haven't been able to find the discussion of motivation for not scanning assemblies. Since I don't know the context, I wonder how much it resembles my situation where a dll is refusing to load and breaking everything else (MonoDevelop.NUnit GetTypes() is failing when I self-host within a NUnit test). I suspect you've considered taking a list of assemblies to load, I see this has been considered: https://github.com/SimpleAuthentication/SimpleAuthentication/pull/130/files. Have you considered the option might be a list of assembly names to skip, or an option to ignore DLLs that fail to load?

In my case, I'd like to just skip MonoDevelop.NUnit: http://stackoverflow.com/questions/30471563/simpleauthencation-throwing-failed-to-reflect-on-the-current-domains-assemblie

@PureKrome
Copy link
Member Author

@fschwiet the idea/context at the time was that scanning was leading to errors getting thrown. So instead of just having the possibility of some errors occurring ... and the cause of the errors (scanning) won't be very obvious to the developer.

So - to avoid this .. we decided on: let the developer specify the dll's that need to be loaded. If an error occurs, they know exactly what they manually added.

Also, the idea that dev's will drop in dll's a lot of the time is low, so we assumed it will be less pain for the developer to manually specify the dll instead of them dropping it in .. 💩 happens and they now need to spend some time debugging the situation and crying.

@fschwiet
Copy link

If the number of failing DLLs is lower, then it might be easier to create an exclude-list (I don't know whats typical, but it seemed interesting).

The failure exception would then list the failing DLLs, and give instructions on how to create an exclude-list (in addition to exception info needed to help the DLL load, as appropriate). If the number of people who encounter failing DLLs is low, then having an exclude-list would require no work for most people (again, I have no idea what is typical).

@PureKrome
Copy link
Member Author

The problem here was that scanning was the issue. We have no idea what will blow up. In nancy, ravendb was blowing up nancy (because nancy uses TinyIOC as the baked in default DI/IoC) so we had to add some crap hack to tell nancy to ignore that, when it scanned on startup.

ref: NancyFx/Nancy#1767

As such - we thought: why bother. Keep things simple, etc.

The scanning was a nice thing to have. It was probably going to be used by an entire 12 people in the world. So to make sure random 💩 don't hurt the majority ... we wanted to take scanning out. make it manual. and make those 12 people manually enter in the stuff, in a config file.

Ok. ok.. 12 people is a bit generous. maybe 3 is more accurate. or 2.

@phillip-haydon
Copy link
Member

The purpose of the scanning (besides loading users assemblies also) was mostly so that we could have a core assembly which defined the main providers that cater for the 95%...

Twitter, Facebook, Google, Github

The remaining would be shoved in a second assembly and loaded if the user happens to drop the assembly in the bin.


The resolving should in theory never fail because at the very least it should load itself if it can't load others. However when dependencies are not met it causes errors, majority of the time it was because people didn't include an assembly in their project they actually required.

This lead to them blaming us rather than them not realizing that its not us, it was a dependency they didn't include for some other code.


In your case, your self hosting for unit testing, I gather. Which IMO is wrong, the Nancy.Testing lib allows you to make real calls to your API without the need for a host. Will that solve your problem, probably not based on the exception in the SO question.

It seems to be a problem with how unit tests are run on Xamarin.

I'm purchasing a new mac this weekend which I'm going to be installing Xamarin on.

If you can create a small repo to reproduce the problem I will definitely debug the issue for you, and make any changes to SA to resolve it.

@fschwiet
Copy link

This is for an integration test that spans sites. I'd prefer to use Nancy.Testing but I need to hit the site with a browser.

When I debug FindAllTypesOf<...> the call to Assembly.GetTypes() is failing for one assembly (with name MonoDevelop.NUnit). Creating a repro I feel I can share would require a lot of whittling, is there specific information I could debug for you instead? To me it seems the problem is understood, the uncertainty is around how to move forward.

At this point I think for me the shortest path forward is to launch the server via Process.Start() from the integration tests. I don't think there is a bugfix appropriate to SimpleAuthentication, but if this design change was finished then I would have an easier option.

I do appreciate the replies on these issues and prior work in SimpleAuthentication/NancyFX.

@phillip-haydon
Copy link
Member

Hmmm I couldn't reproduce this error :( (still trying)

@fschwiet
Copy link

Hmm I didn't really want to share the code as its terrible, but it'd be
worse to let you spin your wheels particularly when you're willing to help.

I put the code at: https://github.com/fschwiet/delete-me-later. To
repro, I run the unit tests from Xamarin Studio (the non-MonoDevelop NUnit
DLLs may not repro it). I am running from a Mac.

I'd like to delete the repo when you're done, so let me know. This is
not a blocker for me, but it would be nice to have a fix and maybe others
would benefit.

(the repository is still being pushed at the moment)

On Sat, May 30, 2015 at 9:29 PM, Phillip Haydon notifications@github.com
wrote:

Hmmm I couldn't reproduce this error :( (still trying)


Reply to this email directly or view it on GitHub
#144 (comment)
.

@phillip-haydon
Copy link
Member

I'm super busy today so I'll clone it locally so you can delete it then i'll take another look tomorrow. (currently re-arranging furniture cos got new furniture arriving today)

@phillip-haydon
Copy link
Member

Cloned. You can delete.

@fschwiet
Copy link

thanks

@PureKrome PureKrome removed this from the 2.0 - .NET 4.5 & Async/Await milestone Oct 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants