[pacman-dev] [PATCH] makepkg: improve removal of installed dependencies
Compare a list of packages on the system before and after dependency resolution in order to get a complete list of packages to remove. This allows makepkg to remove packages installed due to provides. Bail in cases where packages that were on the system originally have been removed as there is a risk of breaking the system when removing the new packages Fixes FS#15144 Signed-off-by: Allan McRae <allan@archlinux.org> --- This moves the generation of the installed package list to directly after the installation of deps to prevent issues if the user installs packages while the build is occurring: http://bugs.archlinux.org/task/15144#comment53938 Rebased on Cedric's run_pacman and $PACMAN patches. doc/makepkg.8.txt | 6 +++--- scripts/makepkg.sh.in | 39 +++++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt index ad27bca..168b369 100644 --- a/doc/makepkg.8.txt +++ b/doc/makepkg.8.txt @@ -166,9 +166,9 @@ Environment Variables --------------------- *PACMAN*:: The command that will be used to check for missing dependencies and to - install and remove packages. Pacman's -U, -T, -S and -Rns operations - must be supported by this command. If the variable is not set or - empty, makepkg will fall back to `pacman'. + install and remove packages. Pacman's -Qq, -Rns, -S, -T, and -U + operations must be supported by this command. If the variable is not + set or empty, makepkg will fall back to `pacman'. Additional Features diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index c730150..5743ea4 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -27,8 +27,10 @@ # makepkg uses quite a few external programs during its execution. You # need to have at least the following installed for makepkg to function: -# bsdtar (libarchive), bzip2, coreutils, fakeroot, find (findutils), -# getopt (util-linux), gettext, grep, gzip, openssl, sed, tput (ncurses) +# bsdtar (libarchive), bzip2, coreutils, diff (diffutils), fakeroot, +# find (findutils), getopt (util-linux), gettext, grep, gzip, openssl, +# sed, tput (ncurses) + # gettext initialization export TEXTDOMAIN='pacman' @@ -343,7 +345,7 @@ download_file() { run_pacman() { local ret=0 - if (( ! ASROOT )) && [[ $1 != "-T" ]] && sudo -l $PACMAN &>/dev/null; then + if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]] && sudo -l $PACMAN &>/dev/null; then sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? else $PACMAN $PACMAN_OPTS "$@" || ret=$? @@ -398,7 +400,6 @@ handle_deps() { } resolve_deps() { - # $pkgdeps is a GLOBAL variable, used by remove_deps() local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1 @@ -408,7 +409,6 @@ resolve_deps() { fi if handle_deps $deplist; then - pkgdeps="$pkgdeps $deplist" # check deps again to make sure they were resolved deplist="$(check_deps $*)" [[ -z $deplist ]] && return $R_DEPS_SATISFIED @@ -425,23 +425,25 @@ resolve_deps() { return $R_DEPS_MISSING } -# fix flyspray bug #5923 remove_deps() { - # $pkgdeps is a GLOBAL variable, set by resolve_deps() (( ! RMDEPS )) && return - [[ -z $pkgdeps ]] && return - local dep depstrip deplist - deplist="" - for dep in $pkgdeps; do - depstrip="${dep%%[<=>]*}" - deplist="$deplist $depstrip" - done + # check for packages removed during dependency install (e.g. due to conflicts) + # removing all installed packages is risky in this case + if (( $(diff <(printf "%s\n" "${original_pkglist[@]}") \ + <(printf "%s\n" "${current_pkglist[@]}") | grep "<" | wc -l) > 0 )); then + warning "$(gettext "Failed to remove installed dependencies.")" + return 0 + fi - msg "Removing installed dependencies..." + local deplist=($(diff <(printf "%s\n" "${original_pkglist[@]}") \ + <(printf "%s\n" "${current_pkglist[@]}") | grep ">")) + deplist=(${deplist[@]#>}) + [ ${#deplist[@]} -eq 0 ] && return + msg "Removing installed dependencies..." # exit cleanly on failure to remove deps as package has been built successfully - if ! run_pacman -Rns $deplist; then + if ! run_pacman -Rn ${deplist[@]}; then warning "$(gettext "Failed to remove installed dependencies.")" return 0 fi @@ -1874,14 +1876,13 @@ if (( SOURCEONLY )); then exit 0 fi -# fix flyspray bug #5973 if (( NODEPS || NOBUILD || REPKG )); then # no warning message needed for nobuild, repkg if (( NODEPS )); then warning "$(gettext "Skipping dependency checks.")" fi elif [ $(type -p "${PACMAN%% *}") ]; then - unset pkgdeps # Set by resolve_deps() and used by remove_deps() + original_pkglist=($(run_pacman -Qq)) # required by remove_deps deperr=0 msg "$(gettext "Checking Runtime Dependencies...")" @@ -1890,6 +1891,8 @@ elif [ $(type -p "${PACMAN%% *}") ]; then msg "$(gettext "Checking Buildtime Dependencies...")" resolve_deps ${makedepends[@]} || deperr=1 + current_pkglist=($(run_pacman -Qq)) # required by remove_deps + if (( deperr )); then error "$(gettext "Could not resolve all dependencies.")" exit 1 -- 1.6.5.3
On 12/03/2009 03:33 PM, Allan McRae wrote:
Compare a list of packages on the system before and after dependency resolution in order to get a complete list of packages to remove. This allows makepkg to remove packages installed due to provides.
Bail in cases where packages that were on the system originally have been removed as there is a risk of breaking the system when removing the new packages
Fixes FS#15144
Signed-off-by: Allan McRae <allan@archlinux.org> ---
I have not tested the patch yet, so just some comments on the code for now.
This moves the generation of the installed package list to directly after the installation of deps to prevent issues if the user installs packages while the build is occurring: http://bugs.archlinux.org/task/15144#comment53938
Rebased on Cedric's run_pacman and $PACMAN patches.
doc/makepkg.8.txt | 6 +++--- scripts/makepkg.sh.in | 39 +++++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt index ad27bca..168b369 100644 --- a/doc/makepkg.8.txt +++ b/doc/makepkg.8.txt @@ -166,9 +166,9 @@ Environment Variables --------------------- *PACMAN*:: The command that will be used to check for missing dependencies and to - install and remove packages. Pacman's -U, -T, -S and -Rns operations - must be supported by this command. If the variable is not set or - empty, makepkg will fall back to `pacman'. + install and remove packages. Pacman's -Qq, -Rns, -S, -T, and -U + operations must be supported by this command. If the variable is not + set or empty, makepkg will fall back to `pacman'.
Additional Features diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index c730150..5743ea4 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -27,8 +27,10 @@
# makepkg uses quite a few external programs during its execution. You # need to have at least the following installed for makepkg to function: -# bsdtar (libarchive), bzip2, coreutils, fakeroot, find (findutils), -# getopt (util-linux), gettext, grep, gzip, openssl, sed, tput (ncurses) +# bsdtar (libarchive), bzip2, coreutils, diff (diffutils), fakeroot, +# find (findutils), getopt (util-linux), gettext, grep, gzip, openssl, +# sed, tput (ncurses) +
# gettext initialization export TEXTDOMAIN='pacman' @@ -343,7 +345,7 @@ download_file() {
run_pacman() { local ret=0 - if (( ! ASROOT )) && [[ $1 != "-T" ]] && sudo -l $PACMAN &>/dev/null; then + if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]] && sudo -l $PACMAN &>/dev/null; then sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? else $PACMAN $PACMAN_OPTS "$@" || ret=$? @@ -398,7 +400,6 @@ handle_deps() { }
resolve_deps() { - # $pkgdeps is a GLOBAL variable, used by remove_deps() local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
@@ -408,7 +409,6 @@ resolve_deps() { fi
if handle_deps $deplist; then - pkgdeps="$pkgdeps $deplist" # check deps again to make sure they were resolved deplist="$(check_deps $*)" [[ -z $deplist ]] && return $R_DEPS_SATISFIED @@ -425,23 +425,25 @@ resolve_deps() { return $R_DEPS_MISSING }
-# fix flyspray bug #5923 remove_deps() { - # $pkgdeps is a GLOBAL variable, set by resolve_deps() (( ! RMDEPS )) && return - [[ -z $pkgdeps ]] && return
- local dep depstrip deplist - deplist="" - for dep in $pkgdeps; do - depstrip="${dep%%[<=>]*}" - deplist="$deplist $depstrip" - done + # check for packages removed during dependency install (e.g. due to conflicts) + # removing all installed packages is risky in this case + if (( $(diff <(printf "%s\n" "${original_pkglist[@]}") \ + <(printf "%s\n" "${current_pkglist[@]}") | grep "<" | wc -l) > 0 )); then + warning "$(gettext "Failed to remove installed dependencies.")" + return 0 + fi
You could use comm here. The advantage would be that it do not pull in a new dependency as it is in coreutils too and you do not need grep to get the unique lines. if [[ -n $(printf "%s\n" ${original_pkglist[@]} \ | comm -23 - <(printf "%s\n" ${current_pkglist[@]})) ]] Note the code above is untested.
- msg "Removing installed dependencies..." + local deplist=($(diff <(printf "%s\n" "${original_pkglist[@]}") \ + <(printf "%s\n" "${current_pkglist[@]}") | grep ">")) + deplist=(${deplist[@]#>})
local deplist=($(printf "%s\n" ${original_pkglist[@]} \ | comm -13 - <(printf "%s\n" ${current_pkglist[@]})))
+ [ ${#deplist[@]} -eq 0 ] && return
(( ${#deplist[@]} == 0 )) :)
+ msg "Removing installed dependencies..." # exit cleanly on failure to remove deps as package has been built successfully - if ! run_pacman -Rns $deplist; then + if ! run_pacman -Rn ${deplist[@]}; then warning "$(gettext "Failed to remove installed dependencies.")" return 0 fi @@ -1874,14 +1876,13 @@ if (( SOURCEONLY )); then exit 0 fi
-# fix flyspray bug #5973 if (( NODEPS || NOBUILD || REPKG )); then # no warning message needed for nobuild, repkg if (( NODEPS )); then warning "$(gettext "Skipping dependency checks.")" fi elif [ $(type -p "${PACMAN%% *}") ]; then - unset pkgdeps # Set by resolve_deps() and used by remove_deps() + original_pkglist=($(run_pacman -Qq)) # required by remove_deps
Could be skipped if RMDEPS == 0 but I do not know how fast / slow this pacman operation actually is.
deperr=0
msg "$(gettext "Checking Runtime Dependencies...")" @@ -1890,6 +1891,8 @@ elif [ $(type -p "${PACMAN%% *}") ]; then msg "$(gettext "Checking Buildtime Dependencies...")" resolve_deps ${makedepends[@]} || deperr=1
+ current_pkglist=($(run_pacman -Qq)) # required by remove_deps +
Same as above.
if (( deperr )); then error "$(gettext "Could not resolve all dependencies.")" exit 1
Cedric Staniewski wrote:
On 12/03/2009 03:33 PM, Allan McRae wrote:
Compare a list of packages on the system before and after dependency resolution in order to get a complete list of packages to remove. This allows makepkg to remove packages installed due to provides.
Bail in cases where packages that were on the system originally have been removed as there is a risk of breaking the system when removing the new packages
Fixes FS#15144
Signed-off-by: Allan McRae <allan@archlinux.org> ---
I have not tested the patch yet, so just some comments on the code for now.
Thanks for the comments. I have pushed the updated patch on my working branch: http://projects.archlinux.org/users/allan/pacman.git/commit/?h=working&id=ffc7b0dc <snip>
You could use comm here. The advantage would be that it do not pull in a new dependency as it is in coreutils too and you do not need grep to get the unique lines.
if [[ -n $(printf "%s\n" ${original_pkglist[@]} \ | comm -23 - <(printf "%s\n" ${current_pkglist[@]})) ]]
I went for: if [[ -n $(comm -23 <(printf "%s\n" "${original_pkglist[@]}") \ <(printf "%s\n" "${current_pkglist[@]}")) ]]; then The only slight annoyance was when using comm the output of pacman -Qq needed sorted as pacman outputs in a very slightly different alphabetical order to what comm expects and "--nocheck-order" is not portable to BSD/OSX. Allan
On 12/10/2009 06:40 AM, Allan McRae wrote:
Cedric Staniewski wrote:
On 12/03/2009 03:33 PM, Allan McRae wrote:
Compare a list of packages on the system before and after dependency resolution in order to get a complete list of packages to remove. This allows makepkg to remove packages installed due to provides.
Bail in cases where packages that were on the system originally have been removed as there is a risk of breaking the system when removing the new packages
Fixes FS#15144
Signed-off-by: Allan McRae <allan@archlinux.org> ---
I have not tested the patch yet, so just some comments on the code for now.
Thanks for the comments. I have pushed the updated patch on my working branch: http://projects.archlinux.org/users/allan/pacman.git/commit/?h=working&id=ffc7b0dc
I tested your latest patch for the last few days and did not encountered an issue so far. Looks good.
You could use comm here. The advantage would be that it do not pull in a new dependency as it is in coreutils too and you do not need grep to get the unique lines.
if [[ -n $(printf "%s\n" ${original_pkglist[@]} \ | comm -23 - <(printf "%s\n" ${current_pkglist[@]})) ]]
I went for: if [[ -n $(comm -23 <(printf "%s\n" "${original_pkglist[@]}") \ <(printf "%s\n" "${current_pkglist[@]}")) ]]; then
The only slight annoyance was when using comm the output of pacman -Qq needed sorted as pacman outputs in a very slightly different alphabetical order to what comm expects and "--nocheck-order" is not portable to BSD/OSX.
Oh, sorry. I thought the output of pacman -Qq is already sorted in the correct way. Good catch.
participants (2)
-
Allan McRae
-
Cedric Staniewski