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

read_packages_file: include unavailable pkgs & simplify refresh_all_pkgapp_status #2557

Closed
wants to merge 1 commit into from

Conversation

Botspot
Copy link
Owner

@Botspot Botspot commented Mar 1, 2024

read_packages_file should be as thin a layer of translation as possible - playing the same role as cat'ing the app's packages file. It should not pull out unavailable packages or return nothing if one of the required packages are unavailable.
This has 3 advantages:

  • It put things back to how I intended
  • If the user views a hidden package-app, the "This app installs these packages" message will not be blank.
  • If the user manually tries installing a hidden package-app using manage, the error from apt will be helpful, rather than saying no packages were specified.

About the changes to the refresh_all_pkgapp_status function:

The current api on master completes refresh_all_pkgapp_status in 0.773+ seconds.
With this change, refresh_all_pkgapp_status completes in 0.624+ seconds. All while removing the duplicate codepath that combined features from read_packages_file, package_available, and refresh_pkgapp_status.

This is accomplished by redefining the package_available and package_installed functions in a bulk-friendly way for subprocesses to use, avoiding the need for a big pile of logic copied from where they belong in other functions.

Other changes:

  • Corrected edge case in refresh_pkgapp_status where it would skip showing an app if it is hidden but now installed. (Necessary for systems that have chromium available and installed)
  • If, for whatever reason, this function-redefining thing is a bad idea, I have added 2 optional arguments to refresh_pkgapp_status to specify the app's availability and installed status. That is a lot more comprehensible than the optional package input which can be left blank to force app-hiding.
    • Otherwise, I should probably remove those arguments before merge because they are not used.
  • Avoid redefining the $arch variable. Use $dpkg_arch instead. This is mostly to prevent my future self from ever getting confused about this again.

refresh_all_pkgapp_status: remove duplicate code and run
refresh_pkgapp_status on each app with optimizations
refresh_pkgapp_status: add arguments for installed and available, likely
to be removed before merge
@theofficialgman
Copy link
Collaborator

these changes regress performance compared to the current implementation on refresh_pkgapp_status with only one argument of input.
this is due to read_packages_file which calls package_available and then package_available is used again inside refresh_pkgapp_status
this is necessary because you changed back read_packages_file to output information that cannot be relied upon for determining the availability of all required packages so you need to check again.

@theofficialgman
Copy link
Collaborator

theofficialgman commented Mar 1, 2024

It should not pull out unavailable packages or return nothing if one of the required packages are unavailable.

so it is better to lie about the packages that will be installed then (eg: neither OR case are available and you output the first anyway)? how does this assist the user?

I think you have an identity crisis with the read_packages_file function. You want to use it for two distinctly different purposes and because of that it does a poor job of both.

theofficialgman added a commit that referenced this pull request Mar 1, 2024
theofficialgman added a commit that referenced this pull request Mar 1, 2024
@Botspot Botspot closed this Mar 1, 2024
@Botspot Botspot deleted the pkgapp-api-changes branch March 1, 2024 03:01
theofficialgman added a commit that referenced this pull request Mar 4, 2024
pick performance patches from #2557

if a secondary OR package is already installed and the first is not, list that as required instead of the first

don't redefine $arch, remove -qq flag

4 places in api use apt-cache policy, only 1 has -qq so it's probably fine to remove it. Otherwise it should go everywhere.

Co-Authored-By: Botspot <[email protected]>
theofficialgman added a commit that referenced this pull request Mar 4, 2024
pick performance patches from #2557

if a secondary OR package is already installed and the first is not, list that as required instead of the first

don't redefine $arch, remove -qq flag

4 places in api use apt-cache policy, only 1 has -qq so it's probably fine to remove it. Otherwise it should go everywhere.

check if `packages_file_required` is empty before attempting to install packages with manage script

Co-Authored-By: Botspot <[email protected]>
theofficialgman added a commit that referenced this pull request Mar 4, 2024
pick performance patches from #2557

if a secondary OR package is already installed and the first is not, list that as required instead of the first

don't redefine $arch, remove -qq flag

4 places in api use apt-cache policy, only 1 has -qq so it's probably fine to remove it. Otherwise it should go everywhere.

check if `packages_file_required` is empty before attempting to install packages with manage script

Co-Authored-By: Botspot <[email protected]>
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.

2 participants