Skip to content

Conversation

@nephros
Copy link
Contributor

@nephros nephros commented Oct 15, 2021

Fixes #71
#71

@nephros nephros marked this pull request as draft October 15, 2021 18:17
@nephros
Copy link
Contributor Author

nephros commented Oct 15, 2021

PoC type implementation - it does work and produces patches that apply.

Mode of operation could be improved - currently it does a lot of looping and grepping the same files. Could probably be done with one or two bigger sed commands all at once.

Testing required:

  • produces correct patch from a 32bit source on a 64bit system
  • produces correct patch from a 32bit source on a "simulated" 64bit system (hack the script to detect 64bit)
  • produces correct patch for a 64bit source on a 32bit system
  • when a converted patch is unapplied: Are the correct files deleted?
  • when a converted patch is uninstalled: Are the correct files deleted?
  • does it work reliably with patches that contain both lib64 and lib paths (e.g. patches a Silica file and a systemd service on 64bit)?
  • does it correctly detect patches to be left alone, i.e. do not need conversion?

Oh, and the candidate list currently contains "/usr/lib/patchmanager-test" for easy testing. This should be removed before merging.

@b100dian
Copy link
Contributor

I tested this on a 64-bit device and the reason it was failing was that /tmp/patchmanager did not had the copies to patch. I've added POC support for mangling those cache files.
I've checked the "produces correct patch from a 32bit source on a 64bit system"

@nephros nephros added this to the 3.1.1 milestone Oct 30, 2021
@nephros
Copy link
Contributor Author

nephros commented Oct 31, 2021

As discussed, we don't want to duplicate the mangle list in both cpp source and shell script.
So introduce a config file to hold that information, and read it from both.

@nephros
Copy link
Contributor Author

nephros commented Oct 31, 2021

@b100dian please review, seems to work but may be bad c++.

@b100dian
Copy link
Contributor

Nothing wrong per se in the changes (just nits) but, however I tried playing around with 1. having only one MANGLED_CANDIDATES entry and 2. see if pm_unapply would work and even 3. If a setting could show the paths to the user (not clear how to pass the setting to pm_apply now, maybe $2) in my fork - but currently I get errors applying the Return old pulley menu 4.x that used to work. So I need to figure out if any of (my|your) changes did this last since I tested this. That's for tomorrow..

@b100dian
Copy link
Contributor

b100dian commented Nov 1, 2021

Ok @nephros managed to pinpoint it to toStringList not splitting the QVariant value - I have made a PR against this PR for the changes are somewhat numerous. Let me know if you can review that and merge --squash it. There is one caveat, see #140

@b100dian b100dian marked this pull request as ready for review November 2, 2021 20:44
Copy link
Contributor

@b100dian b100dian left a comment

Choose a reason for hiding this comment

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

All good from my point of view, maybe it needs more testing. Oh, and the wording in the settings page maybe needs review.

I've added a last commit refreshing patches on bitness mangling change and fixed the 'last' caveat.

@b100dian
Copy link
Contributor

b100dian commented Nov 2, 2021

Hmm maybe I spoke too soon, I still get errors like "PatchManagerObject::tryToUnlinkFakeParent" error while trying to cd from "/usr/lib" to: "qt5" on dynamic switch

@b100dian
Copy link
Contributor

b100dian commented Nov 3, 2021

Ok, just tested this build with 4.3.0 armv7hl and the setting does not harm existing 32-bit patches, and it tried to fix this 64-bit patch https://coderus.openrepos.net/pm2/project/patch-five-cameras-tucana which didn't apply as it shouldn't, but for the right reasons (the /etc/dconf/db/vendor.d/jolla-camera-hw.txt file was not having the expected contents)

Other than this, with the last change (re-order to mangle before test if applied) it seemed to work correctly even at pm3 upgrade scripts on my 4.2.0 64-bit device.

@nephros nephros modified the milestones: 3.1.1, 3.2.x Nov 5, 2021
@Olf0 Olf0 added the enhancement this improves something label Nov 6, 2021
@nephros nephros added backends daemon, systemd and dbus components patching related to activating, deactivating and handling Patches labels Nov 10, 2021
@b100dian
Copy link
Contributor

Zig-zagging through my notifications, I need to test the last two checkboxes. Could find some time tomorrow.

@b100dian
Copy link
Contributor

Tested by "does it work reliably with patches that contain both lib64 and lib paths (e.g. patches a Silica file and a systemd service on 64bit)?" appending this to an existing patch

--- /usr/lib/patchmanager-test/test.txt
+++ /usr/lib/patchmanager-test/test.txt
@@ -1 +1,2 @@
-# Test line
+# test
+# replaced line
--- /usr/lib/test.txt
+++ /usr/lib/test.txt
@@ -1 +1,2 @@
-# Nothing
+# test
+# replaced line

To an existing patch, looks alright (it patched one file in lib64 and another one in lib)

@b100dian b100dian merged commit 9431443 into master Nov 13, 2021
@b100dian b100dian deleted the patch-bitness branch November 13, 2021 16:25
nephros added a commit to nephros/patchmanager that referenced this pull request Feb 22, 2022
a build artefact was introduced in 9431443 / sailfishos-patches#109
nephros added a commit that referenced this pull request Feb 23, 2022
a build artefact was introduced in 9431443 / #109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backends daemon, systemd and dbus components enhancement this improves something patching related to activating, deactivating and handling Patches

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Patches with targets in /usr/lib[,64] do not apply on 32/64bit systems

4 participants