FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux Pacman Development

 
 
LinkBack Thread Tools
 
Old 04-10-2011, 10:58 AM
Sebastian Nowicki
 
Default Add more information in conflicts question

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!

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
", 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),
(char *)data3,
- (char *)data2);
+ alpm_pkg_get_name(data2));
}
break;
case PM_TRANS_CONV_REMOVE_PKGS:
--
1.7.4.4
 
Old 04-11-2011, 06:46 PM
Dan McGee
 
Default Add more information in conflicts question

On Sun, Apr 10, 2011 at 5:58 AM, Sebastian Nowicki <sebnow@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?

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.

> 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
", 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.

> * * * * * * * * * * * * * * * * * * * * * * * *(char *)data3,
> - * * * * * * * * * * * * * * * * * * * * * * * (char *)data2);
> + * * * * * * * * * * * * * * * * * * * * * * * alpm_pkg_get_name(data2));
> * * * * * * * * * * * *}
> * * * * * * * * * * * *break;
> * * * * * * * *case PM_TRANS_CONV_REMOVE_PKGS:
> --
> 1.7.4.4
 
Old 04-12-2011, 11:00 AM
Sebastian Nowicki
 
Default Add more information in conflicts question

On Tue, Apr 12, 2011 at 2:46 AM, Dan McGee <dpmcgee@gmail.com> wrote:

> On Sun, Apr 10, 2011 at 5:58 AM, Sebastian Nowicki <sebnow@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
", 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
>
>
 
Old 04-12-2011, 11:06 AM
Sebastian Nowicki
 
Default Add more information in conflicts question

On Tue, Apr 12, 2011 at 7:00 PM, Sebastian Nowicki <sebnow@gmail.com> wrote:

> On Tue, Apr 12, 2011 at 2:46 AM, Dan McGee <dpmcgee@gmail.com> wrote:
>
>> On Sun, Apr 10, 2011 at 5:58 AM, Sebastian Nowicki <sebnow@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.
>

On second thought, this would be a good compromise. If front-ends require
the pmpkg_t structure, it can be added later on. There's little harm in
changing the strings, although, in a way, this would still be an API change
(if the front-end relies on the string being the name for some reason).
 
Old 04-17-2011, 09:03 AM
Sebastian Nowicki
 
Default Add more information in conflicts question

In addition to the names of the conflicting packages, the origin and
versions will be displayed to the user.

This introduces a slight API change in the PM_TRANS_CONV_CONFLICT_PKG
conversation callback. The format of the first two strings has changed
from package names to strings of the format "db/name-version".

Fixes FS#12536
---

I rewrote the patch to pre-format the package information rather than
changing the API completely, as per Dan's suggestion. This still
introduces a bit of an API change as the two string can no longer be
used as package identifiers, e.g. in alpm_db_get_pkg().

$ sudo ./src/pacman/pacman -S qemu-kvm
resolving dependencies...
looking for inter-conflicts...
:: extra/qemu-kvm-0.14.0-1 and local/qemu-0.14.0-1 are in conflict (qemu). Remove local/qemu-0.14.0-1? [y/N] n
error: unresolvable package conflicts detected
error: failed to prepare transaction (conflicting dependencies)
:: qemu-kvm and qemu are in conflict

lib/libalpm/sync.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index 5428e40..90eeb5d 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -299,6 +299,47 @@ static int compute_download_size(pmpkg_t *newpkg)
return 0;
}

+/** Format package details to "db/name-version".
+ *
+ * If the package's origin is not found, it will be omitted
+ * (i.e. "name-version").
+ *
+ * The returned string must be freed by the caller.
+ *
+ * Sets pm_errno on error.
+ *
+ * @param pkg the package which details are to be formatted.
+ * @return Formatting package string, or NULL on error.
+ */
+static char *format_full_pkgname(pmpkg_t *pkg) {
+ char *full_pkgname;
+ size_t size = 1; /* Reserve for NULL */
+ pmdb_t *origin;
+
+ ASSERT(pkg != NULL, RET_ERR(PM_ERR_WRONG_ARGS, NULL));
+
+ /* Calulate length and allocate memory */
+ origin = alpm_pkg_get_db(pkg);
+ if(origin != NULL) {
+ size += strlen(alpm_db_get_name(origin)) + 1; /* Additional for '/' */
+ }
+ size += strlen(alpm_pkg_get_name(pkg)) + 1; /* Additional for hyphen */
+ size += strlen(alpm_pkg_get_version(pkg));
+ MALLOC(full_pkgname, size, RET_ERR(PM_ERR_MEMORY, NULL));
+
+ if(origin != NULL) {
+ snprintf(full_pkgname, size, "%s/%s-%s",
+ alpm_db_get_name(origin),
+ alpm_pkg_get_name(pkg),
+ alpm_pkg_get_version(pkg));
+ } else {
+ snprintf(full_pkgname, size, "%s-%s",
+ alpm_pkg_get_name(pkg),
+ alpm_pkg_get_version(pkg));
+ }
+ return full_pkgname;
+}
+
int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync, alpm_list_t **data)
{
alpm_list_t *deps = NULL;
@@ -485,12 +526,14 @@ 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);
+ char *sync_pkgname = format_full_pkgname(sync);
+ char *local_pkgname = format_full_pkgname(local);
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_pkgname, local_pkgname,
+ conflict->reason, &doremove);
if(doremove) {
/* append to the removes list */
- _alpm_log(PM_LOG_DEBUG, "electing '%s' for removal
", conflict->package2);
+ _alpm_log(PM_LOG_DEBUG, "electing '%s' for removal
", local_pkgname);
sync->removes = alpm_list_add(sync->removes, local);
} else { /* abort */
_alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected
"));
@@ -506,6 +549,8 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync
alpm_list_free(deps);
goto cleanup;
}
+ FREE(sync_pkgname);
+ FREE(local_pkgname);
}
EVENT(trans, PM_TRANS_EVT_INTERCONFLICTS_DONE, NULL, NULL);
alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_conflict_free);
--
1.7.4.4
 
Old 04-17-2011, 09:08 AM
Sebastian Nowicki
 
Default Add more information in conflicts question

On Sun, Apr 17, 2011 at 5:03 PM, Sebastian Nowicki <sebnow@gmail.com> wrote:

> In addition to the names of the conflicting packages, the origin and
> versions will be displayed to the user.
>
> This introduces a slight API change in the PM_TRANS_CONV_CONFLICT_PKG
> conversation callback. The format of the first two strings has changed
> from package names to strings of the format "db/name-version".
>
> Fixes FS#12536
> ---
>
> I rewrote the patch to pre-format the package information rather than
> changing the API completely, as per Dan's suggestion. This still
> introduces a bit of an API change as the two string can no longer be
> used as package identifiers, e.g. in alpm_db_get_pkg().
>
> $ sudo ./src/pacman/pacman -S qemu-kvm
> resolving dependencies...
> looking for inter-conflicts...
> :: extra/qemu-kvm-0.14.0-1 and local/qemu-0.14.0-1 are in conflict (qemu).
> Remove local/qemu-0.14.0-1? [y/N] n


Hmm, just noticed this breaks the pkgname == data3 comparison:

> if(strcmp(data1, data3) == 0 || strcmp(data2, data3) == 0) {

So the ".. are in conflict (package_name)" bit will always be shown in the
pacman callback. Not sure how to work around this other than to modify the
reason to have the same formatting as well. It seems odd that the package
name is given in general, but I don't know the reasons why it was done that
way.
 
Old 04-17-2011, 09:50 AM
Rémy Oudompheng
 
Default Add more information in conflicts question

On Sun 17 April 2011 at 17:03 +0800, Sebastian Nowicki wrote:
> In addition to the names of the conflicting packages, the origin and
> versions will be displayed to the user.
>
> This introduces a slight API change in the PM_TRANS_CONV_CONFLICT_PKG
> conversation callback. The format of the first two strings has changed
> from package names to strings of the format "db/name-version".
>
> Fixes FS#12536
> ---
>
> I rewrote the patch to pre-format the package information rather than
> changing the API completely, as per Dan's suggestion. This still
> introduces a bit of an API change as the two string can no longer be
> used as package identifiers, e.g. in alpm_db_get_pkg().
>
> $ sudo ./src/pacman/pacman -S qemu-kvm
> resolving dependencies...
> looking for inter-conflicts...
> :: extra/qemu-kvm-0.14.0-1 and local/qemu-0.14.0-1 are in conflict (qemu). Remove local/qemu-0.14.0-1? [y/N] n
> error: unresolvable package conflicts detected
> error: failed to prepare transaction (conflicting dependencies)
> :: qemu-kvm and qemu are in conflict
>
> lib/libalpm/sync.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 48 insertions(+), 3 deletions(-)

I am totally against this. Really really totally.

I personally think a formatting function for package information has
strictly nothing to do in sync.c, and maybe not even in libalpm.
Second, it would introduce additional parsing effort from front-ends to
split against the passed string if they want to display information in a
different way.

And what output do you expect if a conflict arises when running
pacman -U ?? What do you think is the repository from a package loaded
from a file.

I don't even understand why we would want to display any sort of version
information. I don't see how it would help me answer yes or no to that
conflict question either.

That is in my mind the job of the front-end: if you really
want information, it should display the list of selected packages
(repository/pkgname-pkgver) that comes between the parsing of the
command-line and the beginning of the transaction.

For example:

cb_trans_conv (from pacman) should be patched to display :
:: target qemu-kvm conflicts with installed qemu. Remove qemu ?

which is perfectly clear: qemu is installed and pacman wants you to
remove it, "local/qemu" brings no additional information, and the version
number is usually meaningless. The callback returns precisely one local
package and one target package, it can be reflected in the user output.

If you need to display the version number, then I don't feel like
messing with libalpm's internals is the right answer. I would probably
prefer have process_targname() patched (in pacman/sync.c), so that
after running alpm_find_dbs_satisfier(), it prints something.

If I run "pacman -S x-server", I want to see :
"selecting extra/xorg-server providing x-server"
(the case where alpm_find_dbs_satisfier() returns a package whose
name differs from the requested name => any further references
to xorg-server refer to extra/xorg-server and it's totally clear)

If I run "pacman -S kernel26", I'd like to see :
"selecting testing/kernel26-2.6.39-1 (over extra/kernel26-2.6.38.2-1)"
(the case where the pkgname exists in multiple DBs)

I think it should remove most ambiguities with least intrusive changes.

However, I think modifying the callback to return pmpkg_t is not a bad
idea, but then the formatting function should go in pacman. I just want
to understand the first reason why we are doing this. The bug report is
totally inconsistent (the title is inconsistent with the described
problem and so on), and it seems we are trying to solve some imaginary
problem.

The best thing we can probably do is make libalpm functions return more
information about their internal processing. Modifying formatting will
not help that. And if people want to know why some package suddenly
appears in the target list, I fear there is no solution.

--
Rémy.
 
Old 04-17-2011, 03:40 PM
Sebastian Nowicki
 
Default Add more information in conflicts question

On Sun, Apr 17, 2011 at 5:50 PM, Rémy Oudompheng
<remyoudompheng@gmail.com> wrote:
>
> On Sun 17 April 2011 at 17:03 +0800, Sebastian Nowicki wrote:
> > In addition to the names of the conflicting packages, the origin and
> > versions will be displayed to the user.
> >
> > This introduces a slight API change in the PM_TRANS_CONV_CONFLICT_PKG
> > conversation callback. The format of the first two strings has changed
> > from package names to strings of the format "db/name-version".
> >
> > Fixes FS#12536
> > ---
> >
> > I rewrote the patch to pre-format the package information rather than
> > changing the API completely, as per Dan's suggestion. This still
> > introduces a bit of an API change as the two string can no longer be
> > used as package identifiers, e.g. in alpm_db_get_pkg().
> >
> > $ sudo ./src/pacman/pacman -S qemu-kvm
> > resolving dependencies...
> > looking for inter-conflicts...
> > :: extra/qemu-kvm-0.14.0-1 and local/qemu-0.14.0-1 are in conflict (qemu). Remove local/qemu-0.14.0-1? [y/N] n
> > error: unresolvable package conflicts detected
> > error: failed to prepare transaction (conflicting dependencies)
> > :: qemu-kvm and qemu are in conflict
> >
> > *lib/libalpm/sync.c | * 51 ++++++++++++++++++++++++++++++++++++++++++++++++---
> > *1 files changed, 48 insertions(+), 3 deletions(-)
>
> I am totally against this. Really really totally.
>
> I personally think a formatting function for package information has
> strictly nothing to do in sync.c, and maybe not even in libalpm.
> Second, it would introduce additional parsing effort from front-ends to
> split against the passed string if they want to display information in a
> different way.

Agreed, but the alternatives require changes to the API (char * -> pmpkg_t).

> And what output do you expect if a conflict arises when running
> pacman -U ?? What do you think is the repository from a package loaded
> from a file.

Just the package name and version. Having a "file/" prefix would be
meaningless, and if you're installing from a single file, you're
likely to know what you're installing anyway.

> I don't even understand why we would want to display any sort of version
> information. I don't see how it would help me answer yes or no to that
> conflict question either.
>
> That is in my mind the job of the front-end: if you really
> want information, it should display the list of selected packages
> (repository/pkgname-pkgver) that comes between the parsing of the
> command-line and the beginning of the transaction.
>
> For example:
>
> cb_trans_conv (from pacman) should be patched to display :
> *:: target qemu-kvm conflicts with installed qemu. Remove qemu ?
>
> which is perfectly clear: qemu is installed and pacman wants you to
> remove it, "local/qemu" brings no additional information, and the version
> number is usually meaningless. The callback returns precisely one local
> package and one target package, it can be reflected in the user output.

If the version of the local package is newer than the one being
installed, it would be good to inform the user (as would have been
likely with the libdrm-git package). The "local/" prefix can easily be
omitted.

> If you need to display the version number, then I don't feel like
> messing with libalpm's internals is the right answer. I would probably
> prefer have process_targname() patched (in pacman/sync.c), so that
> after running alpm_find_dbs_satisfier(), it prints something.

I think that changing the API of the callback to use pmpkg_t, (or even
pmconflict_t if that's exposed) rather than a string, is the right
thing to do. The front-end shouldn't have to find the package again to
get its metadata (what if there are multiple packages with the same
name?). For instance a GUI front-end may want to show the description
of the package or something. It's a pretty drastic change considering
the result is cosmetic.

> However, I think modifying the callback to return pmpkg_t is not a bad
> idea, but then the formatting function should go in pacman. I just want
> to understand the first reason why we are doing this. The bug report is
> totally inconsistent (the title is inconsistent with the described
> problem and so on), and it seems we are trying to solve some imaginary
> problem.
>
> The best thing we can probably do is make libalpm functions return more
> information about their internal processing. Modifying formatting will
> not help that. And if people want to know why some package suddenly
> appears in the target list, I fear there is no solution.

In which case the API needs to change. I don't think cosmetic changes
are worth doing so, but that's a decision for the devs.
 
Old 04-17-2011, 04:49 PM
Rémy Oudompheng
 
Default Add more information in conflicts question

On Sun 17 April 2011 at 23:40 +0800, Sebastian Nowicki wrote:
> On Sun, Apr 17, 2011 at 5:50 PM, Rémy Oudompheng
> <remyoudompheng@gmail.com> wrote:
> >
> > I am totally against this. Really really totally.
> >
> > I personally think a formatting function for package information has
> > strictly nothing to do in sync.c, and maybe not even in libalpm.
> > Second, it would introduce additional parsing effort from front-ends to
> > split against the passed string if they want to display information in a
> > different way.
>
> Agreed, but the alternatives require changes to the API (char * -> pmpkg_t).

I think we disagree in what we call an API change. All the suggested
patches are API changes in the sens that they change the behaviour of a
function. Even if its type signature does not change, the behaviour
changes in the sense that the nature of what is returned changes.

As you already noticed, going from a simple package name to a complex
string changes a lot of things when considering what a user (developer)
may want to do with it.

The only advantage is keeping binary compatibility for several
applications, e.g. pacman, but that is a coincidence.

> > And what output do you expect if a conflict arises when running
> > pacman -U ?? What do you think is the repository from a package loaded
> > from a file.
>
> Just the package name and version. Having a "file/" prefix would be
> meaningless, and if you're installing from a single file, you're
> likely to know what you're installing anyway.

Is there is a reason you don't know what you are installing in other
cases? Possible from the point of view of the user, not very improbable
from the point of view of the program using libalpm.

> > I don't even understand why we would want to display any sort of version
> > information. I don't see how it would help me answer yes or no to that
> > conflict question either.
> >
> > That is in my mind the job of the front-end: if you really
> > want information, it should display the list of selected packages
> > (repository/pkgname-pkgver) that comes between the parsing of the
> > command-line and the beginning of the transaction.
> >
> > For example:
> >
> > cb_trans_conv (from pacman) should be patched to display :
> > *:: target qemu-kvm conflicts with installed qemu. Remove qemu ?
> >
> > which is perfectly clear: qemu is installed and pacman wants you to
> > remove it, "local/qemu" brings no additional information, and the version
> > number is usually meaningless. The callback returns precisely one local
> > package and one target package, it can be reflected in the user output.
>
> If the version of the local package is newer than the one being
> installed, it would be good to inform the user (as would have been
> likely with the libdrm-git package). The "local/" prefix can easily be
> omitted.

I don't see why the version information is particularly more relevant
than other things. Personnally I'm interested in knowing whether my local
package comes from an official repository, if so, whether it's in
testing... Things that still not solved using such a partial approach.

Does knowing the version really helps you in answering yes or no ???

> > If you need to display the version number, then I don't feel like
> > messing with libalpm's internals is the right answer. I would probably
> > prefer have process_targname() patched (in pacman/sync.c), so that
> > after running alpm_find_dbs_satisfier(), it prints something.
>
> I think that changing the API of the callback to use pmpkg_t, (or even
> pmconflict_t if that's exposed) rather than a string, is the right
> thing to do. The front-end shouldn't have to find the package again to
> get its metadata (what if there are multiple packages with the same
> name?).

The front-end doesn't have to do anything magic to find what packages are
in the transaction. Simple calls to alpm_find_satisfier() and
alpm_trans_get_add() suffice to find the culprit package (even if
of course having the whole pmpkg_t structure would be better).

> > However, I think modifying the callback to return pmpkg_t is not a bad
> > idea, but then the formatting function should go in pacman. I just want
> > to understand the first reason why we are doing this. The bug report is
> > totally inconsistent (the title is inconsistent with the described
> > problem and so on), and it seems we are trying to solve some imaginary
> > problem.
> >
> > The best thing we can probably do is make libalpm functions return more
> > information about their internal processing. Modifying formatting will
> > not help that. And if people want to know why some package suddenly
> > appears in the target list, I fear there is no solution.
>
> In which case the API needs to change. I don't think cosmetic changes
> are worth doing so, but that's a decision for the devs.

Again, the patch indeed *is* an API change, and as yourself say,
isn't worth the trouble for what should be a cosmetic patch for pacman.

--
Rémy.
 
Old 04-19-2011, 11:53 AM
Sebastian Nowicki
 
Default Add more information in conflicts question

On Mon, Apr 18, 2011 at 12:49 AM, Rémy Oudompheng
<remyoudompheng@gmail.com> wrote:
> On Sun 17 April 2011 at 23:40 +0800, Sebastian Nowicki wrote:
>> On Sun, Apr 17, 2011 at 5:50 PM, Rémy Oudompheng
>> <remyoudompheng@gmail.com> wrote:
>> >
>> > I am totally against this. Really really totally.
>> >
>> > I personally think a formatting function for package information has
>> > strictly nothing to do in sync.c, and maybe not even in libalpm.
>> > Second, it would introduce additional parsing effort from front-ends to
>> > split against the passed string if they want to display information in a
>> > different way.
>>
>> Agreed, but the alternatives require changes to the API (char * -> pmpkg_t).
>
> I think we disagree in what we call an API change. All the suggested
> patches are API changes in the sens that they change the behaviour of a
> function. Even if its type signature does not change, the behaviour
> changes in the sense that the nature of what is returned changes.
>
> As you already noticed, going from a simple package name to a complex
> string changes a lot of things when considering what a user (developer)
> may want to do with it.
>
> The only advantage is keeping binary compatibility for several
> applications, e.g. pacman, but that is a coincidence.

We do agree. I mentioned that it's an API change in the patch. I guess
I was referring to ABI rather than API as only the semantics changed
in this patch.

>> > And what output do you expect if a conflict arises when running
>> > pacman -U ?? What do you think is the repository from a package loaded
>> > from a file.
>>
>> Just the package name and version. Having a "file/" prefix would be
>> meaningless, and if you're installing from a single file, you're
>> likely to know what you're installing anyway.
>
> Is there is a reason you don't know what you are installing in other
> cases? Possible from the point of view of the user, not very improbable
> from the point of view of the program using libalpm.

The best example is `pacman -Syu` in which case hundreds of packages
can be updated. For me this is the most common instance of replaces
taking affect.

>> > I don't even understand why we would want to display any sort of version
>> > information. I don't see how it would help me answer yes or no to that
>> > conflict question either.
>> >
>> > That is in my mind the job of the front-end: if you really
>> > want information, it should display the list of selected packages
>> > (repository/pkgname-pkgver) that comes between the parsing of the
>> > command-line and the beginning of the transaction.
>> >
>> > For example:
>> >
>> > cb_trans_conv (from pacman) should be patched to display :
>> > *:: target qemu-kvm conflicts with installed qemu. Remove qemu ?
>> >
>> > which is perfectly clear: qemu is installed and pacman wants you to
>> > remove it, "local/qemu" brings no additional information, and the version
>> > number is usually meaningless. The callback returns precisely one local
>> > package and one target package, it can be reflected in the user output.
>>
>> If the version of the local package is newer than the one being
>> installed, it would be good to inform the user (as would have been
>> likely with the libdrm-git package). The "local/" prefix can easily be
>> omitted.
>
> I don't see why the version information is particularly more relevant
> than other things. Personnally I'm interested in knowing whether my local
> package comes from an official repository, if so, whether it's in
> testing... Things that still not solved using such a partial approach.

I never stated it was more relevant. The origin is already shown. What
other information are you referring to?

> Does knowing the version really helps you in answering yes or no ???

This is what the feature request was for.

>> > If you need to display the version number, then I don't feel like
>> > messing with libalpm's internals is the right answer. I would probably
>> > prefer have process_targname() patched (in pacman/sync.c), so that
>> > after running alpm_find_dbs_satisfier(), it prints something.
>>
>> I think that changing the API of the callback to use pmpkg_t, (or even
>> pmconflict_t if that's exposed) rather than a string, is the right
>> thing to do. The front-end shouldn't have to find the package again to
>> get its metadata (what if there are multiple packages with the same
>> name?).
>
> The front-end doesn't have to do anything magic to find what packages are
> in the transaction. Simple calls to alpm_find_satisfier() and
> alpm_trans_get_add() suffice to find the culprit package (even if
> of course having the whole pmpkg_t structure would be better).

In my opinion this is only a workaround to prevent an API change
(which is reasonable). It still requires traversing through lists in
order to find the package. It may not be "magic", but it is
unnecessary effort to find what is already available in the
_alpm_sync_prepare() function. I'll write up a patch if this is the
preferred method though.
 

Thread Tools




All times are GMT. The time now is 04:06 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright ©2007 - 2008, www.linux-archive.org