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

Execute net tools plugins #6113

Conversation

Nigusu-Allehu
Copy link
Contributor

@Nigusu-Allehu Nigusu-Allehu commented Oct 22, 2024

Bug

Fixes: NuGet/Home#13741

Description

This PR makes sure plugins discovered in #5990 are executed. The plugin files are directly executed by starting a new process.

Unit test

The test case I wrote has a simple batch file with the following content

@echo off
echo File executed > ""{outputPath}""

It is passed as a plugin file to the PluginFactory. The test looks into the contents of outputPath to check if the plugin has been executed or not. The assert contain statement checks if outputPath contains File executed which should be written only if the plugin file has been executed.

nuget-plugin-bat.bat

If you would like to manually test the plugin I used for testing, here it is as a file. : https://github.com/Nigusu-Allehu/NuGetBatAuthPlugin/blob/main/nuget-plugin-bat.bat (Edit the file and change the value of debugLogPath if you would like to see what the plugin has been receiving and sending)
set env variables as follows

  • NUGET_NETFX_PLUGIN_PATHS : null
  • NUGET_NETCORE_PLUGIN_PATHS : null
  • NUGET_PLUGIN_PATHS : path to the nuget-plugin-bat.bat file
    Add a break point in PluginManager.cs at the line var utilities = await PerformOneTimePluginInitializationAsync(plugin, cancellationToken); When the execution stops at this break point take a look into the values of the plugin variable. It should have a value plugin.Connection.State = Success

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.
    Document .net tools Plugin feature Home#13858

@Nigusu-Allehu Nigusu-Allehu self-assigned this Oct 22, 2024
@Nigusu-Allehu Nigusu-Allehu marked this pull request as ready for review October 24, 2024 19:48
@Nigusu-Allehu Nigusu-Allehu requested a review from a team as a code owner October 24, 2024 19:48
@jeffkl jeffkl requested a review from kartheekp-ms October 29, 2024 22:09
Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Just nits for now. I'll review more later today

Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Just nits!

@Nigusu-Allehu Nigusu-Allehu requested a review from zivkan November 4, 2024 23:51
Copy link
Contributor

@jgonz120 jgonz120 left a comment

Choose a reason for hiding this comment

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

Looks good to me, I think a new name for IsRunnablePluginFile would be good though.

src/NuGet.Core/NuGet.Protocol/Plugins/PluginFile.cs Outdated Show resolved Hide resolved
@@ -525,7 +525,6 @@ public void GetPluginsInNuGetPluginPaths_NuGetPluginPathsPointsToAFileThatDoesNo

// Assert
Assert.Single(plugins);
Assert.False(plugins[0].IsDotnetToolsPlugin);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the value return now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The property has been renamed to RequiresDotnetHost and it returns environment based value. It would be false if it is desktop and true otherwise.

@@ -240,7 +245,7 @@ internal List<PluginFile> GetPluginsInPath()
}
else
{
pluginFiles.Add(new PluginFile(path, new Lazy<PluginFileState>(() => PluginFileState.InvalidFilePath)));
pluginFiles.Add(new PluginFile(path, new Lazy<PluginFileState>(() => PluginFileState.InvalidFilePath), isRunnablePluginFile: false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test to validate this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt we don’t need a test specifically for this scenario, as it would end up testing the behavior of PathValidator.IsValidLocalPath and PathValidator.IsValidUncPath within the if statement. Since the else case only triggers if both methods return false, testing this would indirectly validate PathValidator’s internal logic, which I felt isn’t necessary for this context.

@Nigusu-Allehu Nigusu-Allehu merged commit d883a09 into dev-feature-dot-net-tool-plugin-support Nov 6, 2024
15 of 22 checks passed
@Nigusu-Allehu Nigusu-Allehu deleted the dev-nyenework-execute-net-tools-plugins branch November 6, 2024 20:28
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.

6 participants