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

Only hook to solution events when PMC console is opened while executing ps scripts #1122

Merged
merged 1 commit into from Jan 20, 2017

Conversation

jainaashish
Copy link
Contributor

This change is to improve our PMC performance with respect to DPL and improve below scenarios:

  1. In a VS instance, whenever you install first package, it will make sure to execute init scripts for all existing packages. But it will never execute these init scripts with any solution load/unload until you explicitly open NuGet PMC console. Once the console is open, then it will hook to solution events, and make sure every solution load triggers to execute init scripts for all of it's packages. So eventually it will improve solution load performance with or without DPL when there is no PMC console opened.
  2. Currently we hook 2 event handlers for solution load, one when executing any ps script, second when PMC is opened. But these are the same code executed twice. This PR will resolves this as well and make sure only single event handler is hooked.

Fixes NuGet/Home#4258

@rrelyea @emgarten @alpaix @mishra14 @rohit21agrawal

Copy link
Member

@emgarten emgarten left a comment

Choose a reason for hiding this comment

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

Does this call ExecuteInitScriptsAsync before every install?

From what I see it just calls it during the first initialization. If an install has already occurred the second solution will not have the init scripts run since the host was not loaded while that solution was opened.

@jainaashish
Copy link
Contributor Author

@emgarten Added code to call ExecuteInitScriptsAsync() for any new install/ uninstall for a solution for which we haven't executed init scripts. Also it will make sure to execute it only once per solution.

{
return _solutionManager.IsSolutionOpen ?
_solutionManager.SolutionDirectory :
Environment.GetEnvironmentVariable("USERPROFILE");
Copy link
Member

Choose a reason for hiding this comment

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

could this just use string.empty instead of userprofile? I don't see this path actually being used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already there in order to get the target directory so i didn't want to change that. Either way it wouldn't make any difference.

{
NuGetUIThreadHelper.JoinableTaskFactory.Run(async () =>
{
await ExecuteInitScriptsAsync();
Copy link
Member

Choose a reason for hiding this comment

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

It would help to mention that install.ps1/uninstall.ps1 scripts could depend on init.ps1 scripts, and that is why we need to run this first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the comment

@@ -355,6 +367,9 @@ private void UpdateWorkingDirectory()
return;
}

// save current solution directory so that we don't execute init scripts for the same solution again.
_currentSolutionDirectory = GetSolutionDirectory();
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to compare the current and previous directories here inside the lock and noop if they were already run? With the current pattern it looks like all (one) callers need to care about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, updated!

Copy link
Member

@emgarten emgarten left a comment

Choose a reason for hiding this comment

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

Address feedback then 🚀

@rrelyea
Copy link
Contributor

rrelyea commented Jan 18, 2017

test coverage? adequate today? needs tweaks? Additions?
Want to ensure no functionality breaks, this late in the game.

@@ -703,6 +703,40 @@ function Test-BatchEventsApi
Assert-True $result
}

function Test-ExecuteInitScriptsPerSolution
Copy link
Member

Choose a reason for hiding this comment

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

This test doesn't verify the change here.

Installing EF will run the init script and it will stay loaded. Also if any other tests install this package they will also run it.

If there are failures in the entityframework.sqlservercompact install script they won't be caught here, install-package ignores those. So at the end GetInstalledPackages will return everything regardless of what happened for init.ps1 and install.ps1.

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 know, but i cant find a way to restart the VS itself. Do you know how could we achieve it in our existing infrastructure?

Copy link
Member

Choose a reason for hiding this comment

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

To verify that scripts aren't running multiple times you will need to create a package with a custom init.ps1 script. In the init.ps1 script set a failure env var or some other state if it loads and has already been initialized.

From the test verify that the failure state isn't set. Also verify that the init.ps1 was loaded.

You can look at the other init.ps1 tests for examples of this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is any way to actually verify that install.ps1 loads all init.ps1 scripts first. Since the PM console has to be open to run E2E tests it should always load these first. You can't test the background host scenario.

@emgarten
Copy link
Member

emgarten commented Jan 19, 2017

The test looks much better ✨

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