Skip to content

Commit

Permalink
Stop chromium unnecessary reinstall from update to packages file
Browse files Browse the repository at this point in the history
  • Loading branch information
@Botspot
Botspot committed Feb 25, 2024
1 parent 1387b92 commit c202037
Showing 1 changed file with 9 additions and 0 deletions.
9 changes: 9 additions & 0 deletions manage
View file
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,16 @@ elif [ "$1" == 'install' ] || [ "$1" == 'uninstall' ];then
#if this app just lists a package-name, set the appscript to install that package

else #package-app: directly use apt to install what is mentioned in the packages file

appscript=(bash -c -o pipefail "apt_lock_wait ; sudo -E apt $(echo "$action" | sed 's/uninstall/purge --autoremove/g') -yf $(read_packages_file "$app") 2>&1 | less_apt")

#fix edge case: new will_reinstall function avoids a reinstall if packages to install do not change.
#unfortunately updater script does not source the new api so chromium is being reinstalled after we added "| chromium" to the packages file.

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024

Collaborator

this isn't true as far as I can tell.
all the (graphical) update modes call update_now_gui which sends all updatable apps/files as one argument to terminal_manage_multi

terminal_manage_multi then sends that multiline input argument to manaage daemon mode that then operates on files and refreshes first (so that includes the api file if it has been updated) and then updatable applications. apps are updated through a call to the manage script which always sources the api file (which as explained already got updated).

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024

Author Owner

I thought that this would not be a problem, but I looked at the chromium daily numbers and there is a clear spike due to this change.

Date		net	in	un	update
2024-02-19	451	486	35	3
2024-02-20	449	488	39	0
2024-02-21	452	492	40	2
2024-02-22	486	522	36	5
2024-02-23	546	582	36	4
2024-02-24	784	869	85	1504
2024-02-25	793	857	64	1283
2024-02-26	1323	1368	45	812
2024-02-27	1047	1077	30	488
2024-02-28	870	896	26	350

It seems to me that the only time the update shlink is clicked is after a successful app reinstall. Unless I am missing something, it appears you never made app refreshes click the update link.

Code from updater:

if [ "$installback" == 'yes' ];then
      #install the app again
      "${DIRECTORY}/manage" install "$app" update #report to the app install script that this is an install for the purpose of updating by passing "update"
      if [ $? != 0 ]; then
        failed=true
      else
        # click update link only if app is already installed and the update succeeded
        shlink_link "$app" update &
      fi
    fi

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024

Author Owner

I did not bother to try to reproduce the chromium update-reinstall through.

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024 •
edited
Loading

Collaborator

manage script is not re-called for refreshes. if you make it to this part in the script that we are commented under (which is under the elif [ "$1" == 'install' ] || [ "$1" == 'uninstall' ];then case) then you are in install/uninstall part of an update, not a refresh

what you said is correct about shlink. it is only called for app updates, not refreshes

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024

Author Owner

manage script is not re-called for refreshes. if you make it to this part in the script that we are commented under

Not sure what you are trying to say here. Are you saying I put code in the wrong place, or put inaccurate information about api sourcing in the comment?

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024

Collaborator

I thought that this would not be a problem, but I looked at the chromium daily numbers and there is a clear spike due to this change.

Date		net	in	un	update
2024-02-19	451	486	35	3
2024-02-20	449	488	39	0
2024-02-21	452	492	40	2
2024-02-22	486	522	36	5
2024-02-23	546	582	36	4
2024-02-24	784	869	85	1504
2024-02-25	793	857	64	1283
2024-02-26	1323	1368	45	812
2024-02-27	1047	1077	30	488
2024-02-28	870	896	26	350

the initial spike is on the 24th, not the 25th. the commit we are commenting on now is the 25th. So it isn't this commit that is causing the change.

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024

Collaborator

Are you saying I put code in the wrong place, or put inaccurate information about api sourcing in the comment?

the latter. I clicked + on the line #unfortunately updater script does not source the new api so chromium is being reinstalled after we added "| chromium" to the packages file. when adding my original comment. what I wrote just applies to that comment. that comment appears to be factually false since the new api script is sourced during any manage update/install/uninstalls

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024 •
edited
Loading

Author Owner

the initial spike is on the 24th, not the 25th. the commit we are commenting on now is the 25th. So it isn't this commit that is causing the change.

gman...
This commit we are looking at now is the solution. The problem started with this other commit, which indeed is on the 24th ed3023b

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024 •
edited
Loading

Collaborator

anyway I think you are missing the point.
refresh is only ever used for apps that are uninstalled and have new file contents on the pi-apps repo.
updates are always used for apps that are installed and have new file contents on the pi-apps repo.

so changing the packages file for chromium will go through the update case and will click the shlink for update. that is perfectly fine. just because the update link gets clicked doesn't mean that appscript=(bash -c -o pipefail "apt_lock_wait ; sudo -E apt $(echo "$action" | sed 's/uninstall/purge --autoremove/g') -yf $(read_packages_file "$app") 2>&1 | less_apt") got used

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024

Author Owner

what I wrote just applies to that comment. that comment appears to be factually false since the new api script is sourced during any manage update/install/uninstalls

OK understand. Once we figure out what actually did go wrong, we can update the comment.
With this commit, update-reinstalls for chromium will continue to happen due to the change I made on the 24th to the packages file. The shlink will continue to be clicked. This commit does not affect that, but it does prevent chromium from being actually reinstalled. With this commit, manage ignores an outdated updater's attempt to reinstall chromium.

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024 •
edited
Loading

Author Owner

anyway I think you are missing the point. refresh is only ever used for apps that are uninstalled and have new file contents on the pi-apps repo. updates are always used for apps that are installed and have new file contents on the pi-apps repo.

We changed the spec halfway through to say that update always coincides with will_reinstall == true, and refresh can happen for installed apps.
But if it is true that an app can receive a file-update without being reinstalled, then some other codepath would need to be clicking the update shlink. Again, this is the only place I see it

if [ "$installback" == 'yes' ];then
      #install the app again
      "${DIRECTORY}/manage" install "$app" update #report to the app install script that this is an install for the purpose of updating by passing "update"
      if [ $? != 0 ]; then
        failed=true
      else
        # click update link only if app is already installed and the update succeeded
        shlink_link "$app" update &
      fi
    fi

From this code, I do not see how the update shlink could possibly be clicked without manage install having been run first.

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024

Author Owner

So, since it seems the update shlink can only ever be clicked after a reinstall, I was forced to conclude that chromium was actually being reinstalled on thousands of people's computers.
I would love to be proven wrong.

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024 •
edited
Loading

Collaborator

We changed the spec halfway through to say that update always coincides with will_reinstall == true, and refresh can happen for installed apps.

right yeah. forgot about that case.

So, since it seems the update shlink can only ever be clicked after a reinstall, I was forced to conclude that chromium was actually being reinstalled on thousands of people's computers. I would love to be proven wrong.

ok I see what you were trying to say with the comment about api script not being updated.

as a result of the file change to chromium's packages you triggered an update for chromium (not a refresh). will_reinstall was returning true because the old method of checking for file differences between the old and new packages file was being used and not this change 7c6cf2f#diff-14c2529eb4498c5d1ffd6915d05bf58a91bdda796af59f41d480d11c099d0479R1460-R1466

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024

Author Owner

yes exactly

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024 •
edited
Loading

Collaborator

with the hotfix as part of this commit c202037, an update will still be triggered, and the shlink update link will still be clicked, but those apt commands as you said won't be run

This comment has been minimized.

Sign in to view

Copy link
@Botspot

Botspot Feb 29, 2024 •
edited
Loading

Author Owner

It seems that I could make a broader hotfix to validate_apps_gui in manage to silently change unnecessary app updates to refreshes, but I do not see the point.
Edit: actually it is the old version of manage daemon that is run; only manage subprocesses are new

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024

Collaborator

The determination of if an app will be reinstalled vs refreshed happens before any files are updated. In general it is part of showing the user the update dialog.

The only case where the will_reinstall output will be different is if you change the will_reinstall function (which is what you did)

This comment has been minimized.

Sign in to view

Copy link
@theofficialgman

theofficialgman Feb 29, 2024

Collaborator

Edit: actually it is the old version of manage daemon that is run; only manage subprocesses are new

yup. and because of that the place you put the check in this commit is the only place it really could go.
I think what you did here is basically the only thing that could be done given pi-apps architecture.

unfortunately even this does not help f5a5787 if the user clicks on show updates because api is not re-sourced before running list_updates_gui (which runs will_reinstall) even if api was updated as part of the background update process
#fix it here because new manage script sources the new api
if [ "$3" == "update" ] && ! will_reinstall "$app" ;then
#manage was told to reinstall app, but it does not actually need to. Do nothing
appscript=(bash -c -o pipefail "status 'Not reinstalling $app as no changes would be made.\nYou should only see this message once. Exiting.'")
fi
fi

#print to terminal
Expand Down

0 comments on commit c202037

Please sign in to comment.