It is important to perform sanity checks on any and all symlinks created in ${D}. It's possible that a "make install" or a similar script might create invalid symlinks, especially when building .so libraries. I believe this was originally suggested by Vitaly, but somehow it got lost in the ethers :).
um, what constitutes an invalid symlink? What specifically should we be checking for?
we now merge symlinks in the proper order to avoid brokenness; but we *do* allow the merging of broken symlinks and this is needed for some packages and is ok.
After zac fixed one of my ebuilds I was wondering why there is no qa message if a package installs a broken symlink. Should we rethink this?
There's no guarantee that they'll resolve correctly until they're installed, so the check would probably have to be delayed until after the package is installed. Also, once they're installed, if $ROOT != / then they'll only resolve correctly if they're written as relative symlinks like ../../usr/lib/foo or whatnot. We might want to add something to PMS to require that all symlinks are relative rather than absolute.
If we assume that all symlinks resolve to a target that's installed by the package itself, then we can implement a sanity check that performs simulated symlink resolution for absolute symlinks inside $D. Maybe this assumption is good enough to avoid false positives in the vast majority of cases. It's come to my attention that python.eclass has a working implementation of a check like this inside the python_merge_intermediate_installation_images() function, where it resolves the simulated target of a symlink and stores it in the ${absolute_file} variable.
(In reply to comment #5) > If we assume that all symlinks resolve to a target that's installed by the > package itself We can also simulate resolution in $ROOT, so we don't need to make this assumption. That should eliminate nearly all false positives. I'll remove pms-bugs from CC since it appears that it's not really necessary to involve PMS.
(In reply to comment #5) > If we assume that all symlinks resolve to a target that's installed by the > package itself, then we can implement a sanity check that performs simulated > symlink resolution for absolute symlinks inside $D. Maybe this assumption is > good enough to avoid false positives in the vast majority of cases. That's sound good to me. There are only very few packages which create a symlink to something outside it's own files. And there should be something like QA_BROKEN_SYMLINK to skip the check for some files.
Created attachment 342118 [details] portage_symlink_qa_warning.patch Proposed patch for this old bug that we have to resolve one or another day (because it's a good QA idea), I'm new to Portage development so I am not sure if this is the right approach but it would serve as a start; just checking if it is on root or in the destination directory should be sufficient, I don't know of any special cases and I assume there are none unless you tell me otherwise. It took me some time to find where to insert this and I have done it here because it seemed the most easy approach to me (but not necessarily the right one), if you want this to happen before or after the actual merge then please point me to where in the code this happens; the code structure is still quite unclear to me (and the documentation isn't helping me). In this patch this is a warning such that developers are at least aware of this. We should however look at whether there is a valid case for a symlink to be invalid, otherwise it would be better to turn this into an error. As long as there are no valid cases for which it makes sense, I don't see a need for a QA variable. Something to keep in mind to the future might be that we need to refactor the code such that we queue every file to be installed through some "checking" functions; maybe that happens already at the moment, but if that's the case then I simply saw no such thing in the limited search time I spent on it. But I'm just saying that having all these special conditions, checks, messages, warnings, errors and other magic in the code as it is now; makes it hard to maintain in the long run.
(In reply to comment #8) > Created attachment 342118 [details] > portage_symlink_qa_warning.patch > > Proposed patch for this old bug that we have to resolve one or another day > (because it's a good QA idea), I'm new to Portage development so I am not > sure if this is the right approach but it would serve as a start; just > checking if it is on root or in the destination directory should be > sufficient, I don't know of any special cases and I assume there are none > unless you tell me otherwise. I think that should work correctly. > It took me some time to find where to insert this and I have done it here > because it seemed the most easy approach to me (but not necessarily the > right one), if you want this to happen before or after the actual merge then > please point me to where in the code this happens; the code structure is > still quite unclear to me (and the documentation isn't helping me). That place should be fine for now. If we decide to make it an error, then we should do it before the merging starts, like the collision-protect code. > In this patch this is a warning such that developers are at least aware of > this. We should however look at whether there is a valid case for a symlink > to be invalid, otherwise it would be better to turn this into an error. As > long as there are no valid cases for which it makes sense, I don't see a > need for a QA variable. Yeah, we can do that after we've tested the warning for awhile. > Something to keep in mind to the future might be that we need to refactor > the code such that we queue every file to be installed through some > "checking" functions; maybe that happens already at the moment, but if > that's the case then I simply saw no such thing in the limited search time I > spent on it. But I'm just saying that having all these special conditions, > checks, messages, warnings, errors and other magic in the code as it is now; > makes it hard to maintain in the long run. Yeah, that's a good idea. We don't have anything like that yet.
Your patch is in git: http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=ca7467006b112f65a57c4db7816729841a2de694 I've modified it to use lexists, sine if the target happens to be a broken symlink then that should trigger an independent warning: http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=8cfe2dc28e02a9cdd739a83db4ed871a31468114
This is fixed in 2.1.11.56 and 2.2.0_alpha167.
