sailfishos-patches / patchmanager

Patchmanager for SailfishOS
https://openrepos.net/content/patchmanager/patchmanager
Other
21 stars 22 forks source link

[PM3 suggestion] Retrieve the installed SFOS version by using 'version', not 'ssu re' #33

Closed Olf0 closed 2 years ago

Olf0 commented 5 years ago

Prelude: As some Patch authors fail to update the information, which SailfishOS releases a specific version of a web-catalog Patch is compatible to (even though that version is compatible to newer SFOS releases), users have practically no choice but to enable Patchmanager's Developer mode.

Observed behavior: While the update notifications for web-catalog Patches (per system notification, in Patchmanager's top pulley, in PM's list view of the web-catalog and the Patch specific sub-page there) are working as expected, when a device is on the current SailfishOS release. But when a device has an older SFOS release installed, some web-catalog Patches will have newer versions, which are only compatible to more recent SFOS release(s). One surely does not want to update these Patches. Still Patchmanager 3.0.55 notifies one via all aforementioned ways, that updates are available.

Suggestion to mitigate this: When checking for updated Patches from the web-catalog, compare the currently installed SailfishOS release (e.g. per version | rev | cut -f 2 -d ' ' | rev, but not per ssu re) with the compatibility information of a Patch, for which an update candidate (i.e. newer version) exists, before generating notifications (i.e. do not notify, if the currently installed SFOS release is not supported by the update candidate). IMO, this extra check always shall be active, regardless of Patchmanager's Developer mode being switched on or off.

Side note: This suggestion is only about when the update notifications are triggered. The ability to install any version of a Patch, when Patchmanager's Developer mode is active is not at all addressed (and shall be retained).

Tested on a Jolla 1 phone under SailfishOS 2.2.1.18 (while SFOS 3.0.0.8 has been released a few weeks ago) with Patchmanager 3.0.55.

P.S.: As issue reports and feature suggestions implicitly always have a slightly negative touch (as proposing to better something), an explicitly positive statement: Kudos for implementing the multiple, aforementioned ways of notifying that there are update candidates for web-catalog Patches. This is extremely helpful, already.

Olf0 commented 5 years ago

Oh, pardon me, Patchmanager 3.0.55 does all that almost fine! While the remark, "e.g. per version | rev | cut -f 2 -d ' ' | rev, but not per ssu re" was originally just meant to denote, that version outputs the currently installed SFOS release (in contrast to ssu re, which outputs the release set to upgrade to), PM3 already does most of above suggestion, but really seems to use ssu re instead of version to determine the currently installed SFOS release.

Background: Due to being conservative about upgrading to new SFOS releases on my "production phone", I often observed that something (presumably Jolla's store-client) sets SSU's target release to the new release number, even though I never opened Settings -> Sailfish OS updates (i.e. deliberately avoided doing that), but kept on using Jolla's Store app.

Observations: Thinking about above issue report filed yesterday, I checked if [ "$(version | rev | cut -f 2 -d ' ' | rev)" != "$(ssu re | rev | cut -f 1 -d ' ' | rev)" ], and yes, the "something" set SSU's target release to 3.0.0.8 (alike it happened before), while version correctly stated 2.2.1.18. After settings SSU's target release to the currently installed one and rebooting, Patchmanager 3.0.55 handles the compatibility to SFOS releases of web-catalog Patch versions fine, again.

Conclusion: So ultimately this issue has turned into a request to use a different way of determining the currently installed SailfishOS release than something which relies on SSU (as that does not reveal the currently installed SFOS release!).

Olf0 commented 5 years ago

SSU target release is set to 3.0.0.8 every day or reboot (by the "something"), which makes this issue is a little annoying.

Olf0 commented 2 years ago

Oh, I missed to denote here, that I was able to identify the "evil something" in May 2019: See [FSO] Bug: /usr/libexec/sailfish-osupdateservice osupdate-check sets ssu re (rsp. the original report [TJC] Bug: /usr/libexec/sailfish-osupdateservice osupdate-check sets ssu re).

But after all, the baseline still is: Do not use ssu re to determine the installed SailfishOS version, use version instead; as long as Jolla does not fix that bug (and they did not for 3,5 years)!

nephros commented 2 years ago

Meh, why not just use /etc/os-release directly? It's basically what version does anyway. It has a nice VERSION_ID variable making the parsing of the pretty name (which might be unreliable) unnecessary, and has a well-defined format and content.

[1] https://www.freedesktop.org/software/systemd/man/os-release.html#

Olf0 commented 2 years ago

Meh, why not just use /etc/os-release directly? It's basically what version does anyway. [...]

Correct, but that is not the point of this issue #33. The point is: Stop using ssu re! Even more so, as long as ssu re exhibits the ugly bug of informing about the version to update to, if one was detected (but not the installed one, then).

nephros commented 2 years ago

Meh, why not just use /etc/os-release directly? It's basically what version does anyway. [...]

Correct, but that is not the point of this issue #33. The point is: Stop using ssu re!

I realize that, and you propose the use of version as a replacement.

I argue a better replacement is the file because

  1. both location and content are standardized, and are maintained by upstream
  2. does away with the indirection of calling version, which does nothing but to read that file anyway
  3. no long pipestream is needed to parse version's output, most of which is not needed
Olf0 commented 2 years ago

O.K., an explicit "Ack" this time, than just a "thumbs up: :+1:" and an implicit "Correct, ...".

Side note: What I missed in your first reply, was something along the line "I realise that ...", which confirms a mutual / common understanding of the basic issue. BTW, due to my "thumbs up: :+1:" to your initial comment in this thread, IMO it should have been "..., and you originally proposed ...", not "..., and you propose ..." in your second comment here. :wink:

Olf0 commented 2 years ago

Thank you @elros34 for implementing this change per your "PM2-forever" commit ac0991b!

@nephros can you please check that commit ac0991b for correctness (just by reading the code thoroughly), if it applies cleanly to patchmanager/master and eventually apply it, when you have some time for this (it is definitely not urgent, after almost three years since reporting). It is QML, which I avoid to alter / handle / check, because I may create idiotic errors, without being able to notice that; and picking this commit and submitting it to patchmanager/master is beyond my standard repertoire of git handling (i.e., error prone, if I try).

nephros commented 2 years ago

Thank you @elros34 for implementing this change per your "PM2-forever" commit ac0991b!

@nephros can you please check that commit ac0991b

Nice find, good work.

Code seems fine, but:

  1. this bit, while valid I assume, seems unrelated to the overall change: https://github.com/elros34/patchmanager/commit/ac0991b1b27df010066fa4e3f8f287c6b1004a3e#diff-da6a1e0621bec30e25650901e8a69669572d302bbccc7f82e5fc81f4b90e4e68L312
  2. Thinking of the upcoming sandboxing, how do we feel about reading files from /etc instead of dbus-calling a system service? There are rumors that the latter might break when sandboxing is turned on fully, and the former will break certainly if the app profile doesn't allow it. (There's a separate issue #64 on that though) .
nephros commented 2 years ago

@nephros can you please check that commit ac0991b for correctness (just by reading the code thoroughly), if it applies cleanly to patchmanager/master

The answer is no, as the codebase this modifies is ancient and we are doing things differently in pm3 than was done in pm2. (E.g. we call the ssu dbus service from C++ not QML).

However concept can likely be ported - I like the usage of QSettings to read and parse the os-release file especially and will try to pick that up. I'll try a pm3 version - no guarantees as I don't actually speak C++ or Qt :D

nephros commented 2 years ago

This should do the same - however I'm not sure how exactly to test it. Of course editing /etc/os-release should work for that.

https://github.com/sailfishos-patches/patchmanager/pull/65

So please test, review, comment and rip it apart.