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

Api update (install_packages and failed installs) #1323

Merged
merged 4 commits into from
Jan 1, 2022

Conversation

theofficialgman
Copy link
Collaborator

@theofficialgman theofficialgman commented Dec 30, 2021

  • Removed the unnecessary dummy deb package purge if install_packages is run multiple times in a script
  • moved dummy deb package purge to occur if the entire app install script fails (that way dummy debs for a "corrupted" install do not remain on the users OS)

these changes now allow for script writers to implement install_packages in their script and allow for it to fail non-fatally and keep the previous Depends: packages

for example: its nice to have adoptopenjdk-16 but not necessary for MultiMC to work, so install_packages for this specific package is made non-fatal, but without the changes here, ALL other previously installed dependencies are removed from the dummy deb since it is purged

for scripts where failing install_packages is a fatal error, there is no change as the dummy deb gets purged by the manage script now.

removed unnecessary functionality that results in the dummy deb for an app being removed if install_packages fails

removing this functionality allow for install_packages to be used within a script in a non-fatal manner, where previous install_packages dependencies are saved
manage Outdated Show resolved Hide resolved
api Show resolved Hide resolved
@theofficialgman
Copy link
Collaborator Author

theofficialgman commented Dec 31, 2021

these changes now allow for script writers to implement install_packages in their script and allow for it to fail non-fatally and keep the previous Depends: packages

for example: its nice to have adoptopenjdk-16 but not necessary for MultiMC to work, so install_packages for this specific package is made non-fatal, but without the changes here, ALL other previously installed dependencies are removed from the dummy deb since it is purged

to add to this, I've noticed at least one other script is already expecting this non-fatal install_packages and don't purge the dummy deb behavior (the new behavior). Specifically, the cura script where if stl-thumb fails to install the script continues and will only throw a warning.

manage Outdated
#remove dummy deb if app failed to install (to avoid dummy debs being left on a users install for broken apps)
if [[ ${mode} == "install" ]] && [[ "$(app_type "$app")" == standard ]]; then
local package_name="$(app_to_pkgname "$app")"
local output="$(sudo -E apt purge -y $package_name 2>&1 | less_apt | tee /dev/stderr)"
Copy link
Owner

@Botspot Botspot Dec 31, 2021

Choose a reason for hiding this comment

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

Why run apt purge rather than purge_packages? Also this may run unnecessarily if the dummy deb is not installed, so a check needs to be added to address that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

an unnecessary run isn't a big deal, but yeah that can be checked

I ran apt purge because that is what you ran in your script. It mimics the previous functionality by moving it to the manage script only when the app fails while not having the drawbacks of purging the dummy deb unnecessarily during a non-fatal install_packages run

  #If this app's dummy deb is already insalled, uninstall it
  if package_installed "$package_name" ;then
    status "Reinstalling the $package_name package..."
    apt_lock_wait
    local output="$(sudo -E apt purge -y $package_name 2>&1 | less_apt | tee /dev/stderr)"
    if [ $? != 0 ];then
      echo "$output"
      error "Failed to purge dummy deb ($package_name)"
    fi
  else
    status "Installing the $package_name package..."
  fi

the main difference between running what I have written now and doing purge_packages is purge_packages will do an apt autoremove.
I think an autoremove is fine for a failed first time install but I don't think its a good idea for a failed update.

@theofficialgman
Copy link
Collaborator Author

theofficialgman commented Dec 31, 2021

also @Botspot I just noticed this:

    #set the app's status to 'corrupted' if the diagnostics determine the error is NOT internet or system or package, AND if the app is a script-type app
    if ! [[ "$(log_diagnose "$logfile" | head -n1)" =~ ^(system|internet|package)$ ]] && [ "$(app_type "$app")" == app ];then
      echo "corrupted" > "${DIRECTORY}/data/status/${app}"
    fi

is this a bug? I thought the only app_type was "standard" and "package". no such thing as "app" and that will prevent the app status from getting set as corrupted

app_type() { #there are 'standard' apps, and there are 'package' apps - an alias to install an apt package from the existing repositories.
  #$1 is input app-name
  local app="$1"
  [ -z "$app" ] && error "app_type: no app specified!"
  
  if [ -f "${DIRECTORY}/apps/${app}/packages" ];then
    echo package
  elif [ -f "${DIRECTORY}/apps/${app}/uninstall" ] || [ -f "${DIRECTORY}/apps/${app}/install" ] || [ -f "${DIRECTORY}/apps/${app}/install-32" ] || [ -f "${DIRECTORY}/apps/${app}/install-64" ];then
    echo standard
  else
    return 1
  fi
  #if neither conditional above evaluated to true, no output will be returned and the function exits with code 1
}

@theofficialgman
Copy link
Collaborator Author

looks like you introduced the bug 2 weeks ago in this commit eea069e

@Botspot
Copy link
Owner

Botspot commented Dec 31, 2021

Wow you're right.

…oval if part of an update

also check if dummy deb is installed before purging

remove local definitions as this is not a function and unset variables since they should not be set
@theofficialgman
Copy link
Collaborator Author

@Botspot I think this addresses your concern 13f57b8

(also I assume you will just go ahead and fix that unrelated bug on your own at some point)

@Botspot Botspot merged commit 08f4be9 into Botspot:master Jan 1, 2022
@theofficialgman theofficialgman deleted the api-update branch January 1, 2022 07:35
@Botspot Botspot mentioned this pull request Jan 1, 2022
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.

None yet

2 participants