Optimise manglelist handling (Was: Fails if a Patch contains multiple sections matching different elements on the manglelist!)

[Olf0]

AFAICS, the function mangle_libpath fails if a Patch contains multiple sections matching different elements on the manglelist.conf (which is sourced as MANGLE_CANDIDATES in pm_apply): The return statement is placed right before the done, hence the for loop is run once at most (i.e., it never loops).

But AFAICS, a Patch might deliberately contain multiple sections matching different elements on the manglelist.conf, thus the for loop should iterate through the elements of MANGLE_CANDIDATES.

Am I missing some aspect, or is this really a flaw worth pondering about (i.e., developing an enhancement)?

[nephros]

I agree that return statement should go.
I'll have to set up a proper test case before I can say for sure whether that's a one-liner or more changes are required. I don´t trust my brain with analyzing loops.

Edit: yeah, no, we need some more involved re-looping if we want to support "mixed" patches, i.e. patches which match more than one "candidate".

[Olf0]

yeah, no, we need some more involved re-looping if we want to support "mixed" patches, i.e. patches which match more than one "candidate".

I will ponder about it and likely come up with a suggestion. It should not be too hard, rather getting this loop closer to general programming guidelines (i.e., "do avoid jumping out of loops, rather adapt the loop for that to become unnecessary"), which makes it simpler to comprehend.

P.S.: As this is shell code, I should be able to handle that well. What I am rather concerned of, after finding a couple of flaws in these few lines of new code, are the "mangling"-related changes elsewhere, which are way harder for me to understand due to being written in languages foreign to me. I know you partially or mostly authored them, which makes reviewing hard, but maybe after many weeks you can look at these changes / additions as if they were written by someone else; i.e., can you try to review MRs #109 and #140 again (or better, but I fail to find it: whichever MR merged the "patch-bitness" branch into "master").

[Olf0]

^{Missed to mention this:} Yes, please do create a test case, sometime (not urgent; i.e., take your time).

[Olf0]

yeah, no, we need some more involved re-looping if we want to support "mixed" patches, i.e. patches which match more than one "candidate".

I will ponder about it and likely come up with a suggestion.

• Preparation, with current functionality unaltered (hopefully): MR #221 Please merge and / or test that first!

• Implementing looping over all elements of the candidates: MR #225 Actually this seemed to be the easier step, after proper preparation.

Please review and test. Thanks!

P.S.:

I'll have to set up a proper test case before I can say for sure whether that's a one-liner or more changes are required.

Yes, please do create a test case, sometime (not urgent, i.e., take your time).

Are your "test cases" scripts, or performed ad-hoc and manually? If any of your test cases are fully or mostly programmatically executable, I would appreciate much when you provide them here at GitHub, e.g. in a new test-cases top-level directory.

[nephros]

Yes, please do create a test case, sometime (not urgent, i.e., take your time).

Are your "test cases" scripts, or performed ad-hoc and manually? If any of your test cases are fully or mostly programmatically executable, I would appreciate much when you provide them here at GitHub, e.g. in a new test-cases top-level directory.

Unfortunately not.

I had begun assembling such a set of files (no automation though!) but gave up and deleted them again, thinking it would be too complicated or too error-prone to release.
So I'll have to start from scratch :/

[Olf0]

1. Fixing this issue (#220)

Please review MR #225. I do not really mind who merges it (although at the start we had the soft convention that usually the original committer does that, which I liked), but please do not squash the two commits.

2. Test cases

Yes, please do create a test case, sometime (not urgent; i.e., take your time).

Are your "test cases" scripts, or performed ad-hoc and manually? If any of your test cases are fully or mostly programmatically executable, I would appreciate much when you provide them here at GitHub, e.g. in a new test-cases top-level directory.

Unfortunately not. I had begun assembling such a set of files (no automation though!) but gave up and deleted them again, thinking it would be too complicated or too error-prone to release. So I'll have to start from scratch :/

^{Side note: IMO anything helpful is worth to be published, because any one of us could be ceasing to contribute tomorrow (for a variety of reasons). Thus manually applicable test cases / Test-Patches with rough and terse notes how to use them are better than nothing. E.g., I still would really like to obtain a clue / hint what the original considerations for PM4 were; maybe @b100dian can try to kindly ask @CODeRUS to provide a little feedback.} As stated, "take your time" to address this topic. Although when MR #225 is merged, I would appreciate if you test that somehow, be it fully manually and privately, because after addressing issue #17 (well, a bit more: reworking all strings; I am working on this in a branch at my fork of PM), I would like to start preparing a v3.2.1 release.

3. Re-reviewing the former "patch-bitness" branch

^{As denoted before (in the P.S. there): What I am rather concerned of, after finding a couple of flaws in these few lines of new [shell] code [in pm_apply], are the "mangling"-related changes elsewhere, which are way harder for me to understand due to being written in a language much more foreign to me (C++). I know you partially or mostly authored them, which makes reviewing hard, but maybe after many weeks you can look at these changes / additions as if they were written by someone else; i.e., can you try to review MRs #109 and #140 again (or better, but I fail to find it: whichever MR merged the "patch-bitness" branch into "master").}

[nephros]

Reviewing the PRs (thanks Olf!) I had another idea.

Right now the script does basically

 foreach candidate_line in candidate_list
     sed  's/candidate_line/fixed_line/' patchfile
 done

(... and as noted, the original implementation only does the first loop)

What we also could do instead is just

 foreach line in candidate_list
     #create a sed command string or sed command file like
      SEDCOMMAND += s/line/fix/ ;
 done
 # apply all sed manging in one go
 # SEDCOMMAND is something like 's/pattern1/fix1/ ;  s/pattern2/fix2/; s/pattern3/fix3 ;
 sed -e $SEDCOMMAND patch

OR

remove /etc/patchmanager/manglelist.conf completely, replacing it with mangle32.sed and mangle64.sed sed command files, and in pm_apply simply do a

 if (64bit)
     sed -f mangle64.sed patchfile
 elseif (32bit)
     sed -f mangle32.sed patchfile

Thoughts? :)

[nephros]

2. Test cases

Yes, please do create a test case, sometime (not urgent; i.e., take your time).

Are your "test cases" scripts, or performed ad-hoc and manually? If any of your test cases are fully or mostly programmatically executable, I would appreciate much when you provide them here at GitHub, e.g. in a new test-cases top-level directory.

Unfortunately not. I had begun assembling such a set of files (no automation though!) but gave up and deleted them again, thinking it would be too complicated or too error-prone to release. So I'll have to start from scratch :/

Side note: IMO anything helpful is worth to be published, because any one of us could be ceasing to contribute tomorrow (for a variety of reasons). Thus manually applicable test cases / Test-Patches with rough and terse notes how to use them are better than nothing.

A first set of test cases are now in branch test-cases.

Discussion here: https://github.com/sailfishos-patches/patchmanager/discussions/232

[Olf0]

Reviewing the PRs (thanks Olf!) I had another idea.

Right now the script does basically

 foreach candidate_line in candidate_list
     sed  's/candidate_line/fixed_line/' patchfile
 done

(... and as noted, the original implementation only does the first loop)

What we also could do instead is just

 foreach line in candidate_list
     #create a sed command string or sed command file like
      SEDCOMMAND += s/line/fix/ ;
 done
 # apply all sed manging in one go
 # SEDCOMMAND is something like 's/pattern1/fix1/ ;  s/pattern2/fix2/; s/pattern3/fix3 ;
 sed -e $SEDCOMMAND patch

OR

remove /etc/patchmanager/manglelist.conf completely, replacing it with mangle32.sed and mangle64.sed sed command files, and in pm_apply simply do a

 if (64bit)
     sed -f mangle64.sed patchfile
 elseif (32bit)
     sed -f mangle32.sed patchfile

Thoughts? :)

I took my time to think about this in order not to come up with premature answers. I will categorise this reply into two sections, one for the technical aspects and a second one for procedural ones.

I. Technical aspects

1. Your first suggestion is an optimisation of the current implementation, which reduces file I/O. As I have stated before (second half of point 2.), the pm_apply script is a "hotspot", i.e., the place where Patchmanager spends most time when activating all Patches on OS boot-up. Hence I do think we should pursue this (see also section "II. Procedural aspects").
2. Your second, alternative suggestion maximises the pre-computing of the sed-scriptlets in order to further optimise the run-time of pm_apply. But it comes at a cost, which primarily is the much more complex API, which you defined as "user-facing": A simple list of path prefixes is transformed into two sed-scriptlets (with mutually dependent content), which are still to be handled by users. _{BTW, there is one thing shell scripts are (comparably) quick at, which are string operations (as pointed out in the first half of point 2.).} The only strong reason to pursue this would be IMO, if you expect the two sed-scriptlets ever not to be automatically (i.e., programmatically) transformable into each other. AFAICS that is not the case, rather the opposite: They always shall be programmatically transformable into each other. If you concur with this assessment, we should keep it that way and do programmatically transform one into the other (i.e., the simpler 32-bit version into the 64-bit one), as it currently is the case.

II. Procedural aspects

1. I think we should progress this step-by-step: My current PR #225 aims to be a simple fix of the issue we identified in this issue. I think it provides a correct, traceable and comprehensible code change to achieve this. Hence it should be merged first. Additionally this PR has accumulated the counterparts of PRs #226 and #228 meanwhile: I would like to avoid to tediously untangle these commits. Thus I am asking you to please finish your review of PR 225 and approve it (or suggest changes), as well as the related PR 228 for pm_unapply.
2. I am sure willing to implement you first suggestion as an optimisation, when PR 225 has been merged. I can well imagine how to achieve that easily, but do not want to overload PR 225 with additional commits. There I also might address https://github.com/sailfishos-patches/patchmanager/discussions/232#discussioncomment-1966155 , which seems to be easy, too.

IMO this approach best provides a proper commit history of simple, traceable and comprehensible changes and limits the organisational overhead for us.

[Olf0]

Oh, I had auto-close switched on for this issue! Re-opening due to nephros' request https://github.com/sailfishos-patches/patchmanager/issues/220#issuecomment-1012297290 and my reply https://github.com/sailfishos-patches/patchmanager/issues/220#issuecomment-1013606466, specifically section II.2. there.

[Olf0]

See also subsequent PRs #237, (#238) / #239, #241 (already mentioned above) and #242.