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

Potential fix for assembly resolving issues #1502

Closed
wants to merge 2 commits into from
Closed

Potential fix for assembly resolving issues #1502

wants to merge 2 commits into from

Conversation

FrankvdStam
Copy link
Contributor

Potential fix for #1501

Please run the tests for the core project - I was unable to get them running so am unsure if they still pass.

Commit message:
Copied BaseAssemblyResolver from Mono.Cecil into CustomAssemblyResolver.cs, removed conditional compilation. First it tries to resolve the .NET core way, if that doesn't work it proceeds with the .NET framework way.

EmbeddedResourcesGenerator.cs now passes the added CustomAssemblyResolver to LoadModule call to override the default resolving behavior. This object is re-created for each call, maybe it can be instantiated once? Not sure what the lifecycle should be.

…er.cs, removed conditional compilation. First it tries to resolve the .NET core way, if that doesn't work it proceeds with the .NET framework way.

EmbeddedResourcesGenerator.cs now passes the added CustomAssemblyResolver to LoadModule call to override the default resolving behavior. This object is re-created for each call, maybe it can be instantiated once? Not sure what the lifecycle should be.
@rouke-broersma
Copy link
Member

rouke-broersma commented Apr 16, 2021

Hi @FrankvdStam 👋

I've been thinking about this fix for some time and I am a bit hesitant to merge this.

  1. I don't like the idea of being responsible for this piece of code, since I have very little understanding of what it does
  2. Because I have very little understanding of what it does, I cannot judge the impact this might have on projects where embedded resource resolving currently works perfectly fine
  3. This fix seems like it might be relevant but I'm not sure and there hasn't been any communication on it: Dynamicaly determine whether to use the .NET Framework-specific assembly resolver on .NET Standard. jbevain/cecil#701

For that reason I'm going to attempt to get in contact with the Mono.Cecil team first and ask them to judge if this is a good idea. Hope you don't mind, I really appreciate the effort!

@FrankvdStam
Copy link
Contributor Author

Understandable - it is a bit dubious.

Looked at the fix you included, does not look like it would be relevant here - it still uses conditional compilation. See, Ryder is .NET core. This causes only .NET core specific resolving code to be used in the cecil default resolver. The proposed fix would solve this for .NET standard - but only at compile - building Ryder as .NET standard might work, but will in the end probably have the same issues when resolving framework libraries.

As far as I can see you need dynamic, runtime checking on what path to take: core/standard/framework. Or you're stuck in your branch: Core applications will likely have problems resolving framework libraries and framework apps will likely have issues resolving core libraries. It does not look like that PR includes a fix like that.

Maybe someone from cecil will end up reading this and help them along or maybe I don't really understand it enough either and am wasting everyone's time, these are just my findings.

@rouke-broersma
Copy link
Member

Maintainers of Mono.Cecil seem to be quite active with replying to questions. If they end up not answering soon enough we will probably still go with this fix you propose. I am just looking for ways to avoid having to maintain this particular piece of magic if at all possible :p

@FrankvdStam
Copy link
Contributor Author

Maybe I should propose this fix over at cecil? I just figured it would take far too long.

@rouke-broersma
Copy link
Member

Response to pull requests at Cecil seem kinda hit or miss. Not sure why some PR's get merged within the same day and some don't even get a comment. The questions seem to get more response. I agree that it would probably take a long time before a change like this would be accepted by Cecil if at all. Let's see what they reply to my questions and go from there.

@rouke-broersma
Copy link
Member

@FrankvdStam The maintainer of Cecil has replied that the default assembly resolver in Cecil is for the generic case and that any project is free to implement their own resolver to their own requirements. It seems like this PR is the way to go.

I would like to take some time to clean test, understand (ish) and clean up the custom resolver. There are things marked 'internal for testing' for example that we're not using and can probably safely be removed.

@FrankvdStam
Copy link
Contributor Author

@rouke-broersma okay, my time is limited (priorities yadayada) but let me know what you need from me.

@rouke-broersma
Copy link
Member

Integrated in #1564

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants