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

Fix extension loading for extensions which target multiple runtimes #848

Merged
merged 3 commits into from
Jan 16, 2021

Conversation

ChrisMaddock
Copy link
Member

Fixes #844.

There was an error in the extension selection logic, where an extension of a higher version could replace an extension of a lower version, even if the higher version could not be loaded in the current runtime. This changes brings the check of where an extension is valid in the current runtime forward to prevent that.

The actual change is in the first commit, the second commit is subsequent reformatting and removal of the old check. Unfortunately this whole piece of logic currnently relies on 'real files' and is untested - something I think we'll need to refactor in future.

@ChrisMaddock ChrisMaddock changed the title Issue 844 [WIP] Issue 844 Dec 27, 2020
@ChrisMaddock
Copy link
Member Author

Ah - turns out this was better tested than I thought...although that doesn't stop the check being in the wrong place! This needs a bit more refactoring that I really have time for today. Will come back to it at some point.

@ChrisMaddock ChrisMaddock marked this pull request as draft December 27, 2020 16:41
var candidate = new ExtensionAssembly(filePath, fromWildCard);

if (!CanLoadTargetFramework(Assembly.GetEntryAssembly(), candidate))
return;

Copy link
Member Author

@ChrisMaddock ChrisMaddock Jan 10, 2021

Choose a reason for hiding this comment

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

The addition of lines 364-5 is the only substantial change in this PR.

@ChrisMaddock ChrisMaddock changed the title [WIP] Issue 844 Fix extension loading for extensions which target multiple runtimes Jan 10, 2021
@ChrisMaddock
Copy link
Member Author

Apologies folks, I've done more re-formtting in this PR than is helpful. Have highlighted the only important change with a comment, this is ready for review now, so long as CI passes this time.

@ChrisMaddock ChrisMaddock marked this pull request as ready for review January 10, 2021 17:01
@ChrisMaddock ChrisMaddock merged commit 584f0f7 into master Jan 16, 2021
@ChrisMaddock ChrisMaddock deleted the issue-844 branch January 16, 2021 22:20
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.

.NET Core console runner fails to load extensions when netfx and netstandard versions conflict
1 participant