sailfishos-patches / patchmanager

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

Fix disabling of mangling? #218

Closed Olf0 closed 2 years ago

Olf0 commented 2 years ago

While reviewing the shell script pm_apply, specifically the new functionality of mangling library paths, I wondered how the disabling of the whole mangling is supposed to work.

a. In line 27 the variable DISABLE_MANGLING is evaluated for being zero length, but never set (at any place I am aware of); hence I assume it is meant to be set as an environment variable. Q: Is this documented somewhere?

b. If DISABLE_MANGLING contains something or sourcing the file /etc/patchmanager/manglelist.conf does not set the variable MANGLE_CANDIDATES to something, this variable stays empty (i.e., initialised, but zero length). But then the for loop in the function mangle_libpath is entered once with an empty string for p, consequently totl_lines = cand_lines = n (with n ≥ 1), … IMO this cannot yield anything right.

Hence I suggest to prefix the call of the function mangle_libpath (in the script body) with [ -n "$MANGLE_CANDIDATES" ] && per MR #219.

nephros commented 2 years ago

Thanks or review, analysis and fixing MR, merged.

You are right, that variable was supposed to be an env var, or be set for either testing of whatever reason a user may have to turn this off. (It may have been added before there was the UI option to disable that too...)

Which leaves open the nonexisting documentation - what would be the best place to put that? As far as I'm aware we don't have an "innards of PM" section of documentation at the moment.

Olf0 commented 2 years ago

Which leaves open the non-existing documentation - what would be the best place to put that?

The PM-wiki here at GH.

As far as I'm aware we don't have an "innards of PM" section of documentation at the moment.

~~Correct. New sections are easy to create, many of the extant ones are just stubs, others contain some collected information which needs to be overhauled. Thus creating proper content is the hard but crucial part. Feel free to draft a new section; I may try as well but have enough open tasks, that this will be mid-January, earliest. At least I now know what to document; thanks for that information.~~ → I did not look properly, see next message.

Olf0 commented 2 years ago

But we do already have a section for environment variables, see https://github.com/sailfishos-patches/patchmanager/wiki/Environment-variables IMO it fits well there.

nephros commented 2 years ago

Documented.

BTW, it is actually used in https://github.com/sailfishos-patches/patchmanager/blob/master/src/bin/patchmanager-daemon/patchmanagerobject.cpp#L1822 to pass the UI configuration to disable bitness mangling to the script.