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

(#3237) Reduce number of queries for dependencies #3260

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

gep13
Copy link
Member

@gep13 gep13 commented Jul 10, 2023

Description Of Changes

This commit attempt to address this problem by changing to only query for the latest package version (and/or the requested package version) when attempting the look up for the parent package dependencies. This greatly reduces the number of outgoing queries, but it also means that it is possible for things to not work correctly due to not all package information being available. To guard against this, a fallback has been put in place such that if the initial attempt at resolving the solution fails, it goes back and fetches information about all package versions for the parent packages, and attempt to gather the resolve the solution again.

This change has been made in a way that there is some code duplication, however, based on some discussion within the team, the way that this method is written needs to be overhauled, and now is not the right time to do this. Instead, we are accepting the duplication, until some effort can be put in to overhaul this method completely.

Motivation and Context

When attempting to figure out what packages need to be verified/checked during a package upgrade operation, we were previously fetching information about all package versions for all "parent packages" (i.e. packages that include a dependency on the package being acted on). This causes a problem when doing something like choco upgrade chocolatey when you have the chromium package installed. When in this situation, due to the high number of package versions for chromium and the fact that it takes a dependency on Chocolatey, the number of queries that are emitted for Chocolatey CLI is very large. This makes it appear as though the operation has stalled, when in actual fact it is just working its way through the queries (this can be verified by looking at the requests through fiddler when this command is running).

Testing

  1. Open Fiddler
  2. Clone PR locally onto machine
  3. Install chromium package through Visual Studio debugging
  4. Manually copy and rename the Chocolatey CLI v2.0.0 nupkg into the debug/lib folder, replacing the nupkg that is already there
  5. Run the choco upgrade chocolatey command through Visual Studio debugging
  6. Verify in Fiddler that there aren't hundreds of requests being made, and then that the operation completes in a reasonable timeframe
  7. Run the command choco install notepadplusplus --version 8.4.5 through Visual Studio debugging
  8. Run the command choco upgrade notepadplusplus.install --version 8.5.1 through Visual Studio debugging
  9. Verify in Fiddler that there aren't hundreds of requests being made, and then that the operation completes in a reasonable timeframe NOTE: The expectation here is that both the .install and the meta package is upgraded correctly
  10. Run the command choco upgrade notepadplusplus.install through Visual Studio debugging
  11. Verify in Fiddler that there aren't hundreds of requests being made, and then that the operation completes in a reasonable timeframe NOTE: The expectation here is that both the .install and the meta package is upgraded correctly
  12. Clear out the lib folder in the debug directory
  13. Repeat steps 3 and 4, and then run the command choco upgrade chocolatey --version 2.1.0
  14. Verify in Fiddler that there aren't hundreds of requests being made, and then that the operation completes in a reasonable timeframe

Operating Systems Testing

  • Windows 10

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Fixes #3237

@gep13 gep13 requested a review from vexx32 July 10, 2023 14:58
@gep13 gep13 force-pushed the package-resolution-fix branch from 595985d to f4f25f6 Compare July 11, 2023 08:12
Copy link
Member

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

few tiny things

Logic is Not Fun to look through and duplication is... well. 😔
But if it works 😁

@gep13 gep13 marked this pull request as ready for review July 11, 2023 12:40
Copy link
Member

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Seems to work as described. Great work figuring out a way this can be improved!

When attempting to figure out what packages need to be verified/checked
during a package upgrade operation, we were previously fetching
information about all package versions for all "parent packages" (i.e.
packages that include a dependency on the package being acted on). This
causes a problem when doing something like `choco upgrade chocolatey`
when you have the chromium package installed.  When in this situation,
due to the high number of package versions for chromium and the fact
that it takes a dependency on Chocolatey, the number of queries that
are emitted for Chocolatey CLI is very large.  This makes it appear as
though the operation has stalled, when in actual fact it is just
working its way through the queries (this can be verified by looking at
the requests through fiddler when this command is running).

This commit attempt to address this problem by changing to only query
for the latest package version (and/or the requested package version)
when attempting the look up for the parent package dependencies.  This
greatly reduces the number of outgoing queries, but it also means that
it is possible for things to not work correctly due to not all package
information being available.  To guard against this, a fallback has
been put in place such that if the initial attempt at resolving the
solution fails, it goes back and fetches information about all package
versions for the parent packages, and attempt to gather the resolve the
solution again.

This change has been made in a way that there is some code duplication,
however, based on some discussion within the team, the way that this
method is written needs to be overhauled, and now is not the right time
to do this.  Instead, we are accepting the duplication, until some
effort can be put in to overhaul this method completely.
@vexx32 vexx32 force-pushed the package-resolution-fix branch from d6540ed to 46edab5 Compare July 11, 2023 21:14
@vexx32
Copy link
Member

vexx32 commented Jul 11, 2023

@gep13 I've gone ahead and rebased this branch so we can merge it

@vexx32 vexx32 merged commit a3305eb into chocolatey:develop Jul 11, 2023
@gep13 gep13 deleted the package-resolution-fix branch July 12, 2023 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants