[Bug] allow incompatible not working as expected

[nephros]

it seems #334 introduces one or several bugs where

1. the setting isn't initialized properly after updating from a previous version of PM. One has to toggle the option list to make the wanted option stick.
2. while incompatible patches are allowed to be activated and show up as such, the aren't actually applied by the PM backend. <-- not actually this bug here

test example patches:

• "smooth remorse timers"
• "browser tab pulley menu"

[nephros]

Point 2. is not actually true - it's in the situation after a PM update where either there's still the old version of PM daemon running, or the old version of the preload library used (e.g. by lipstick or the browser booster).

[Olf0]

Do I understand correctly, that point 2 is not a regression, i.e., "always has been this way for PM3"?

Does "the setting" in point 1 address only a Patch's "enabled / disabled" status as GUI (= user) setting?

[nephros]

Do I understand correctly, that point 2 is not a regression, i.e., "always has been this way for PM3"?

I think yes.
It could also be that it only appears for someone like me who switches around PM versions a lot.

Sometimes (apparently) downgrading when I test one of my development builds. The combination of applying/unapplying patches, while sometimes not restarting lipstick, and sometimes installing a different build of PM in the middle of this can lead to a "confused" situation of the system.

The interesting part I'm not sure about is:

If I

• LD_PRELOAD=patchmanager.so[version X] lipstick
• ... do things
• update PM so it installs patchmanager.so[version Y]
• do not restart lipstick

... will patchmanager.so[version Y] do its thing? I think not, as it's not really loaded into the lipstick process.

[nephros]

About 1, I think what happens is:

The initialization sequence of QML Components is "undefined" for "siblings" - however children are initialized before parents.

So when SettingsPage.qml loads on a System where a migration from old developerMode should happen,

1. ComboBox loads, and its property is checked:

                onCurrentIndexChanged: PatchManager.sfosVersionCheck = currentIndex
                currentIndex: (PatchManager.sfosVersionCheck) ? PatchManager.sfosVersionCheck : VersionCheck.Strict

now at this point (on a never-migrated system), PatchManager.sfosVersionCheck is undefined, so it gets set to VersionCheck.Strict. So far so good, we agreed that we should default to the conservative option.

But that I think does not trigger the onCurrentIndexChanged signal handler, so the actual PM setting never gets set.

Only interactively toggling the switch will do it.

Later, the migration code will fire and toggle the switch again, if developerMode was true. Maybe that doesn't do the right thing, OR maybe there's a race with the plugin's dbus calls to the daemon.

[Olf0]

WRT point 2:

The interesting part I'm not sure about is: If I

• LD_PRELOAD=patchmanager.so[version X] lipstick
• ... do things
• update PM so it installs patchmanager.so[version Y]
• do not restart lipstick

... will patchmanager.so[version Y] do its thing? I think not, as it's not really loaded into the lipstick process.

So it is basic requirement would be to reload the shared library patchmanager.so (in its freshly deployed version Y), which means to restart the daemonized Patchmanager running in the background (correctly understood?): Is this feasible at all? I.e., without casing any interruptions.

[Olf0]

About 1, I think what happens is:

[…] now at this point (on a never-migrated system), PatchManager.sfosVersionCheck is undefined, so it gets set to VersionCheck.Strict. So far so good, we agreed that we should default to the conservative option.

But that I think does not trigger the onCurrentIndexChanged signal handler, so the actual PM setting never gets set.

Only interactively toggling the switch will do it.

Reading this, it sound not hard to resolve, if that is indeed the cause, right?

[nephros]

So it is basic requirement would be to reload the shared library patchmanager.so (in its freshly deployed version Y), which means to restart the daemonized Patchmanager running in the background (correctly understood?): Is this feasible at all?

No, what should happen in the case that patchmanager.so actually does something different in Y than in X, that every process that has it loaded (i.e. almost all processes, as LD_PRELOAD is more or less global) should restart so they load the new version.
That is definitely not feasible. (At most, we can give a note to the user they should reboot after install but, meh.)

Incidentially, daemonized patchmanager is one of the very few processes that is exempt from the effects of the lib, as it sets the magic env var NO_PM_PRELOAD, which basically makes the lib do nothing.

So indeed the only way to update PM and be sure all changes actually are live is to boot afresh (well, or kill all processes which have the .so loaded which amounts to almost the same thing).
BUT, in reality this should rarely be needed as breaking changes to the .so almost never happen.

[nephros]

Explanation variant 2 for Point 1:

As above, but actually in patchmanager we default to 0 (== Strict), so that check for PatchManager.sfosVersionCheck in QML is not uninitialized but 0, which leads to a double whammy setting it - again - to Strict ;)

[nephros]

If Explanation 1 is true, the setting will never reach persistent storage on-disk, and everything will always default to "Strict" checking in that case.

If Explanation 2 is true, the setting will be saved as "Strict" on-disk.

In both cases the fix for a user is to wiggle the setting.

As for code fixes, the latter is actually unfixable AFAICS, as damage is done for anyone who updated from PM < 3.2.6 and has ever launched the Settings page.

[Olf0]

[…] (At most, we can give a note to the user they should reboot after install but, meh.)

Well, this is better than something not working without such a indication ("without a reboot strange things may happen"). Jolla's version dup does exactly that (though it is a CLI app).

BUT, in reality this should rarely be needed as breaking changes to the .so almost never happen.

Mmmh, you observe it, so anybody else could. I will follow any assessment from you here, because I do not have the time and barely have the know-how to retrace that, but just want to ask "checks & balances" questions.

P.S.: I had to chuckle when reading "Incidentially", because I tend to do the same spelling mistake and a related one regularly with a couple of similar words. But there is no second "i" in "accidentally", "incidentally", plus there is no "c" but (always) a "t" in these: "instantiated", the aforementioned two and a couple of similar ones. I love to spell "accidentially", "instanciated" etc. and wonder what's wrong.

[nephros]

and has ever launched the Settings page.

Oh, and there's another thing: someone who has updated, but never opened Settings also does not benefit from the migration code.

I guess that will be the majority of users - and they will see behaviour of the the defaults (off) for both new switches.

[nephros]

Summing up:

• wrt. changes to the shared library:

IF we ever see the need to make significant changes to that component's code, we should very strongly advice a reboot to users, maybe even force a lipstick restart in a rpm post* script. (Although I would vote against that for various reasons.)

Until that time (and since we took over, no functional changes have happened to the library code) I see no need for action.

• wrt. restarting the PM daemon after an update:

If I read the .spec correctly, we already do that more than once in the %post/%preun/%postun scriptlets.
I think I might hit the special case that I sometimes (from rpm's point ov view) do a downgrade of PM, in which case the argument handling in those scriptlets might decide a reload/restart is not necessary.

• wrt. the actual bug here

I'll do some more tests to verify what has been said, but is seems clear to me the upgrade handling should be moved from QML into the daemon, as it may be a long period of time between a PM upgrade and the user going into Settings in the UI.
Alternatively, migration could be done in the .spec directly on the config file, but that feels a bit hackish to me. OTOH it means less "dead" code in the product itself.

Also, users should be made aware of this bug, and while no definite fix has been deployed, be advised to go into Settings and confirm their preference there (and as noted, there may be no other way to resolve it).

Personally, I shall test more thoroughly in the future :D

[Olf0]

Summing up:

• wrt. changes to the shared library: IF […]

IMO this is way too complicated to communicate and to remember and check when doing such a "potentially critical" release. Is there anything wrong in communicating:

Always reboot after deploying a different Patchmanager release: I.e., after every installation, upgrade, re-installation (or even downgrade) of Patchmanager do restart your device!

This is simple and unambiguously phrased statement, which is easy to comprehend and unconditionally applicable. If you agree I will try to find am appropriate location (e.g., the user section of our README) to denote that.

• wrt. restarting the PM daemon after an update: If I read the .spec correctly, we already do that more than once in the %post/%preun/%postun scriptlets. I think I might hit the special case that I sometimes (from rpm's point ov view) do a downgrade of PM, in which case the argument handling in those scriptlets might decide a reload/restart is not necessary.

I may take a look at this, if we can easily restart more often unconditionally. Though, I might not be able to perform this task properly without in-depth knowledge what happen outside of the spec file (i.e., in / by Patchmanager).

• wrt. the actual bug here I'll do some more tests to verify what has been said, but is seems clear to me the upgrade handling should be moved from QML into the daemon, as it may be a long period of time between a PM upgrade and the user going into Settings in the UI.

:+1:

Alternatively, migration could be done in the .spec directly on the config file, but that feels a bit hackish to me. OTOH it means less "dead" code in the product itself.

Sounds good (and simpler) to me. Maybe we should rather follow this route, also because that will be maintainable by shell code / spec file adaptions only.

Also, users should be made aware of this bug, and while no definite fix has been deployed, be advised to go into Settings and confirm their preference there (and as noted, there may be no other way to resolve it).

Again: I see no practical way to communicate such a specific, complex message to all users. Only messages hammered into the heads over years will stick; unfortunately un-reversible and often incorrectly simplified (any condition, context and reasons will be omitted) as "Do disable all OpenRepos repositories before upgrading SailfishOS [from a release below 1.0.4 to 1.0.4 or higher]" or "Disable all Patches before upgrading SailfishOS [if you are using Patchmanger < 3]". This is why any such message must be dead simple, free of any conditionals, correctly understandable out of context etc.: i.e., easy to comprehend even for mendicants (morons, idiots, "DAUs").

Personally, I shall test more thoroughly in the future :D

Lol, a conclusion most developers regularly come to. But then, time is scarce, I rather implement new things that to test thoroughly: This is why I love CI runs and do deliberately cut down on testing recently (only "smoke tests": it is O.K. if it starts and performs its basic functionality fine). Plus it is FLOSS: No formal responsibility, and while I try to not publish crap, to shift testing in detail to users is a valid strategy to rather code than test by oneself. YMMV, but I do believe it makes sense to evaluate every now and then how much time to spend on which FLOSS activities to be most efficient and effective.

[nephros]

• wrt. the actual bug here I'll do some more tests to verify what has been said, but is seems clear to me the upgrade handling should be moved from QML into the daemon, as it may be a long period of time between a PM upgrade and the user going into Settings in the UI.

Meh, change of plan. The daemon doesn't actually know about dev mode or patch compatibility (it just knows the "applied" === "activated" list, and will read and write the settings entries, but does not deal with most of their values). It's all on the UI side.

But I think I have found a fix for any of this (below).

Alternatively, migration could be done in the .spec directly on the config file, but that feels a bit hackish to me. OTOH it means less "dead" code in the product itself.

Sounds good (and simpler) to me. Maybe we should rather follow this route, also because that will be maintainable by shell code / spec file adaptions only.

Yeah, turns out is' not totally simple - one needs to restart the daemon after changing the file (so it gets read again), otherwise daemon state, settings content, and UI state are out of sync. (This is because the deamon reads the file only at launch, and after that keeps state of most setting values in memory, and will not check that state against the file on all reads or writes. So it will not write a value that has not changed compared to its memory, and will return memory values, not stored values, to any clients, like the GUI. Therefore, changing the file behind its back will cause its state to not match it.)

The correct way to to a migration from .spec would then mean:

1. check that the daemon is running
2. if no, change the config file on-disk
3. if yes, use several DBUS calls to:
   1. determine value of developerMode
   2. if it is true, set the two new settings to the correct values
   3. set developerMode to false

call org.SfietKonstantin.patchmanager.putSettings(name, value) call org.SfietKonstantin.patchmanager.getSettings(name, default)

Or of course:

1. check that the daemon is running
2. if yes, stop it
3. change the config file on-disk
4. if it was running, start it again

And, I'm not quite sure how the GUI will react if it is running while all of this happens. My guess would be though that it would also require a restart after this is done.

So, all doable but I think the QML route is simpler after all:

https://github.com/sailfishos-patches/patchmanager/compare/master...nephros:patchmanager:fix-405

^^^ this will run the migration immediately when launching the GUI (Settings), and everything should be okay from that point on.

[nephros]

Summing up:

• wrt. changes to the shared library: IF […]

IMO this is way too complicated to communicate and to remember and check when doing such a "potentially critical" release. Is there anything wrong in communicating:

Always reboot after deploying a different Patchmanager release: I.e., after every installation, upgrade, re-installation (or even downgrade) of Patchmanager do restart your device!

This is simple and unambiguously phrased statement, which is easy to comprehend and unconditionally applicable. If you agree I will try to find am appropriate location (e.g., the user section of our README) to denote that.

Absolutely fine, but as said I don't see the need to do that now, or in the near future.

[nephros]

Again: I see no practical way to communicate such a specific, complex message to all users.

I'll do a quick post about it on the Forum, and put a link to it to the two Telegram groups (English and German). This should reach a good amount of users, and get the word out.

The effect of this bug is a small annoyance after all, and fixable by curious users by trial and error (toggling the switch).

[b100dian]

I'll do a quick post about it on the Forum

Thanks for this and for the investigation.