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

Filter framework-mismatched assemblies from NUGET_PLUGIN_PATHS #2859

Closed
wants to merge 1 commit into from

Conversation

zjrunner
Copy link

@zjrunner zjrunner commented May 22, 2019

Prior to this, if you configured both a netcore and a netfx plugin to handle both nuget and dotnet,
both would fail. One would fail on the dll, the other on the exe.
nuget_plugin_paths=d:\tools\credprovider\plugins\netcore\CredentialProvider.Microsoft\CredentialProvider.Microsoft.dll;d:\tools\credprovider\plugins\netfx\CredentialProvider.Microsoft\CredentialProvider.Microsoft.exe

This change does a naive filter based on existing discovery models for convention-based plugin loading.

Bug

Fixes: NuGet/Home#8151
Regression: No

Fix

Use the same logic as the convention based plugin discovery to filter the paths in NUGET_PLUGIN_PATHS. Do not fallback to the baseline discovery if all paths are filtered out to hold a line on typo discovery. This does exclude self-contained dotnet exes from plugin_paths, but since they weren't possible in discovery anyway this feels like an ok compromise.

Testing/Validation

Tests Added: Yes
Validation:
verified failure of nuget 5.0.0 as in the linked issue, verified failing unit test as created, fixed the code, verified passing unit tests and passing commandline of built-nuget.exe

Prior to this, if you configured both a netcore and a netfx plugin to handle both nuget and dotnet,
both would fail.  One would fail on the dll, the other on the exe.
nuget_plugin_paths=d:\tools\credprovider\plugins\netcore\CredentialProvider.Microsoft\CredentialProvider.Microsoft.dll;d:\tools\credprovider\plugins\netfx\CredentialProvider.Microsoft\CredentialProvider.Microsoft.exe

This change does a naive filter based on existing discovery models for convention-based plugin loading.
@dnfclas
Copy link

dnfclas commented May 22, 2019

CLA assistant check
All CLA requirements met.

@nkolev92
Copy link
Member

I just now noticed this. :)
Thanks for creating the PR @zjrunner

@nkolev92
Copy link
Member

This does exclude self-contained dotnet exes from plugin_paths, but since they weren't possible in discovery anyway this feels like an ok compromise.

Not sure what you mean by this?

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

I'm ok with this change.
We need to document it.

If you define a plugin for .NET Framework, you need to define one for .NET Core as well.
The environment variables are a special scenario.

We are also kind of changing the behavior where we require plugins to be exe or dll, but I don't imagine that's a big deal, as the convention based discovery already does that.

Again, we can document that.

@nkolev92
Copy link
Member

nkolev92 commented Jun 4, 2019

@rrelyea @anangaur @dtivel
Can you please review this behavior change?

@nkolev92
Copy link
Member

@rrelyea @anangaur @dtivel

Can you please take a look at this so we can merge it to 5.2?

@zivkan @heng-liu @dominoFire @donnie-msft

Any feedback is appreciated.

@rrelyea
Copy link
Contributor

rrelyea commented Jun 26, 2019

dotnet3 sdk now will create an exe by default for netcoreapp3 projects. (tested with p5 on my machine)
as such, i'm concerned about the reliance on the file extension.

@nkolev92
Copy link
Member

Another option that we discsussed would be multiple environment variables. One of netfx and netcore each.

Those variables would take preference over the old one which would apply to both.

@nkolev92
Copy link
Member

nkolev92 commented Aug 6, 2019

Sorry for the delay @zjrunner
Due to the concerns with the extension approach, we will go for a 2 env var approach.
Any issues with that from your end?

I will have a write-up and a PR later today.

@nkolev92
Copy link
Member

nkolev92 commented Aug 6, 2019

Closing in favor of #2986.

@nkolev92 nkolev92 closed this Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants