Re: [pacman-dev] [PATCH 10/14] Warn on optdep removal

On Wed, Nov 23, 2011 at 9:51 AM, Benedikt Morbach wrote:

Issue a warning if the user for whatever reason tries to remove an optdepend of an installed package.

Signed-off-by: Benedikt Morbach ---  lib/libalpm/alpm.h   |    5 ++++-  lib/libalpm/deps.c   |   35 ++++++++++++++++++++++++++---------  lib/libalpm/error.c  |    2 ++  lib/libalpm/remove.c |   16 +++++++++++++---  lib/libalpm/sync.c   |    2 +-  lib/libalpm/trans.c  |   10 ++++++++--  src/pacman/remove.c  |   23 +++++++++++++++++++----  src/util/testdb.c    |    2 +-  8 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index d06ad0d..34dbaa1 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -162,6 +162,8 @@ typedef struct _alpm_depmissing_t {        alpm_depend_t *depend;        /* this is used in case of remove dependency error only */        char *causingpkg; +       /* this is used for optdepends only */ +       char *description; This is needed only because depmissing_t points at a depend_t and not an optdepend_t? Yet another reason to make them one and the same.

 } alpm_depmissing_t;

 /** Conflict */ @@ -1096,7 +1098,7 @@ int alpm_remove_pkg(alpm_handle_t *handle, alpm_pkg_t *pkg);  */

 alpm_list_t *alpm_checkdeps(alpm_handle_t *handle, alpm_list_t *pkglist, -               alpm_list_t *remove, alpm_list_t *upgrade, int reversedeps); +               alpm_list_t *remove, alpm_list_t *upgrade, int reversedeps, int consider_optdeps);  alpm_pkg_t *alpm_find_satisfier(alpm_list_t *pkgs, const char *depstring);  alpm_pkg_t *alpm_find_dbs_satisfier(alpm_handle_t *handle,                alpm_list_t *dbs, const char *depstring); @@ -1184,6 +1186,7 @@ typedef enum _alpm_errno_t {        ALPM_ERR_DLT_PATCHFAILED,        /* Dependencies */        ALPM_ERR_UNSATISFIED_DEPS, +       ALPM_ERR_UNSATISFIED_OPTDEPS,        ALPM_ERR_CONFLICTING_DEPS,        ALPM_ERR_FILE_CONFLICTS,        /* Misc */ diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index 83bc619..c9efce4 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -52,7 +52,7 @@ void _alpm_optdep_free(alpm_optdepend_t *optdep)  }

 static alpm_depmissing_t *depmiss_new(const char *target, alpm_depend_t *dep, -               const char *causingpkg) +               const char *causingpkg, const char *description)  {        alpm_depmissing_t *miss;

@@ -61,6 +61,7 @@ static alpm_depmissing_t *depmiss_new(const char *target, alpm_depend_t *dep,        STRDUP(miss->target, target, return NULL);        miss->depend = _alpm_dep_dup(dep);        STRDUP(miss->causingpkg, causingpkg, return NULL); +       STRDUP(miss->description, description, return NULL);

       return miss;  } @@ -70,6 +71,7 @@ void _alpm_depmiss_free(alpm_depmissing_t *miss)        _alpm_dep_free(miss->depend);        FREE(miss->target);        FREE(miss->causingpkg); +       FREE(miss->description);        FREE(miss);  }

@@ -283,11 +285,12 @@ alpm_pkg_t SYMEXPORT *alpm_find_satisfier(alpm_list_t *pkgs, const char *depstri  * @param remove an alpm_list_t* of packages to be removed  * @param upgrade an alpm_list_t* of packages to be upgraded (remove-then-upgrade)  * @param reversedeps handles the backward dependencies + * @param consider_optdeps handles optional dependencies instead of normal ones  * @return an alpm_list_t* of alpm_depmissing_t pointers.  */  alpm_list_t SYMEXPORT *alpm_checkdeps(alpm_handle_t *handle,                alpm_list_t *pkglist, alpm_list_t *remove, alpm_list_t *upgrade, -               int reversedeps) +               int reversedeps, int consider_optdeps)  {        alpm_list_t *i, *j;        alpm_list_t *dblist = NULL, *modified = NULL; @@ -326,7 +329,7 @@ alpm_list_t SYMEXPORT *alpm_checkdeps(alpm_handle_t *handle,                                _alpm_log(handle, ALPM_LOG_DEBUG, "checkdeps: missing dependency '%s' for package '%s'\n",                                                missdepstring, tp->name);                                free(missdepstring); -                               miss = depmiss_new(tp->name, depend, NULL); +                               miss = depmiss_new(tp->name, depend, NULL, NULL);                                baddeps = alpm_list_add(baddeps, miss);                        }                        release_filtered_depend(depend, nodepversion); @@ -338,8 +341,17 @@ alpm_list_t SYMEXPORT *alpm_checkdeps(alpm_handle_t *handle,                 * the packages listed in the requiredby field. */                for(i = dblist; i; i = i->next) {                        alpm_pkg_t *lp = i->data; -                       for(j = alpm_pkg_get_depends(lp); j; j = j->next) { -                               alpm_depend_t *depend = j->data; +                       j = consider_optdeps ? alpm_pkg_get_optdepends(lp) : alpm_pkg_get_depends(lp); +                       for(; j; j = j->next) { +                               alpm_depend_t *depend; +                               const char *description = NULL; +                               if(consider_optdeps) { +                                       alpm_optdepend_t *optdep = j->data; +                                       depend = optdep->depend; +                                       description = optdep->description; +                               } else { +                                       depend = j->data; +                               } OMG please unify depend_t and optdepend_t. :)

                               depend = filtered_depend(depend, nodepversion);                                alpm_pkg_t *causingpkg = find_dep_satisfier(modified, depend);                                /* we won't break this depend, if it is already broken, we ignore it */ @@ -350,10 +362,15 @@ alpm_list_t SYMEXPORT *alpm_checkdeps(alpm_handle_t *handle,                                   !find_dep_satisfier(dblist, depend)) {                                        alpm_depmissing_t *miss;                                        char *missdepstring = alpm_dep_compute_string(depend); -                                       _alpm_log(handle, ALPM_LOG_DEBUG, "checkdeps: transaction would break '%s' dependency of '%s'\n", -                                                       missdepstring, lp->name); +                                       if(consider_optdeps) { +                                               _alpm_log(handle, ALPM_LOG_DEBUG, "checkdeps: transaction would break '%s' optional dependency of '%s'\n", We're way over the normal line wrap width here, so please move the start of the string to the next line. +                                                                                       missdepstring, lp->name); +                                       } else { +                                               _alpm_log(handle, ALPM_LOG_DEBUG, "checkdeps: transaction would break '%s' dependency of '%s'\n", +                                                                                       missdepstring, lp->name); +                                       }                                        free(missdepstring); -                                       miss = depmiss_new(lp->name, depend, causingpkg->name); +                                       miss = depmiss_new(lp->name, depend, causingpkg->name, description);                                        baddeps = alpm_list_add(baddeps, miss);                                }                                release_filtered_depend(depend, nodepversion); @@ -801,7 +818,7 @@ int _alpm_resolvedeps(alpm_handle_t *handle, alpm_list_t *localpkgs,        for(i = alpm_list_last(*packages); i; i = i->next) {                alpm_pkg_t *tpkg = i->data;                targ = alpm_list_add(NULL, tpkg); -               deps = alpm_checkdeps(handle, localpkgs, remove, targ, 0); +               deps = alpm_checkdeps(handle, localpkgs, remove, targ, 0, 0);                alpm_list_free(targ);

               for(j = deps; j; j = j->next) { diff --git a/lib/libalpm/error.c b/lib/libalpm/error.c index 044dec7..5283392 100644 --- a/lib/libalpm/error.c +++ b/lib/libalpm/error.c @@ -136,6 +136,8 @@ const char SYMEXPORT *alpm_strerror(alpm_errno_t err)                /* Dependencies */                case ALPM_ERR_UNSATISFIED_DEPS:                        return _("could not satisfy dependencies"); +               case ALPM_ERR_UNSATISFIED_OPTDEPS: +                       return _("could not satisfy optional dependencies");                case ALPM_ERR_CONFLICTING_DEPS:                        return _("conflicting dependencies");                case ALPM_ERR_FILE_CONFLICTS: diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index d7e06bc..012f9c8 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -102,7 +102,7 @@ static int remove_prepare_cascade(alpm_handle_t *handle, alpm_list_t *lp)                alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free);                alpm_list_free(lp);                lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(handle->db_local), -                               trans->remove, NULL, 1); +                               trans->remove, NULL, 1, 0);        }        return 0;  } @@ -133,7 +133,7 @@ static void remove_prepare_keep_needed(alpm_handle_t *handle, alpm_list_t *lp)                alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free);                alpm_list_free(lp);                lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(handle->db_local), -                               trans->remove, NULL, 1); +                               trans->remove, NULL, 1, 0);        }  }

@@ -164,7 +164,7 @@ int _alpm_remove_prepare(alpm_handle_t *handle, alpm_list_t **data)                EVENT(handle, ALPM_EVENT_CHECKDEPS_START, NULL, NULL);

               _alpm_log(handle, ALPM_LOG_DEBUG, "looking for unsatisfied dependencies\n"); -               lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(db), trans->remove, NULL, 1); +               lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(db), trans->remove, NULL, 1, 0);                if(lp != NULL) {

                       if(trans->flags & ALPM_TRANS_FLAG_CASCADE) { @@ -206,6 +206,16 @@ int _alpm_remove_prepare(alpm_handle_t *handle, alpm_list_t **data)

       if(!(trans->flags & ALPM_TRANS_FLAG_NODEPS)) {                EVENT(handle, ALPM_EVENT_CHECKDEPS_DONE, NULL, NULL); +               lp = alpm_checkdeps(handle, _alpm_db_get_pkgcache(db), trans->remove, NULL, 1, 1); +               if(lp != NULL) { +                       if(data) { +                               *data = lp; +                       } else { +                               alpm_list_free_inner(lp, (alpm_list_fn_free)_alpm_depmiss_free); +                               alpm_list_free(lp); +                       } +                       RET_ERR(handle, ALPM_ERR_UNSATISFIED_OPTDEPS, -1); +               } We should not be checking deps, whether optional or required, after we have issued CHECKDEPS_DONE. This can't be done here, you need to move this to a more logical place earlier in teh method.

       }

       return 0; diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 3817ec8..b501807 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -605,7 +605,7 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data)        if(!(trans->flags & ALPM_TRANS_FLAG_NODEPS)) {                _alpm_log(handle, ALPM_LOG_DEBUG, "checking dependencies\n");                deps = alpm_checkdeps(handle, _alpm_db_get_pkgcache(handle->db_local), -                               trans->remove, trans->add, 1); +                               trans->remove, trans->add, 1, 0);                if(deps) {                        handle->pm_errno = ALPM_ERR_UNSATISFIED_DEPS;                        ret = -1; diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index cb97a4a..40e6786 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -101,6 +101,7 @@ static alpm_list_t *check_arch(alpm_handle_t *handle, alpm_list_t *pkgs)  int SYMEXPORT alpm_trans_prepare(alpm_handle_t *handle, alpm_list_t **data)  {        alpm_trans_t *trans; +       int retval = 0;

       /* Sanity checks */        CHECK_HANDLE(handle, return -1); @@ -127,7 +128,12 @@ int SYMEXPORT alpm_trans_prepare(alpm_handle_t *handle, alpm_list_t **data)        if(trans->add == NULL) {                if(_alpm_remove_prepare(handle, data) == -1) {                        /* pm_errno is set by _alpm_remove_prepare() */ -                       return -1; +                       /* UNSATISFIED_OPTDEPS is nonfatal. */ +                       if(alpm_errno(handle) == ALPM_ERR_UNSATISFIED_OPTDEPS) { +                               retval = -1; +                       } else { +                         return -1; +                       } I don't like this special case at all, -1 from me implemented this way. Please use callbacks and resolve it during checkdeps resolution, rather than whatever we have going on here.

               }        }       else {                if(_alpm_sync_prepare(handle, data) == -1) { @@ -138,7 +144,7 @@ int SYMEXPORT alpm_trans_prepare(alpm_handle_t *handle, alpm_list_t **data)

       trans->state = STATE_PREPARED;

-       return 0; +       return retval;  }

 /** Commit a transaction. */ diff --git a/src/pacman/remove.c b/src/pacman/remove.c index f56c1ec..739a6eb 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -111,8 +111,8 @@ int pacman_remove(alpm_list_t *targets)        /* Step 2: prepare the transaction based on its type, targets and flags */        if(alpm_trans_prepare(config->handle, &data) == -1) {                alpm_errno_t err = alpm_errno(config->handle); -               pm_printf(ALPM_LOG_ERROR, _("failed to prepare transaction (%s)\n"), -                       alpm_strerror(err)); +               retval = 1; +                switch(err) {                        case ALPM_ERR_PKG_INVALID_ARCH:                                for(i = data; i; i = alpm_list_next(i)) { @@ -128,12 +128,27 @@ int pacman_remove(alpm_list_t *targets)                                        free(depstring);                                }                                break; +                       case ALPM_ERR_UNSATISFIED_OPTDEPS: +                               for(i = data; i; i = alpm_list_next(i)) { +                                       alpm_depmissing_t *miss = i->data; +                                       char *depstring = alpm_dep_compute_string(miss->depend); +                                       if(miss->description) { +                                               printf(_(":: %s: optionally requires %s (%s)\n"), miss->target, depstring, miss->description); +                                       } else { +                                               printf(_(":: %s: optionally requires %s\n"), miss->target, depstring); +                                       } +                                       free(depstring); +                               } +                               retval = 0; +                               break;                        default:                                break;                }                FREELIST(data); -               retval = 1; -               goto cleanup; +               if(retval) { +                       pm_printf(ALPM_LOG_ERROR, _("failed to prepare transaction (%s)\n"), alpm_strerror(err)); +                       goto cleanup; +               }        }

       /* Search for holdpkg in target list */ diff --git a/src/util/testdb.c b/src/util/testdb.c index b15bbe5..e1401cc 100644 --- a/src/util/testdb.c +++ b/src/util/testdb.c @@ -99,7 +99,7 @@ static int checkdeps(alpm_list_t *pkglist)        alpm_list_t *data, *i;        int ret = 0;        /* check dependencies */ -       data = alpm_checkdeps(handle, pkglist, NULL, pkglist, 0); +       data = alpm_checkdeps(handle, pkglist, NULL, pkglist, 0, 0);        for(i = data; i; i = alpm_list_next(i)) {                alpm_depmissing_t *miss = i->data;                char *depstring = alpm_dep_compute_string(miss->depend); -- 1.7.7.3