Skip to content

Fix install_distro_packages() to return True when packages already installed#6298

Merged
harvey0100 merged 1 commit into
avocado-framework:masterfrom
disgoel:fix-install-distro-pkgs
Jun 16, 2026
Merged

Fix install_distro_packages() to return True when packages already installed#6298
harvey0100 merged 1 commit into
avocado-framework:masterfrom
disgoel:fix-install-distro-pkgs

Conversation

@disgoel

@disgoel disgoel commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Problem
The install_distro_packages() function returns False when all required packages are already installed. This causes ensure_tool() (introduced in #6272) to incorrectly raise a RuntimeError, even though the packages are available.

Root Cause
The function initializes result = False and only sets it to True when software_manager.install() is called. When all packages are already installed, the installation block is skipped, leaving result as False.

This bug was exposed when testing avocado-misc-tests PR #3087, which uses ensure_tool() on systems where packages are already installed. The tests fail with RuntimeError: Failed to install packages for perf even though perf is installed and functional.

@mr-avocado mr-avocado Bot moved this to Review Requested in Default project Apr 15, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the install_distro_packages function in avocado/utils/software_manager/distro_packages.py to return True if all required packages are already present, rather than only when new packages are installed. It also adds a log message to indicate when no installation is necessary. I have no feedback to provide as there were no review comments to evaluate.

@codecov

codecov Bot commented Apr 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.30%. Comparing base (48854e8) to head (f0863b7).
⚠️ Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
avocado/utils/software_manager/distro_packages.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6298      +/-   ##
==========================================
- Coverage   72.31%   72.30%   -0.01%     
==========================================
  Files         206      206              
  Lines       23258    23260       +2     
==========================================
  Hits        16818    16818              
- Misses       6440     6442       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@harvey0100 harvey0100 self-requested a review April 24, 2026 12:52

@harvey0100 harvey0100 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I tested this locally -- install_distro_packages() returns False when everything's already installed, which makes ensure_tool() blow up with a RuntimeError even though the packages are there. The fix handles it correctly.

Just a heads up, the RTD CI failure isn't from your change -- it's a pre-existing docstring issue on master from #6274 the fix for that has been merged. Could you please do a rebase and then this one should be good to go :)

…stalled

The function was returning False when all required packages were already
installed, which caused ensure_tool() to incorrectly raise RuntimeError.

This fix ensures the function returns True when packages are available,
regardless of whether they were just installed or already present.

Signed-off-by: Disha Goel <disgoel@linux.ibm.com>
@disgoel disgoel force-pushed the fix-install-distro-pkgs branch from d1b3c21 to f0863b7 Compare April 24, 2026 13:16
@disgoel

disgoel commented Apr 24, 2026

Copy link
Copy Markdown
Contributor Author

LGTM. I tested this locally -- install_distro_packages() returns False when everything's already installed, which makes ensure_tool() blow up with a RuntimeError even though the packages are there. The fix handles it correctly.

Just a heads up, the RTD CI failure isn't from your change -- it's a pre-existing docstring issue on master from #6274 the fix for that has been merged. Could you please do a rebase and then this one should be good to go :)

Thanks for the review. I have rebased the code and sent again.

@disgoel disgoel requested a review from harvey0100 May 22, 2026 08:10
@harvey0100 harvey0100 merged commit 488be4b into avocado-framework:master Jun 16, 2026
12 checks passed
@github-project-automation github-project-automation Bot moved this from Review Requested to Done 114 in Default project Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done 114

Development

Successfully merging this pull request may close these issues.

2 participants