lib/transaction_files.c: allow replacing file with alternative

[Chocimier]

Fix alternative-file clobber by adding alternatives to obsolete files processing. More test cases, like removing one alternative provider when other is installed and not in transaction, are already written, so not adding duplicates.

Other commit is not related to topic, just found it breaking compilation with -Og.

[Duncaen]

This seems to fix the broken test cases, but with comments like "Leave removing alternative to dedicated phase." I think the test cases are not really correct, i.e. still depend on order of and/or that an alternative wasn't replaced with a non alternative symlink like it is the case with the gcc, clang to cc symlink replacing.

I think for the future it would be good to open an RFC issue to get input before writing code, because I've also worked on this and my conclusion is that to actually fix all edge cases we have to know what alternatives are set, switched to and removed before starting the transaction.
This imho should also include a rewrite of the alternatives in general to make it more easy to incorporate something like this into the files checks.

[Chocimier]

I believe this is as reliable as checking of current clobber, like file-directory, with order awareness. I couldn't come up with transaction sequence that causes wrong result and is not limitation of transaction_files too, like overwriting file of package that is not in transaction. Can you?

By "Leave removing alternative to dedicated phase" I mean if there is nothing to install instead of removed alternative, let xbps_alternatives_unregister do the job, because there may be other providers installed.

Probably I should open issue first but I feel they do not work well either, there are some of them with no conclusions for years.

[Duncaen]

[*] Unpacking packages
gcc-1.1_1: updating to 1.1_2 ...
gcc-1.1_2: unpacking ...
gcc-1.1_2: removed obsolete entry: ./usr/bin/cc
gcc-1.1_2: unpacked file `./usr/bin/cc' (0 bytes)
gcc-1.1_2: unregistered 'cc' alternatives group
clang-1.1_1: updating to 1.1_2 ...
Removing 'cc' alternatives group symlink: cc
clang-1.1_2: unpacking ...
clang-1.1_2: unregistered 'cc' alternatives group

This is one of the ways this doesn't solve the problem, I really think to solve all edge cases a rewrite of the alternatives is necessary.

From 16d00405bf05cde3a44ac9c1e8bb65f978eac7dc Mon Sep 17 00:00:00 2001
From: Duncan Overbruck <[email protected]>
Date: Tue, 24 Mar 2020 20:54:00 +0100
Subject: [PATCH] tests: add test case for cc alternatives removal

---
 tests/xbps/xbps-alternatives/main_test.sh | 44 ++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/tests/xbps/xbps-alternatives/main_test.sh b/tests/xbps/xbps-alternatives/main_test.sh
index 6f11b906..f5e8e4c9 100644
--- a/tests/xbps/xbps-alternatives/main_test.sh
+++ b/tests/xbps/xbps-alternatives/main_test.sh
@@ -891,12 +891,53 @@ replace_file_with_alternative_body() {
 	xbps-install -r root --repository=repo -ydv -Su
 	atf_check_equal $? 0
 
-	test -L root/usr/bin/file
+	test -h root/usr/bin/file
 	atf_check_equal $? 0
 	lnk=$(readlink -f root/usr/bin/file)
 	atf_check_equal $lnk "$PWD/root/usr/bin/pkg-a-file"
 }
 
+atf_test_case cc_alternatives_removal
+
+cc_alternatives_removal_head() {
+	atf_set "descr" "xbps-alternatives: removal of the cc alternatives group"
+}
+cc_alternatives_removal_body() {
+	mkdir -p repo pkg_A/usr/bin
+	mkdir -p repo pkg_B/usr/bin
+	touch pkg_A/usr/bin/gcc
+	touch pkg_B/usr/bin/clang
+
+	cd repo
+	xbps-create -A noarch -n gcc-1.1_1 -s "gcc pkg" --alternatives "cc:cc:/usr/bin/gcc" ../pkg_A
+	atf_check_equal $? 0
+	xbps-create -A noarch -n clang-1.1_1 -s "clang pkg" --alternatives "cc:cc:/usr/bin/clang" ../pkg_B
+	atf_check_equal $? 0
+	xbps-rindex -d -a $PWD/*.xbps
+	atf_check_equal $? 0
+	cd ..
+
+	xbps-install -r root --repository=repo -ydv clang gcc
+	atf_check_equal $? 0
+
+	ln -s gcc pkg_A/usr/bin/cc
+	cd repo
+	rm -f *.xbps
+	xbps-create -A noarch -n gcc-1.1_2 -s "gcc pkg" ../pkg_A
+	atf_check_equal $? 0
+	xbps-create -A noarch -n clang-1.1_2 -s "clang pkg" ../pkg_B
+	atf_check_equal $? 0
+	xbps-rindex -d -a $PWD/*.xbps
+	atf_check_equal $? 0
+	cd ..
+
+	xbps-install -r root --repository=repo -dvyu gcc clang
+	atf_check_equal $? 0
+
+	test -h root/usr/bin/cc
+	atf_check_equal $? 0
+}
+
 atf_init_test_cases() {
 	atf_add_test_case register_one
 	atf_add_test_case register_one_dangling
@@ -919,4 +960,5 @@ atf_init_test_cases() {
 	atf_add_test_case replace_alternative_with_symlink
 	atf_add_test_case keep_provider_on_update
 	atf_add_test_case replace_file_with_alternative
+	atf_add_test_case cc_alternatives_removal
 }
-- 
2.25.2

Probably I should open issue first but I feel they do not work well either, there are some of them with no conclusions for years.

Even if there are some without resolution, I could have given my thoughts on this so we don't end up writing the same things or come up with incomplete fixes that work around the current test cases.

[Duncaen]

I believe this is as reliable as checking of current clobber, like file-directory, with order awareness.

This is the main problem, file conflicts are completely independent of order and can be detected anyways.
The obsolete file removal stuff, i.e. switching from a directory to a file or a symlink to some other type fully depends on install order, you only have to remove the given obsolete file before unpacking the package that the new file contains.
Same with switching files from one package to another, in this case we can remove the obsolete file when the first package in the transaction is unpackaged, the old owner package can then through the obsoletes files collection remove only files that haven't been moved between packages.

Alternatives are different in this regard, the alternatives code is broken by itself and IMHO needs to be rewritten and properly designed to fix this issue.
The only time the order inside the transaction matters for alternatives is when they are being installed and no alternatives provider for a given group exists.
But inside the transactions, groups can switch around, to the next entry in the alternatives group.
The group entries are ordered by install date or when switching alternatives then one of the items is pushed to the top.
So the alternatives code that removes symlinks is conflicting with the normal files, the solution would be something like the obsoletes array which is aware of the switches before the transaction starts and then delegates the removal to the right package and avoids removing old alternative links that have been already replaced by a previous update in the transaction.

[Chocimier]

Ok, cc example indeed shows there will be always unexpected case.

Your proposed solution is better, but is still complicated.

What about following process?

1. Save alternatives state in memory and remove alternative symlinks from filesystem. (Here target not matching selected alternative group may or may not be respected.)
2. Collect alternatives and files but alternatives information is used only to discover conflicting paths, not to manage them.
3. Unpack, with registering alternatives but without creating them on filesystem.
4. Create alternatives on filesystem.

[Duncaen]

I was also thinking about simplifying this, but things like hooks that potentially use alternatives like awk and the old cc alternative make this complicated.

I think the only complete solution would be something that takes the transaction and creates an "alternatives changeset" which can be fed into transaction_files.c for conflict checks but I think they need to be handled separate from obsolete files removal.
We would still have to be able to switch the alternative to the next one when the current provider is removed, not when the new provider is being unpackaged, except that we can't switch to a new providers view before that package has been unpackaged, we would have to switch to the currently installed one as its currently done.
Thinking more and more about this I feel like my idea is already contradicting itself and there are other edge cases, I'm not taking into account or really know how to handle well at the moment, I think I have to think about this a bit more.

As example pkg A is removed and switches to the next next alternative group from pkg B, then B is removed and switches to the next alternative C which then is also removed.
Should the new algorithm skip B and switch to C when A is uninstalled.
Then there are variations to this like C is actually not installed and is being installed with the transaction, we can't switch to C when A or B are removed because its not unpackaged at the time A or B are being removed,
should we then switch from A to B or completely remove the alternative from when A is uninstalled and recreate it when C is being unpacked?

[Duncaen]

I think we have to come up with a spec/design for alternatives in general to actually start solving this problem.

[Chocimier]

hooks

Pre-install hook run during extract means we also need to reorder transaction to avoid: uninstall gawk - use awk - install nawk.

This is going too far. Maybe solution is to remove all alternatives for time of unpacking to guarantee fail of using awk and not to care about order.

[Duncaen]

I think hooks are fine, but I would like to reduce the need for them in the long term.
Currently INSTALL with "pre" runs before the package is unpackaged, but obsolete files are already being removed, which is 100% a bug.
"pre" should run with the existing software and alternatives,
"post" should run with the new software and alternatives.

[Duncaen]

Pre-install hook run during extract means we also need to reorder transaction to avoid: uninstall gawk - use awk - install nawk.

Oh yea I see, there could be INSTALL scripts from unrelated packages where the alternative is theoretically installed but not available.

The transaction order tbh is something I was thinking about a bit too, I reverted some code I wrote that recursively adds revdeps if there are shlib issues but I wasn't really sure if the order is important.

The readline revbump and the resulting update of bash is a good example for something that we really can not solve and makes hooks really fragile.
For users that had bash as sh alternative, either readline or bash was updated first, therefor breaking the INSTALL scripts.

Edit:
I think the best solution to this would be to run all INSTALL scripts with "pre", before starting to unpack anything,
then after everything is unpacked, run all INSTALL scripts with "post".
And I guess reducing the necessity for INSTALL by supporting something like make_dirs and maybe chown/chmod stuff by xbps itself instead of INSTALL scripts.

[Chocimier]

I think the best solution to this would be to run all INSTALL scripts with "pre", before starting to unpack anything,
then after everything is unpacked, run all INSTALL scripts with "post".
And I guess reducing the necessity for INSTALL by supporting something like make_dirs and maybe chown/chmod stuff by xbps itself instead of INSTALL scripts.

Sounds good.
Pre-install are rare both in packages (base-files, gimp, os-prober), and xbps-triggers (kernel-hooks (not used?)), so there is not a lot to verify.

[Chocimier]

I was also thinking about simplifying this, but things like hooks that potentially use alternatives like awk and the old cc alternative make this complicated.

With pre-install hooks run before unpacking, assuring that there is some provider of alternative in middle of transaction seems to not be a problem. Are there any more concerns with this simplified approach?

[Duncaen]

It would simplifies it so far that we can compute the end result of all alternative chanegs before starting the transaction and feed the changes into transaction_files.c.

I have a WIP branch that moves the hooks at the beginning of the transaction, but because I still need to solve some issues and optimize it to avoid having to open each archive twice at the beginning of the transaction, once for INSTALL/REMOVE scripts, once for files.plist.

Not sure yet about INSTALL.msg, I guess I'll keep the where it is now at unpack time, but maybe we could move them at the end of the transaction so users might actually read them.

[Chocimier]

INSTALL.msg is already stored in index.plist, so it can be even printed before transaction, together with list of installed packages.

[ericonr]

The part about moving hooks has been implemented and merged already, right?