[pacman-dev] [PATCH] Add more information in conflicts question
Sebastian Nowicki
sebnow at gmail.com
Tue Apr 12 07:00:53 EDT 2011
On Tue, Apr 12, 2011 at 2:46 AM, Dan McGee <dpmcgee at gmail.com> wrote:
> On Sun, Apr 10, 2011 at 5:58 AM, Sebastian Nowicki <sebnow at gmail.com>
> wrote:
> > In addition to the names of the conflicting packages, the origin and
> > versions will be displayed to the user.
> >
> > This introduces an API change in the PM_TRANS_CONV_CONFLICT_PKG
> > conversation callback. All three arguments were strings. The first
> > two arguments are now pmpkg_t structures (sync and local, respectively).
> >
> > Fixes FS#12536
> > ---
> >
> > In FS#12536 there was some discussion about changing pmconflict_t to
> > contain pmpkg_t structures rather than just strings. This would
> > be a much larger change, which I didn't think was worthwhile for
> > a cosmetic change.
> >
> > Even so, this does introduce an API change, and a sneaky one at that.
> > The conversation callback had three string arguments, now it has two
> > pmpkg_t and a string. Due to these being all void pointers, end-users
> > won't get any compiler warnings/errors if they don't update their code!
>
> Any consideration to just building the relevant string in the backend
> so we don't need 1) an API change and 2) the awfully ugly code in the
> frontend to print what you are printing?
>
This would work for pacman, but I don't think it's ideal for other
front-ends (especially GUI). As a front-end developer, I'd prefer having the
data in the callback rather than pre-processed information. I guess another
way of getting this information without changing the API is to search for
these packages in the sync/local databases. This could be inaccurate though,
and quite inefficient.
> I also notice this fixes the callback case, but the other two cases
> touched by 12b55958d8884bd are left the same. Not quite sure what to
> make of that though.
>
I think to do this properly the pmconflict_t should have a reference to the
packages rather than their names. It may be problematic to keep this up to
date (in case the pmpkg_t is freed, although I believe the database/cache is
responsible for this?). These could then be accessed easily and the relevant
information can be printed. Obviouslysame could be done as with with the
conversation callback, but it's a bit of a hack.
I didn't modify those other parts since the conversation callback is the
only place where user interaction is required, and in my opinion it's not
crucial to have so much detail in the conflict error (thought it would be
nice for consistency).
> > Sample output without the patch applied:
> >
> > $ sudo pacman -S qemu-kvm
> > resolving dependencies...
> > looking for inter-conflicts...
> > :: qemu-kvm and qemu are in conflict. Remove qemu? [y/N] n
> > error: unresolvable package conflicts detected
> > error: failed to prepare transaction (conflicting dependencies)
> > :: qemu-kvm and qemu are in conflict
> >
> > Sample output with the patch applied:
> >
> > $ sudo ./src/pacman/pacman -S qemu-kvm
> > resolving dependencies...
> > looking for inter-conflicts...
> > :: extra/qemu-kvm-0.14.0-1 and qemu-0.14.0-1 are in conflict. Remove
> qemu? [y/N] n
> > error: unresolvable package conflicts detected
> > error: failed to prepare transaction (conflicting dependencies)
> > :: qemu-kvm and qemu are in conflict
> >
> > This patch was generated against master (commit d3d18a).
> >
> >
> > lib/libalpm/sync.c | 4 ++--
> > src/pacman/callback.c | 31 +++++++++++++++++++++----------
> > 2 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> > index 5428e40..4b54969 100644
> > --- a/lib/libalpm/sync.c
> > +++ b/lib/libalpm/sync.c
> > @@ -486,8 +486,8 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t
> *db_local, alpm_list_t *dbs_sync
> > pmpkg_t *sync = _alpm_pkg_find(trans->add,
> conflict->package1);
> > pmpkg_t *local =
> _alpm_db_get_pkgfromcache(db_local, conflict->package2);
> > int doremove = 0;
> > - QUESTION(trans, PM_TRANS_CONV_CONFLICT_PKG,
> conflict->package1,
> > -
> conflict->package2, conflict->reason, &doremove);
> > + QUESTION(trans, PM_TRANS_CONV_CONFLICT_PKG, sync,
> local,
> > + conflict->reason,
> &doremove);
> > if(doremove) {
> > /* append to the removes list */
> > _alpm_log(PM_LOG_DEBUG, "electing '%s' for
> removal\n", conflict->package2);
> > diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> > index 08c1cf3..b253546 100644
> > --- a/src/pacman/callback.c
> > +++ b/src/pacman/callback.c
> > @@ -266,19 +266,30 @@ void cb_trans_conv(pmtransconv_t event, void
> *data1, void *data2,
> > alpm_pkg_get_name(data2));
> > break;
> > case PM_TRANS_CONV_CONFLICT_PKG:
> > - /* data parameters: target package, local
> package, conflict (strings) */
> > + /* data parameters: target package, local package
> (pmpkg_t), conflict (string) */
> > /* print conflict only if it contains new
> information */
> > - if(strcmp(data1, data3) == 0 || strcmp(data2,
> data3) == 0) {
> > - *response = noyes(_(":: %s and %s are in
> conflict. Remove %s?"),
> > - (char *)data1,
> > - (char *)data2,
> > - (char *)data2);
> > + if(strcmp(alpm_pkg_get_name(data1), data3) == 0
> > + ||
> strcmp(alpm_pkg_get_name(data2), data3) == 0) {
> > + *response = noyes(_(":: %s%s%s-%s and
> %s-%s are in conflict. Remove %s?"),
> > + /* The db will be NULL if
> the package was read from file. */
> > + alpm_pkg_get_db(data1) ==
> NULL ? "" : alpm_db_get_name(alpm_pkg_get_db(data1)),
> > + alpm_pkg_get_db(data1) ==
> NULL ? "" : "/",
> > + alpm_pkg_get_name(data1),
> > +
> alpm_pkg_get_version(data1),
> > + alpm_pkg_get_name(data2),
> > +
> alpm_pkg_get_version(data2),
> > +
> alpm_pkg_get_name(data2));
> > } else {
> > - *response = noyes(_(":: %s and %s are in
> conflict (%s). Remove %s?"),
> > - (char *)data1,
> > - (char *)data2,
> > + *response = noyes(_(":: %s%s%s-%s and
> %s-%s are in conflict (%s). Remove %s?"),
> > + /* The db will be NULL if
> the package was read from file. */
> > + alpm_pkg_get_db(data1) ==
> NULL ? "" : alpm_db_get_name(alpm_pkg_get_db(data1)),
> > + alpm_pkg_get_db(data1) ==
> NULL ? "" : "/",
> > + alpm_pkg_get_name(data1),
> > +
> alpm_pkg_get_version(data1),
> > + alpm_pkg_get_name(data2),
> > +
> alpm_pkg_get_version(data2),
> This makes me cringe a bit...we should have a helper function for
> this, not try to do it inline in a print format string.
>
Yes, it's quite horrible. I'll try to make something nicer.
> > (char *)data3,
> > - (char *)data2);
> > +
> alpm_pkg_get_name(data2));
> > }
> > break;
> > case PM_TRANS_CONV_REMOVE_PKGS:
> > --
> > 1.7.4.4
>
>
More information about the pacman-dev
mailing list